-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Add support for "StepScaling" autoscaling policies. #4277
Conversation
Whoops. I know what's up here. Will correct later tonight. |
8c31f46
to
bae0fd5
Compare
@catsby -- not sure what to make of that build error. Best I can tell it's not at all related to my changes, which pass both Acc and unit tests locally. |
return nil, fmt.Errorf( | ||
"metric_interval_lower_bound isn't a string. This is a bug. Please file an issue.") | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the record, I really hate this code. As a schema.Set
, we lose the ability to distinguish between an uninitialized float (i.e. 0
) and a user-declared value of 0
in the state file for TypeFloat
elements. But the StepScaling policies critically depend on absent boundaries -- AWS treats missing boundaries as positive and negative infinity, which apply to a ton of use cases.
Hence I made them strings... which have to be parsed into floats... I hate it too. I'm open to any suggestions to rework this function more sanely, but I can't find any other way to distinguish between a legitimate 0.0
from a user and an empty boundary value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is agreed! currently, 0's are really tough to work with in Terraform :( The only way around this would be to set default values of 0 and then test for those and them treat as excluded
Any word on a merge date for this, @catsby, @radeksimko? We'd like to start using this functionality but we avoid custom versions of TF if we can so as not to poison our state. Totally happy to use the tip of |
Hey @phinze, @radeksimko, @catsby -- any word on whether or not this could be merged? We'd love to start using this functionality but I really don't want to fork TF and potentially munge up our state files if I can help it. |
@catsby -- can you offer me any update on this one? |
👍 This would be awesome! |
@ctiwald can you rebase this please so I can get it merged after a review? |
Unlike SimpleScaling policies, StepScaling policies require one or more "steps", which are interval ranges in which a tracked metric can lie. Policies can then execute scaling adjustments wedded to these steps. This commit also adds a slew of additional policy attributes which are only applicable to step policies.
bae0fd5
to
f5c898e
Compare
Hi @ctiwald the PR looks good. Going to run the acceptance tests to make sure all looks good. Pending review, I will merge :) Btw, loving that you have added the deprecation warnings to match that of AWS! 👍 P. |
Tests look good!!!
|
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. |
Amazon added this policy type last spring, but terraform doesn't support it. This PR would implement "StepScaling" policies for autoscaling groups.
This took far more doing than I anticipated. AWS doesn't provide a secondary API endpoint for step policy management. It simply overloads the current policy API, and then switches certain attributes on and off depending on the policy type. I added support for every existing policy attribute, and a fair amount of validation within resource_aws_autoscaling_policy to confirm user input is correct.
The resource is further complicated by a nested Schema which doesn't exist outside the context of these API calls. In all this was surprisingly complicated, but I think I've got something that works. I wouldn't mind a little 3rd party testing -- the edge cases mired me for a while during development.