Adding upgrade settings to all node pool blocks #10376
Conversation
ghost
added
This comment has been minimized.
This comment has been minimized.
|
Thanks @evenh for doing the rebase here 👍 |
Co-authored-by: Even Holthe <even.holthe@me.com>
evenh
force-pushed
the
feature/node-pool-upgrade-settings
branch
from
3b58b7a
to
0ea3e92
Compare
|
|
There was a problem hiding this comment.
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
tombuildsstuff
force-pushed
the
feature/node-pool-upgrade-settings
branch
from
3438ed4
to
540d4f3
Compare
There was a problem hiding this comment.
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.
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) |
| if profile.OsType != "" { | ||
| agentPoolProfile["os_type"] = string(profile.OsType) | ||
| osType = string(profile.OsType) |
| 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"), |
| 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"), |
|
Thanks for the review @tombuildsstuff and @jackofallops. I've pushed a commit that addresses the latest comments. |
|
Thanks for that @evenh 👍 |
|
Tests pass:
|
|
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 👉 hashibot-feedback@hashicorp.com. Thanks! |
Adding upgrade settings to all Kubernetes cluster node pool blocks.
make testpasses locally andTestAccKubernetesClusterNodePool_upgradeSettingsacceptance test passes.Rebased version of #8123 with some minor fixes. Resolves #7892.