-
Notifications
You must be signed in to change notification settings - Fork 4.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
azurerm_nginx_deployment - support for the auto_scale_profile property #24950
azurerm_nginx_deployment - support for the auto_scale_profile property #24950
Conversation
This PR stays as a draft which is pending #24868 |
f4181fe
to
60a9541
Compare
TODOs:
|
1fb4d1f
to
a9b7452
Compare
a9b7452
to
0d16ef4
Compare
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.
Thanks for this PR @puneetsarna. I think there is a breaking change in the way the existing capacity
property has been modified to support the new auto_scale_profile
block. I also have some suggestions around property naming and the documentation. If you could take a look and address those comments left in-line then we can take another look through this.
0d16ef4
to
101b636
Compare
101b636
to
9d7054d
Compare
9d7054d
to
34a0e8f
Compare
26c7bd1
to
81ec2f1
Compare
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.
Thanks @puneetsarna, a few more minor comments around the spacing in the tests and on leaving a comment on the desired behaviour for the next major release. Once those are addressed this should be good to go!
ValidateFunc: validation.IntPositive, | ||
Type: pluginsdk.TypeInt, | ||
Optional: true, | ||
ConflictsWith: []string{"auto_scale_profile"}, |
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.
If the desired behaviour is ExactlyOneOf
then we usually wrap this around the features.FourPointOh
flag or at the very least leave a TODO 4.0
comment so that maintainers know a breaking change needs to be introduced here for the next major release
ConflictsWith: []string{"auto_scale_profile"}, | |
// TODO 4.0 replace this with ExactlyOneOf | |
ConflictsWith: []string{"auto_scale_profile"}, |
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.
👍 Thanks for sharing this. After an internal chat within our team, I think we'd like to leave the breaking change off entirely for now as we are also evaluating other changes as well which might impact some behavior in Terraform.
Given it's unclear from our end around what those changes might be and the associated timelines, this comment might be confusing, and I am going to leave it off for the time being.
81ec2f1
to
439de7a
Compare
Hi @stephybun Thanks for the helping out with the review and for sharing the details around breaking changes. I have addressed all the feedback you shared. Please let me know if there is anything else 👍 |
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.
Thanks @puneetsarna LGTM 🍰
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Acceptance test: