issue #4350 change datapointsToAlarm attribute in aws_cloudwatch_metric_alarm

[saravanan30erd]

Fixes #4350

Changes proposed in this pull request:

• Change 1
  changed datapointsToAlarm to computed attribute.
• Change 2
  Set validation for minimum value.
  https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_PutMetricAlarm.html

Output from acceptance testing:

$ 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 (36.07s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_basic
--- PASS: TestAccAWSCloudWatchMetricAlarm_basic (28.09s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm
--- PASS: TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm (33.13s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_treatMissingData
--- PASS: TestAccAWSCloudWatchMetricAlarm_treatMissingData (49.53s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles
--- PASS: TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles (51.51s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_extendedStatistic
--- PASS: TestAccAWSCloudWatchMetricAlarm_extendedStatistic (28.78s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_missingStatistic
--- PASS: TestAccAWSCloudWatchMetricAlarm_missingStatistic (8.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	235.760s
...

[bflad]

changed datapointsToAlarm to computed attribute.

I'm not sure we want to disable Terraform's drift detection for the attribute. Someone can modify the alarm outside of Terraform and re-configure it for M out of N (in Terraform terms, a different datapoints_to_alarm from evaluation_period), which Terraform should show as a difference if its not defined in their Terraform configuration. It seems more valid to ignore the difference when the API value for datapoints_to_alarm matches evaluation_period and Terraform wants to set the attribute to the schema.TypeInt zero value.

[saravanan30erd]

@bflad then its better to ignore this difference. But I think its better to add validation for minimum value as per API doc(https://docs.aws.amazon.com/AmazonCloudWatch/latest/APIReference/API_PutMetricAlarm.html).
what do you think?

[bflad]

We can add the validation, but it would not resolve the linked issue. I'll mark the PR to not close it.

[bflad]

Thanks @saravanan30erd! 🚀

7 tests passed (all tests)
=== RUN   TestAccAWSCloudWatchMetricAlarm_missingStatistic
--- PASS: TestAccAWSCloudWatchMetricAlarm_missingStatistic (2.79s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm
--- PASS: TestAccAWSCloudWatchMetricAlarm_datapointsToAlarm (6.66s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_extendedStatistic
--- PASS: TestAccAWSCloudWatchMetricAlarm_extendedStatistic (6.81s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_basic
--- PASS: TestAccAWSCloudWatchMetricAlarm_basic (6.96s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_importBasic
--- PASS: TestAccAWSCloudWatchMetricAlarm_importBasic (7.29s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_treatMissingData
--- PASS: TestAccAWSCloudWatchMetricAlarm_treatMissingData (11.36s)
=== RUN   TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles
--- PASS: TestAccAWSCloudWatchMetricAlarm_evaluateLowSampleCountPercentiles (11.55s)

[saravanan30erd]

@bflad is there any possible solution for this issue?

[bflad]

Theoretically, it can be done via DiffSuppressFunc returning true when the old value matches d.Get("evaluation_periods").(string) and new being reset to its zero value.

The new acceptance test to cover this behavior will need to create an alarm in its first step (omitting datapoints_to_alarm), then in the second step:

• PreConfig: simulate the console behavior of updating the alarm to set datapoints_to_alarm to match evaluation_periods
• Config same configuration as the first step
• PlanOnly: true to ensure no updates are expected

[bflad]

This has been released in version 1.27.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

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!