-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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: Cycle node-pool if node_pool has node level relevant changes #21719
azurerm_kubernetes_cluster: Cycle node-pool if node_pool has node level relevant changes #21719
Conversation
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 @jkroepke.
Aside from the comments left in-line we would also need to add a test for these. Once that's done we can take another look through this!
0bd7398
to
2d95415
Compare
internal/services/containers/kubernetes_cluster_scaling_resource_test.go
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_scaling_resource_test.go
Outdated
Show resolved
Hide resolved
I hope, everything fits now. |
2a804f9
to
f5c05a0
Compare
Looks fine on my site now. Mention that I add more properties here, since. See also #21762 which also request |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
7cc1679
to
0453b54
Compare
Rebased |
|
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 extending this to additional properties @jkroepke. I left some comments in-line which need to be addresed - once those are resolved I will run the tests.
Co-authored-by: stephybun <[email protected]>
Co-authored-by: stephybun <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
Signed-off-by: Jan-Otto Kröpke <[email protected]>
@stephybun Issues are resolved. |
Signed-off-by: Jan-Otto Kröpke <[email protected]>
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 your effort on this @jkroepke. Hope you don't mind I pushed a minor update to the docs adding the Note from the review to the default_node_pool
block.
The tests also look good so this is good to go! LGTM 🌵
You are welcome. One less review cycle :) |
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. |
Follow up from #20628
At the moment, only vm_size is support for the builtin cycle of node pool. I would also like to go this way, if
os_disk_type
has been changed from Managed to Ephemeral and back.the
os_disk_type
,os_sku
,os_disk_size_gb
property of thedefault_node_pool
is no longer ForceNew and can be resized by specifyingtemporary_name_for_rotation