provider/aws: Add autoscaling_policy + cloudwatch_metric_alarm

[radeksimko]

This is @xaptronic's work from #1585 which I merely took and squashed those 39 commits to 2 + rebased latest changes from master, so it's up to date and can be eventually merged without any conflicts.

Test plan

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=AutoscalingPolicy' 2>/dev/null

go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=AutoscalingPolicy -timeout 90m
=== RUN TestAccAWSAutoscalingPolicy_basic
--- PASS: TestAccAWSAutoscalingPolicy_basic (256.27s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    256.286s

$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=CloudWatchMetricAlarm' 2>/dev/null

go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=CloudWatchMetricAlarm -timeout 90m
=== RUN TestAccAWSCloudWatchMetricAlarm_basic
--- PASS: TestAccAWSCloudWatchMetricAlarm_basic (2.41s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    2.423s

[phinze]

@radeksimko since you're the one with most recent context - can you give a quick rundown of remaining nits? if no time, that's 👌

[radeksimko]

@phinze Yeah, I intend to do so for sure, I'm just not sure when. 😺

Either way I know I will need these resources in the project I'm working on at some point, so it's motivating. 😃

[radeksimko]

After reading through all the code & playing with this for a while I decided to:

1. Add a few links pointing to AWS docs inside the docs pages, namely CloudWatch metrics.
2. Fine-tune some log messages to make it easier for potential debugging (e.g. change "Put" to "Create" & "Update" so we can recognise the difference)
3. Modify the autoscaling policy example and add the following note to docs:

I can't think of any example when people would want to use both desired_capacity and scaling policy - each time you'd run apply and the current "scaled" capacity would differ, Terraform would scale it back down or up.

Before merging this in, I'd like to hear some feedback to the following:

• policy_arn => shall we rename this to arn? It will always be referenced w/ the resource name - i.e. ${aws_autoscaling_policy.bat.policy_arn} (it should be obvious that it's ARN of the policy)
• cloudwatch_metric_alarm.actions_enabled is optional and by default false, which effectively means that alarms won't cause any scaling/notifications by default. I believe that the parameter will mainly be used for disabling alarms temporarily and it should be enabled (true) by default.

Other than that, it works 👌 and after solving the two questions above it's ready to be merged.

[mikeyhill]

no.2 I would vote that true should be the default and only explicitly disabled. Thanks for this btw, I was just thinking this a.m. about how I was going to manage these alarms.

[radeksimko]

Thanks for this btw

Credit goes mostly to @xaptronic I've just done a few enhancements to otherwise good work 😃

[radeksimko]

@phinze @catsby Any ideas on #2201 (comment) - i.e. policy_arn & cloudwatch_metric_alarm.actions_enabled?

The PR is almost ready to be merged when we clear out these two small things.

[xaptronic]

@radeksimko

I'm +1 on renaming policy_arn to arn - I had named it as present to match up with the AWS api.

I gather the idea behind Terraform is not only to provide cardboard and glue around APIs but to also make it easier and better to work with these APIs. I don't see why setting actions_enabled to default true would be a problem, other than a divergence from the base API, but it enables the common use case which is the point of terraform right?

[phinze]

I gather the idea behind Terraform is not only to provide cardboard and glue around APIs but to also make it easier and better to work with these APIs

This is most definitely true! However, we're trying to walk a fine line of improving UX wherever we can without introducing too many foreign abstractions or new behavior.

I think the policy_arn -> arn rename is definitely worth doing to remove stutter, but I'm a bit more hesitant to change the default on an upstream field.

We have precedent of diverging from AWS APIs in certain cases where it makes life easier for a TF user (e.g. security_group.description). I believe this seems to be another case where it's warranted, so I'd be good to flip the default like you suggest @radeksimko.

To restate the general case: we should always default to the upstream API, and only diverge where it clearly improves UX for Terraform users and does not unreasonably increase the potential for confusion. It's a balance between applying the "Principle of Least Astonishment" to Terraform users unfamiliar with AWS and Terraform users very familiar with AWS.

tl;dr - +1 on both counts @radeksimko 😀

[radeksimko]

OK, thank you for the feedback & @phinze's explanation, both things fixed, merging in...

[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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.