-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add check to skip toggling CloudWatch alarms #3682
Conversation
8467b5d
to
484de3c
Compare
Full-stack documentation: https://docs.openverse.org/_preview/3682 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. Changed files 🔄: |
b6e7c7f
to
0b26dff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving to unblock local testing of the data refreshes -- good catch @AetherUnbound, I did not think of that! I do think it would be preferable to actually skip the tasks if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the drive-by review but I find the name SKIP_TOGGLING...
a bit confusing like a multiple negative. I feel like ENABLE_TOGGLING...
or TOGGLE...
would be clearer. This would default to true
so that the outcome is the same as SKIP_TOGGLING...
that defaults to false
.
559c231
to
f2f08b3
Compare
This is good to go now! Thanks for the suggestions; they are excellent! I am asking for a review from @AetherUnbound, as you may want to check this too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got a few small, suggested changes. Thanks for modifying this to use the idiomatic approach! 😄
9d49c65
to
a00ae33
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Co-authored-by: Madison Swain-Bowden <bowdenm@spu.edu>
a00ae33
to
88b45ea
Compare
@AetherUnbound Ah, good catch! They were supposed to inherit the TriggerRule from the group task 🤔 Anyway, I changed it. |
88b45ea
to
86ae58a
Compare
Based on the medium urgency of this PR, the following reviewers are being gently reminded to review this PR: @dhruvkb Excluding weekend1 days, this PR was ready for review 4 day(s) ago. PRs labelled with medium urgency are expected to be reviewed within 4 weekday(s)2. @krysal, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates look great, I'm really excited to get this back in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Local testing now works well for me with skipping the alarms steps, thanks for fixing that! This is good to go IMO!
Fixes
Follow up to #3652.
Description
@AetherUnbound mentioned in the PR above a problem running the data refresh locally when AWS credentials aren't set (as is usual locally, since they are not necessary): #3652 (comment)
This PR adds an Airflow variable,
AIRFLOW_VAR_TOGGLE_CLOUDWATCH_ALARMS
, to decide when to actually modify Cloudwatch alarms, and change the connection to CloudWatch to use a more idiomatic Airflow AWS connection with a hook.Testing Instructions
Connection
AIRFLOW_CONN_AWS_CLOUDWATCH
variable with the required credentials as shown in theenv.example
file. You could also generate it using this snippet to create Connection and convert to URI.Config for whether to toggle the alarm
Header menu > Admin > Variables
, with the valuefalse
.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin