Export FQDN for managed Kubernetes cluster #907
Conversation
|
|
| @@ -98,11 +103,6 @@ func resourceArmKubernetesCluster() *schema.Resource { | |||
| Computed: true, | |||
| }, | |||
|
|
|||
| "fqdn": { | |||
There was a problem hiding this comment.
By moving this out we will break existing terraform code.
| @@ -321,10 +322,6 @@ func flattenAzureRmKubernetesClusterAgentPoolProfiles(profiles *[]containerservi | |||
| agentPoolProfile["dns_prefix"] = *profile.DNSPrefix | |||
| } | |||
|
|
|||
| if profile.Fqdn != nil { | |||
There was a problem hiding this comment.
Could we just set this to profile.Fqdn for now?
There was a problem hiding this comment.
I'd be surprised that it would break existing code (profile.Fqdn has always been empty, so the field hasn't been useful), but I can make that change and deprecate it.
There was a problem hiding this comment.
Hello @phekmat,
Thank you for this PR, I noticed this issue myself last night. I've left a couple of comments in-line but the gist is if the FQDN property is removed from agent_pool_profile it will break existing terraform. If we can just keep it for now and set it to the parent value this should be good to merge.
Thanks!
- Set the FQDN for the Kubernetes cluster - Deprecate the FQDN on the agent_pool_profile as it's not populated by Azure for managed Kubernetes
|
I updated the code to deprecate the FQDN in the agent_pool_profile, but overlooked your latest comment. Should I go ahead and update the code to use the parent FQDN in the It feels counterproductive to me since the field will eventually be removed (and any current usages never worked to begin with), but I'm ok with making that change too. |
|
Yes please if you don't mind :) This is so we can merge the new field in sooner and removing the old field later, likely grouping with multiple breaking changes in a larger release. |
|
Updated. Thanks for the review and feedback! |
| @@ -260,7 +260,7 @@ func resourceArmKubernetesClusterRead(d *schema.ResourceData, meta interface{}) | |||
| return fmt.Errorf("Error setting `linux_profile`: %+v", err) | |||
| } | |||
|
|
|||
| agentPoolProfiles := flattenAzureRmKubernetesClusterAgentPoolProfiles(resp.ManagedClusterProperties.AgentPoolProfiles) | |||
| agentPoolProfiles := flattenAzureRmKubernetesClusterAgentPoolProfiles(resp.ManagedClusterProperties.AgentPoolProfiles, *resp.Fqdn) | |||
There was a problem hiding this comment.
In the unlikely situation where resp.Fqdn is null this will crash. I suggest passing in the pointer and then checking it for null like all the other values in agent_pool_profile and dereferencing it there.
|
Other then one last minor change this LGTM, once that is done we can get this merged :) |
- Temporarily use the parent FQDN as the value for the FQDN on the agent pool profile. The latter is not populated by Azure and the field should eventually be removed
There was a problem hiding this comment.
hey @phekmat
Thanks for pushing the latest updates - I've taken a look through and this now LGTM 👍
I'll kick off the test suite now - but thanks for this!
|
Tests pass:
|
|
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! |
ghost
locked and limited conversation to collaborators
Azure for managed Kubernetes