-
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
Adding upgrade settings to all node pool blocks #10376
Adding upgrade settings to all node pool blocks #10376
Conversation
This comment has been minimized.
This comment has been minimized.
Thanks @evenh for doing the rebase here 👍 |
Co-authored-by: Even Holthe <[email protected]>
3b58b7a
to
0ea3e92
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.
hey @evenh
Thanks for rebasing this PR & apologies for the delayed review @jmcshane
I've taken a look through and whilst on the whole this looks pretty good I've left some comments inline - since I've got a number of other Kubernetes PR's I'm hoping to get in today, so that this doesn't conflict I hope you don't mind but so that we can get this merged I'm going to push a commit to fix the PR comments, then ask @jackofallops to take another look.
Thanks!
azurerm/internal/services/containers/kubernetes_cluster_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_node_pool_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_node_pool_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_node_pool_resource.go
Outdated
Show resolved
Hide resolved
3438ed4
to
540d4f3
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 @evenh and @tombuildsstuff - Couple comments and questions below, otherwise LGTM 👍
if profile.Type != "" { | ||
agentPoolProfile["type"] = string(profile.Type) | ||
agentType = string(profile.Type) |
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.
Since this is a string, not a pointer, we could just reference it directly in the return below?
if profile.VMSize != "" { | ||
agentPoolProfile["vm_size"] = string(profile.VMSize) | ||
vmSize = string(profile.VMSize) |
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.
As above.
if profile.OsType != "" { | ||
agentPoolProfile["os_type"] = string(profile.OsType) | ||
osType = string(profile.OsType) |
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.
As above.
Check: resource.ComposeTestCheckFunc( | ||
check.That(data.ResourceName).ExistsInAzure(r), | ||
check.That(data.ResourceName).Key("upgrade_settings.#").HasValue("1"), | ||
check.That(data.ResourceName).Key("upgrade_settings.0.max_surge").HasValue("2"), |
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 should be 4?
Check: resource.ComposeTestCheckFunc( | ||
check.That(data.ResourceName).ExistsInAzure(r), | ||
check.That(data.ResourceName).Key("default_node_pool.0.upgrade_settings.#").HasValue("1"), | ||
check.That(data.ResourceName).Key("default_node_pool.0.upgrade_settings.0.max_surge").HasValue("4"), |
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.
Should be 2
here?
Thanks for the review @tombuildsstuff and @jackofallops. I've pushed a commit that addresses the latest comments. |
Thanks for that @evenh 👍 |
This has been released in version 2.48.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.48.0"
}
# ... other configuration ... |
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! |
Adding upgrade settings to all Kubernetes cluster node pool blocks.
make test
passes locally andTestAccKubernetesClusterNodePool_upgradeSettings
acceptance test passes.Rebased version of #8123 with some minor fixes. Resolves #7892.