-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Avoid creating alarms for zero thresholds #40
Avoid creating alarms for zero thresholds #40
Conversation
@adamantike sorry for the delay in reviewing this! Practically, this looks fine to me, but do you think it could be a breaking change for some users? |
@adamantike also it looks like the workflows may be out of date. Would you mind running:
Thanks |
I just updated the workflows and Readme, thanks for the heads up! I don't consider this a breaking change, but a fix instead. Threshold variables are positive integers by default, so this code will only affect users that have explicitly set them as zero. For those users, I assume zero is being used to disable the alarm for the low thresholds, as they wouldn't be triggering with the current implementation in |
@joe-niland, gentle ping. Let me know if there's anything else I can do to move this forward. |
/terratest |
@adamantike Waiting on #41 so the tests will run |
@adamantike @joe-niland is merged. Can you update this pr ? |
This change allows users to disable some of the alarms, by providing zero threshold. At the moment, setting zero as threshold (e.g. to disable alarms on low CPU utilization) still creates the alarm, and incurs in CloudWatch cost for a no-op alarm.
4c22357
to
f1d499f
Compare
Thanks @adamantike for creating this pull request! A maintainer will review your changes shortly. Please don't be discouraged if it takes a while. While you wait, make sure to review our contributor guidelines. Tip Need help or want to ask for a PR review to be expedited?Join us on Slack in the |
Just rebased this PR! |
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.
LGTM
/terratest |
Could someone clarify when it is going to be released? |
Ah @rkul this should have already been released, but seems there is an issue with our automation. We'll look into it. I'll circle back. |
@Gowiem hello! Just wanted to kindly ask do you have any updates here? |
I can see version |
what
This change allows users to disable some of the alarms, by providing zero threshold.
why
At the moment, setting zero as threshold (e.g. to disable alarms on low CPU utilization) still creates the alarm, and incurs in CloudWatch cost for a no-op alarm.