provider/aws: Autoscaling Metrics Collection resource

[stack72]

Scaffold the AWS Autoscaling Group Metrics Collection - #4683

Decided to make this as a separate resource to allow simple manipulation of the metrics collected without needing to change the Autoscaling Group. Added a computed type to the autoscaling group to add the enabled_metrics to the asg state

• Schema
• CRUD
• Acceptance Tests
• Documentation

Test results:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=AWSAutoscalingMetricsCollection' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
go generate ./...
TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSAutoscalingMetricsCollection -timeout 120m
=== RUN   TestAccAWSAutoscalingMetricsCollection_basic
--- PASS: TestAccAWSAutoscalingMetricsCollection_basic (39.79s)
=== RUN   TestAccAWSAutoscalingMetricsCollection_update
--- PASS: TestAccAWSAutoscalingMetricsCollection_update (47.82s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    87.631s

[catsby]

Let's check the error on this, recently bit by this

[catsby]

This looks good! I have some questions/comments but other than that 👍

[catsby]

if possible, we should add a test for this in structure_test.go

[stack72]

This is ready for final review. Metrics can only be enabled / updated / disabled for an ASG. There is no way that there can be multiple terraform configs for an ASG metrics collection

[phinze]

Metrics can only be enabled / updated / disabled for an ASG. There is no way that there can be multiple terraform configs for an ASG metrics collection

This makes me rethink the utility of having this as a separate resource. Seems like it just makes for possible confusion if a user accidentally has two of these floating around in the same config.

Whereas if they were just fields on the ASG there's no possibility for confusion:

resource "aws_autoscaling_group" "foo" {
  metrics = [...]
  metric_granularity = "1Minute"
}

That seems much simpler to me - happy to discuss further though!

[stack72]

@phinze having it as a separate resource allows the user to start managing the metrics for non-managed terraform resources now

[phinze]

Ah yes this is true!

Though I think that argument could be made for any pair of separated API requests. e.g. we have to call asg.AttachLoadBalancers, should we implement aws_autoscaling_group_load_balancers so users can manage load balancers on ASGs not in Terraform as well?

I don't think partially managing resources is necessarily a good enough reason to keep the separation, especially when the relationship is so 1:1. Handling these fields just like the load_balancers field seems more consistent and much simpler to me. What do you think?

[stack72]

@phinze combining now :)

[stack72]

@phinze merged the resources so ready for you when you want :)

[phinze]

Solid, LGTM!

[stack72]

jsut a FYI for the PR

ake testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSAutoScalingGroup' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSAutoScalingGroup -timeout 120m
=== RUN   TestAccAWSAutoScalingGroup_basic
--- PASS: TestAccAWSAutoScalingGroup_basic (223.98s)
=== RUN   TestAccAWSAutoScalingGroup_autoGeneratedName
--- PASS: TestAccAWSAutoScalingGroup_autoGeneratedName (44.96s)
=== RUN   TestAccAWSAutoScalingGroup_terminationPolicies
--- PASS: TestAccAWSAutoScalingGroup_terminationPolicies (77.61s)
=== RUN   TestAccAWSAutoScalingGroup_tags
--- PASS: TestAccAWSAutoScalingGroup_tags (264.83s)
=== RUN   TestAccAWSAutoScalingGroup_VpcUpdates
--- PASS: TestAccAWSAutoScalingGroup_VpcUpdates (78.50s)
=== RUN   TestAccAWSAutoScalingGroup_WithLoadBalancer
--- PASS: TestAccAWSAutoScalingGroup_WithLoadBalancer (454.80s)
=== RUN   TestAccAWSAutoScalingGroup_withMetrics
--- PASS: TestAccAWSAutoScalingGroup_withMetrics (43.77s)
PASS
github.com/hashicorp/terraform/builtin/providers/aws    1835.674s

[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.