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_scheduled_action: Fix bugs #8777

Merged
merged 8 commits into from
Mar 8, 2021
Merged

r/aws_appautoscaling_scheduled_action: Fix bugs #8777

merged 8 commits into from
Mar 8, 2021

Conversation

stefansundin
Copy link
Contributor

Do not default max_capacity and min_capacity to 0. Allow them to be left undefined to allow either one to be used individually.

Basically, it is currently impossible to use Terraform to create the following:

$ aws application-autoscaling put-scheduled-action --service-namespace rds --scalable-dimension rds:cluster:ReadReplicaCount --resource-id cluster:xxx --scheduled-action-name scale-down --schedule "cron(0 22 ? * MON-FRI *)" --scalable-target-action MinCapacity=0

$ aws application-autoscaling describe-scheduled-actions --service-namespace rds
{
    "ScheduledActions": [
        {
            "ScheduledActionName": "scale-down",
            "ScheduledActionARN": "arn:aws:autoscaling:xxxxxxx",
            "ServiceNamespace": "rds",
            "Schedule": "cron(0 22 ? * MON-FRI *)",
            "ResourceId": "cluster:xxx",
            "ScalableDimension": "rds:cluster:ReadReplicaCount",
            "ScalableTargetAction": {
                "MinCapacity": 0
            }
        }
    ]
}

Basically, this provider would set max_capacity = 0, which would very likely trigger the following error:

Error: ValidationException: Maximum capacity cannot be less than minimum capacity

This change allows for specifying only min_capacity, or only max_capacity.

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Release note for CHANGELOG:

- resource/aws_appautoscaling_scheduled_action: Do not default `max_capacity` and `min_capacity` to `0`, allow either to be used on its own.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSAppautoscalingScheduledAction_dynamo'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSAppautoscalingScheduledAction_dynamo -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAppautoscalingScheduledAction_dynamo
=== PAUSE TestAccAWSAppautoscalingScheduledAction_dynamo
=== CONT  TestAccAWSAppautoscalingScheduledAction_dynamo
--- PASS: TestAccAWSAppautoscalingScheduledAction_dynamo (127.86s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	127.916s

With only the test change, the following happened:

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSAppautoscalingScheduledAction_dynamo -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAppautoscalingScheduledAction_dynamo
=== PAUSE TestAccAWSAppautoscalingScheduledAction_dynamo
=== CONT  TestAccAWSAppautoscalingScheduledAction_dynamo
--- FAIL: TestAccAWSAppautoscalingScheduledAction_dynamo (112.68s)
    testing.go:568: Step 0 error: errors during apply:

        Error: ValidationException: Maximum capacity cannot be less than minimum capacity
        	status code: 400, request id: xxxxxx-xxxx-xxxx-xxxx-xxxxxxxxx

          on /var/folders/h6/r59_wnd905g_8s6x2mt7w0ch0000gn/T/tf-test995279429/main.tf line 22:
          (source code not available)


FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	112.733s
make: *** [testacc] Error 1

…eft undefined so that they can be used individually.
@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/applicationautoscaling tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels May 25, 2019
@stefansundin
Copy link
Contributor Author

@bflad If you have time, this one also needs a review.

@stefansundin
Copy link
Contributor Author

I have found more bugs in this resource.. For example, it does not detect changes (e.g. updated schedule), so it's currently only usable for the initial resource creation since it can't update any changes made.

Do you want me to add more fixes to this PR, or open two new smaller PRs?

…ate multiple scheduled actions with the same name if they target different resources. For example, you can have multiple scheduled actions named "scale-down" that target different Aurora clusters.
…updating these attributes, we can just put and it will overwrite the existing resource.
@ghost ghost added size/M Managed by automation to categorize the size of a PR. and removed size/XS Managed by automation to categorize the size of a PR. labels May 30, 2019
@stefansundin stefansundin changed the title r/aws_appautoscaling_scheduled_action: Do not default max_capacity and min_capacity to 0 r/aws_appautoscaling_scheduled_action: Fix bugs May 30, 2019
@stefansundin
Copy link
Contributor Author

Okay, I decided to put it all in here since it's all kind of dependent on each other. I tried to be as descriptive as possible in my commit messages. Let me know what needs tweaking.

And the test is still passing!

$ make testacc TESTARGS='-run=TestAccAWSAppautoscalingScheduledAction_dynamo'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSAppautoscalingScheduledAction_dynamo -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAppautoscalingScheduledAction_dynamo
=== PAUSE TestAccAWSAppautoscalingScheduledAction_dynamo
=== CONT  TestAccAWSAppautoscalingScheduledAction_dynamo
--- PASS: TestAccAWSAppautoscalingScheduledAction_dynamo (41.27s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	41.327s

@aeschright aeschright requested a review from a team June 28, 2019 18:15
@stefansundin
Copy link
Contributor Author

@ewbankkit @bflad This PR needs some love a review, please. :)

I think it's done and from what I remember everything was working when I created this PR (over 250 days ago!).

Base automatically changed from master to main January 23, 2021 00:56
@breathingdust breathingdust requested a review from a team as a code owner January 23, 2021 00:56
@gdavison gdavison self-assigned this Mar 5, 2021
Copy link
Contributor

@gdavison gdavison 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 the PR, @stefansundin. I've made some updates to the acceptance tests and made the Update call only update parameters that have changed 🚀

@ghost
Copy link

ghost commented Mar 12, 2021

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Apr 8, 2021

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 as resolved and limited conversation to collaborators Apr 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants