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

azurerm_nginx_deployment - support for the auto_scale_profile property #24950

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

puneetsarna
Copy link
Contributor

@puneetsarna puneetsarna commented Feb 21, 2024

Acceptance test:

terraform-provider-azurerm $ make acctests SERVICE='nginx' TESTARGS='-run=TestAccNginxDeployment_autoscaling' TESTTIMEOUT='60m'
==> Checking that code complies with gofmt requirements...
==> Checking that Custom Timeouts are used...
==> Checking that acceptance test packages are used...
TF_ACC=1 go test -v ./internal/services/nginx -run=TestAccNginxDeployment_autoscaling -timeout 60m -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccNginxDeployment_autoscaling
=== PAUSE TestAccNginxDeployment_autoscaling
=== CONT  TestAccNginxDeployment_autoscaling
--- PASS: TestAccNginxDeployment_autoscaling (490.74s)
PASS
ok  	github.com/hashicorp/terraform-provider-azurerm/internal/services/nginx	493.336s

@puneetsarna
Copy link
Contributor Author

This PR stays as a draft which is pending #24868

@puneetsarna puneetsarna force-pushed the ps-dev-nginx-autoscaling branch 2 times, most recently from f4181fe to 60a9541 Compare February 24, 2024 01:51
@puneetsarna
Copy link
Contributor Author

puneetsarna commented Feb 24, 2024

TODOs:

  • user facing docs. ✅
  • Update tests to account for capacity/autoscale ExactlyOneOf. ✅
  • Run autoscaling tests. ✅

@puneetsarna puneetsarna force-pushed the ps-dev-nginx-autoscaling branch 2 times, most recently from 1fb4d1f to a9b7452 Compare February 27, 2024 17:54
@puneetsarna puneetsarna marked this pull request as ready for review February 27, 2024 17:55
@puneetsarna puneetsarna changed the title Add autoscaling to NGINXaaS azurerm_nginx_deployment - support for the auto_scale_profile property Feb 27, 2024
Copy link
Member

@stephybun stephybun 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 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.

internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved
internal/services/nginx/nginx_deployment_resource_test.go Outdated Show resolved Hide resolved
internal/services/nginx/nginx_deployment_resource.go Outdated Show resolved Hide resolved
website/docs/r/nginx_deployment.html.markdown Outdated Show resolved Hide resolved
Copy link
Member

@stephybun stephybun left a 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!

internal/services/nginx/nginx_deployment_resource_test.go Outdated Show resolved Hide resolved
internal/services/nginx/nginx_deployment_resource_test.go Outdated Show resolved Hide resolved
ValidateFunc: validation.IntPositive,
Type: pluginsdk.TypeInt,
Optional: true,
ConflictsWith: []string{"auto_scale_profile"},
Copy link
Member

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

Suggested change
ConflictsWith: []string{"auto_scale_profile"},
// TODO 4.0 replace this with ExactlyOneOf
ConflictsWith: []string{"auto_scale_profile"},

Copy link
Contributor Author

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.

@puneetsarna
Copy link
Contributor Author

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 👍

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @puneetsarna LGTM 🍰

@stephybun stephybun merged commit 0442700 into hashicorp:main Mar 15, 2024
36 checks passed
@github-actions github-actions bot added this to the v3.97.0 milestone Mar 15, 2024
stephybun added a commit that referenced this pull request Mar 15, 2024
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants