r/aws_cloudwatch_metric_alarm: Support optional datapoints_to_alarm configuration #2609
Conversation
Ninir
added
the
enhancement
There was a problem hiding this comment.
Hi @bflad
This is looking very good! :)
$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSCloudWatchMetricAlarm_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCloudWatchMetricAlarm_ -timeout 120m
=== RUN TestAccAWSCloudWatchMetricAlarm_importBasic
--- PASS: TestAccAWSCloudWatchMetricAlarm_importBasic (24.11s)
=== RUN TestAccAWSCloudWatchMetricAlarm_basic
--- PASS: TestAccAWSCloudWatchMetricAlarm_basic (21.92s)
=== RUN TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm
--- PASS: TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm (21.49s)
=== RUN TestAccAWSCloudWatchMetricAlarm_treatMissingData
--- PASS: TestAccAWSCloudWatchMetricAlarm_treatMissingData (36.71s)
=== RUN TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles
--- PASS: TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles (37.45s)
=== RUN TestAccAWSCloudWatchMetricAlarm_extendedStatistic
--- PASS: TestAccAWSCloudWatchMetricAlarm_extendedStatistic (21.59s)
=== RUN TestAccAWSCloudWatchMetricAlarm_missingStatistic
--- PASS: TestAccAWSCloudWatchMetricAlarm_missingStatistic (5.75s)
PASS
ok github.com/terraform-providers/terraform-provider-aws/aws 169.084s
Just left a few nitpicks. Thanks!
| @@ -81,6 +81,7 @@ The following arguments are supported: | |||
| * `actions_enabled` - (Optional) Indicates whether or not actions should be executed during any changes to the alarm's state. Defaults to `true`. | |||
| * `alarm_actions` - (Optional) The list of actions to execute when this alarm transitions into an ALARM state from any other state. Each action is specified as an Amazon Resource Number (ARN). | |||
| * `alarm_description` - (Optional) The description for the alarm. | |||
| * `datapoints_to_alarm` - (Optional) The number of datapoints that must be breaching to trigger the alarm.. | |||
| evaluation_periods = "2" | ||
| metric_name = "CPUUtilization" | ||
| namespace = "AWS/EC2" | ||
| period = "120" |
There was a problem hiding this comment.
Oi! I have since turned on whitespace character visibility to prevent this in the future. 😄
| func testAccAWSCloudWatchMetricAlarmConfigDatapointsToAlarm(rName string) string { | ||
| return fmt.Sprintf(` | ||
| resource "aws_cloudwatch_metric_alarm" "foobar" { | ||
| alarm_name = "%s" |
There was a problem hiding this comment.
Shouldn't we use the same way to name resources as other tests (e.g. for future sweepers) in this file?
(it's just me trying to keep consistency between tests as much as possible! 😄 )
There was a problem hiding this comment.
No problem about this, I'll switch it to rInt for consistency with the other tests.
Something I think that would be really helpful in the future is switching the testing to include the test names so we can easily find "bad apples". Just jumped the gun a little bit.
There was a problem hiding this comment.
@bflad totally agree on that :)
Was just wondering on this specific question why we were using t.Name() rather than what we do elsewhere: you may had a good reason :)
* Use rInt and fix indentation in new datapoints_to_alarm test for consistency * Remove extraneous period in datapoints_to_alarm documentation
|
PR updated! 🚀 |
|
Hey @bflad Happy new year! :) This looks good to me! thanks for the work! 🚀 ⭐️ |
|
This has been released in terraform-provider-aws version 1.7.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
ghost
locked and limited conversation to collaborators
Prerequisite #2608
Closes #2607