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_kubernetes_cluster_node_pool - node_count, min_node, and max_node can now be set to 0 #8300

Merged
merged 14 commits into from
Oct 5, 2020

Conversation

zeroX-tj
Copy link
Contributor

This updates the validation rules for the new Azure SDK where you can create zero size nodepools

#8219

If there's anything else i should do, let me know, first PR. I tested it locally by using it.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @zeroX-tj, this change looks good but can we add a test to confirm we can make a node pool with a node count of 0 and another test for a min count of 0

@gdippolito
Copy link

I think this would be really nice to have. @zeroX-tj are you looking into addressing the comment?

@zeroX-tj
Copy link
Contributor Author

I think this would be really nice to have. @zeroX-tj are you looking into addressing the comment?

I'm really not so familiar with Go so i was planning in looking into it when i had more time. But that didn't happen yet.

@ghost ghost added size/M and removed size/XS labels Sep 18, 2020
@migolfi
Copy link

migolfi commented Sep 23, 2020

This looks great and it would be very helpful for some work I am currently on. Is there anything else needed on this?
It looks like it's waiting for feedback from @mbfrahry

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @zeroX-tj. These changes look good and the test changes works! We are missing two things after further review. max_count validation should also start at 0 since it can be greater than or equal to min_count and we need to update the docs to reflect the changes here and we'll be good to go. https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/website/docs/r/kubernetes_cluster_node_pool.html.markdown

@ghost ghost added the documentation label Sep 28, 2020
@zeroX-tj
Copy link
Contributor Author

Sorry for the reverts, was a bit confused by the failed builds but then i figured out it's because of cancelation :) Monday mornings

I think everything should be good now.

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM!

@mbfrahry mbfrahry changed the title Update min node count validation on aks nodepool azurerm_kubernetes_cluster_node_pool - node_count, min_node, and max_node can now be set to 0 Oct 5, 2020
@mbfrahry
Copy link
Member

mbfrahry commented Oct 5, 2020

Thanks for this @zeroX-tj!

@mbfrahry mbfrahry merged commit 09eaead into hashicorp:master Oct 5, 2020
mbfrahry added a commit that referenced this pull request Oct 5, 2020
@mbfrahry mbfrahry added this to the v2.31.0 milestone Oct 5, 2020
@ghost
Copy link

ghost commented Oct 8, 2020

This has been released in version 2.31.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.31.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Nov 5, 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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 5, 2020
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.

5 participants