-
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
"azurerm_kubernetes_cluster" - supports "max_node_provisioning_time","max_unready_percentage" and "max_unready_nodes" #11406
"azurerm_kubernetes_cluster" - supports "max_node_provisioning_time","max_unready_percentage" and "max_unready_nodes" #11406
Conversation
…x_total_unready_percentage" and "ok_total_unready_count"
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!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍
azurerm/internal/services/containers/kubernetes_cluster_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/containers/kubernetes_cluster_resource.go
Outdated
Show resolved
Hide resolved
@njuCZ About the default values, I'm a bit concerned if some existing resource has different default values being set when they created the resource with old provider, then they'll get a diff in the new provider. Though this is not likely to happen if the service keeps the behavior consistent. |
@magodo the simplest way is to make them typeString, empty string will make the backend service return the default value. This is a little common for aks. But I also agrees that exlipcit type is more helpful. To make it TypeInt, we should set a default value, because 0 is also a valid value. |
If they are O+C, I assume the default is not necessary? |
If it's TypeString, O+C has no problem. but if we set it as TypeInt, it will have a default value 0, 0 is a valid value for this property, and the backend service will think client want to set it as 0 and does not return the default value |
Ah.. Because those properties are inside a nested block, which means there is no way to tell whether they are omitted in the config or not.. |
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 @njuCZ - just a couple comments
@katbyte thanks for your opinion, I have renamed those fields. |
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 @njuCZ - LGTM 👍
This has been released in version 2.57.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.57.0"
}
# ... other configuration ... |
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. |
This PR adds support for the
max_node_provision_time
,max_total_unready_percentage
andok_total_unready_count
property in theauto_scaler_profile block
forazurerm_kubernetes_cluster
.As per https://docs.microsoft.com/en-us/azure/aks/cluster-autoscaler#using-the-autoscaler-profile
the original acctest
TestAccKubernetesCluster_autoScalingProfile
fails because the version "1.19.6" is not available in the westeurope