Skip to content
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

r/aws_appautoscaling_target: Support updating max_capacity, min_capacity, and role_arn attributes #2542

Closed

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Dec 5, 2017

The RegisterScalableTarget API call supports updating attributes on existing targets.

Closes #240 and potentially #968

make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppautoScalingTarget'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAppautoScalingTarget -timeout 120m
=== RUN   TestAccAWSAppautoScalingTarget_basic
--- PASS: TestAccAWSAppautoScalingTarget_basic (67.34s)
=== RUN   TestAccAWSAppautoScalingTarget_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingTarget_spotFleetRequest (69.89s)
=== RUN   TestAccAWSAppautoScalingTarget_emrCluster
--- PASS: TestAccAWSAppautoScalingTarget_emrCluster (556.08s)
=== RUN   TestAccAWSAppautoScalingTarget_multipleTargets
--- PASS: TestAccAWSAppautoScalingTarget_multipleTargets (32.41s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws   725.772s

@Ninir Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Dec 5, 2017
}

if d.HasChange("role_arn") {
input.RoleARN = aws.String(d.Get("role_arn").(string))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the role_arn update in tests so that we ensure it's all ok?
Seems rather good for me otherwise :)

* Add role_arn attribute testing
* Add update ValidationException retrying for IAM eventual consistency
@bflad
Copy link
Contributor Author

bflad commented Dec 11, 2017

@Ninir updated! Thanks for noticing that. I changed the test to actually update the IAM role and it triggered an IAM eventual consistency issue, which is now fixed with retries:

        * aws_appautoscaling_target.bar: 1 error(s) occurred:

        * aws_appautoscaling_target.bar: ValidationException: Unable to assume IAM role: arn:aws:iam::123456789012:role/autoscaleroleupdatecluster-tyavrhv2bh
            status code: 400, request id: eef62b85-deb0-11e7-84a9-6b6b83f6f609
 make testacc TEST=./aws TESTARGS='-run=TestAccAWSAppautoScalingTarget'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSAppautoScalingTarget -timeout 120m
=== RUN   TestAccAWSAppautoScalingTarget_basic
--- PASS: TestAccAWSAppautoScalingTarget_basic (91.72s)
=== RUN   TestAccAWSAppautoScalingTarget_spotFleetRequest
--- PASS: TestAccAWSAppautoScalingTarget_spotFleetRequest (59.53s)
=== RUN   TestAccAWSAppautoScalingTarget_emrCluster
--- PASS: TestAccAWSAppautoScalingTarget_emrCluster (2045.85s)
=== RUN   TestAccAWSAppautoScalingTarget_multipleTargets
--- PASS: TestAccAWSAppautoScalingTarget_multipleTargets (36.86s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	2234.006s

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the test.

One thing that I'd consider is to repurpose the existing resourceAwsAppautoscalingTargetCreate method (call it resourceAwsAppautoscalingTargetPut or something like that) instead of adding a new one since they seem to be doing the same thing.

It's not only going to be shorter/cleaner, but should address some confusions I noticed in the current Update implementation. It seems we'd be wiping existing max_capacity and/or min_capacity and/or role_arn if it's not being changed in a single update call.

Let me know what you think.

@@ -74,8 +72,8 @@ func resourceAwsAppautoscalingTargetCreate(d *schema.ResourceData, meta interfac
_, err = conn.RegisterScalableTarget(&targetOpts)

if err != nil {
if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == "ValidationException" {
log.Printf("[DEBUG] Retrying creation of Application Autoscaling Scalable Target due to possible issues with IAM: %s", awsErr)
if isAWSErr(err, "ValidationException", "Unable to assume IAM role") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 More precise matching, yey

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 8, 2018
@bflad
Copy link
Contributor Author

bflad commented Jan 11, 2018

Given the large amount of headache with the newly forced IAM service roles, I'm going to resubmit this PR, likely without role_arn bits.

@ghost
Copy link

ghost commented Apr 8, 2020

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 ghost locked and limited conversation to collaborators Apr 8, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws_appautoscaling_target update detaches any aws_appautoscaling_policy's
4 participants