-
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
New Resource: azurerm_kubernetes_cluster_extension
and azurerm_arc_kubernetes_cluster_extension
#21310
New Resource: azurerm_kubernetes_cluster_extension
and azurerm_arc_kubernetes_cluster_extension
#21310
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 @ms-zhenhua but I think this should be split out into two separate resources, one for managed clusters and one for arc clusters:
azurerm_kubernetes_cluster_extension
and azurerm_arc_kubernetes_cluster_extension
…r-azurerm into kubernetes-cluster-extension
azurerm_kubernetes_cluster_extension
azurerm_kubernetes_cluster_extension
and azurerm_arc_kubernetes_cluster_extension
ac86391
to
2ff1bbd
Compare
Hi @stephybun, thank you for your suggestion and I have splitted the resource into 2 resources. Kindly take another review. |
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.
Even though there are commonalities between these resources I think the schema should just be duplicated and the resources go into arckubernetes
and containers
respectively. That way the testdata/scripts we use for provisioning the arc resources can be reused for the arc extension.
On the whole this is looking good and we can take another look through once all the comments have been resolved and the resources moved into the arckubernetes
and containers
folder.
internal/services/kubernetesconfiguration/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/kubernetesconfiguration/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/kubernetesconfiguration/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/kubernetesconfiguration/arc_kubernetes_cluster_extension_resource_test.go
Outdated
Show resolved
Hide resolved
internal/services/kubernetesconfiguration/kubernetes_cluster_extension_common.go
Outdated
Show resolved
Hide resolved
internal/services/kubernetesconfiguration/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/kubernetesconfiguration/kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
Hi @stephybun, thank you for your review. I have updated the PR and put these resources into the |
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.
Appreciate you making those changes @ms-zhenhua and the tests are looking good! On my second pass through I picked up a few other minor things - but once those are resolved I think we should be able to merge this 🙂
internal/services/arckubernetes/arc_kubernetes_cluster_extension_resource.go
Show resolved
Hide resolved
internal/services/arckubernetes/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/arckubernetes/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/arckubernetes/arc_kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
internal/services/containers/kubernetes_cluster_extension_resource.go
Outdated
Show resolved
Hide resolved
Hi @stephybun, thank you for reviewing. I have updated the code and document. Kindly have another look. |
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 @ms-zhenhua LGTM 🥟
This functionality has been released in v3.54.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you! |
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. |
Swagger:
https://github.com/Azure/azure-rest-api-specs/blob/main/specification/kubernetesconfiguration/resource-manager/Microsoft.KubernetesConfiguration/stable/2022-11-01/extensions.json
Test Result: