-
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
kubernetes_cluster: Add support for private_dns_zone_id #10201
kubernetes_cluster: Add support for private_dns_zone_id #10201
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.
hey @favoretti
Thanks for this PR :)
I've taken a look through and left a few minor comments inline but this otherwise LGTM - I've merged #8737, so if we can rebase this then the tests should be good to run 👍
Thanks!
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
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.
LGTM 👍
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 @favoretti
I've run the tests for this and it appears the configuration is slightly out - but I think if we can update the field name then we should otherwise be good here 👍
Thanks!
azurerm/internal/services/containers/tests/kubernetes_cluster_network_resource_test.go
Outdated
Show resolved
Hide resolved
@tombuildsstuff This should work, although I haven't tested all the options. |
We need this! initial testing looks promising |
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 @favoretti
Thanks for pushing those changes - I've taken a look through and this is looking good, if we can remove the None
option and add an acceptance test covering System
(which I think'll require adjusting the validation logic in the Create) - this should otherwise be good to go 👍
Thanks!
azurerm/internal/services/containers/kubernetes_cluster_other_resource_test.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
azurerm/internal/services/containers/kubernetes_cluster_resource.go
Outdated
Show resolved
Hide resolved
|
|
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.
LGTM 👍
Running the tests for this, it appears that Private Clusters may need to explicitly set |
Push it to 2.47 otherwise? I can add more acceptance tests as well to see whether not setting it to anything works. |
Yeah, I have a feeling this only affects Private Clusters (I spotted this for |
Private AKS clusters should be able to use
So I assume the workflow would be to:
|
…ce.go Co-authored-by: Tom Harvey <[email protected]>
…ce.go Co-authored-by: Tom Harvey <[email protected]>
…ce.go Co-authored-by: Tom Harvey <[email protected]>
…network_resource_test.go Co-authored-by: Tom Harvey <[email protected]>
…resource_test.go Co-authored-by: Tom Harvey <[email protected]>
…ce.go Co-authored-by: Tom Harvey <[email protected]>
Co-authored-by: Tom Harvey <[email protected]>
…e from shooting themselves in the feet
This has been released in version 2.47.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.47.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! |
Fixes #10193
Blocked on #7979 #8737