Output cluster's fqdn#251
Conversation
|
MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
zioproto
left a comment
zioproto
left a comment
There was a problem hiding this comment.
Please check if the test passes when cluster is not a private cluster.
|
|
||
| output "cluster_private_fqdn" { | ||
| description = "The FQDN for the Kubernetes Cluster when private link has been enabled, which is only resolvable inside the Virtual Network used by the Kubernetes Cluster." | ||
| value = azurerm_kubernetes_cluster.main.private_fqdn |
There was a problem hiding this comment.
This value in the terraform state is "" when not using private clusters:
"private_fqdn": "",
There was a problem hiding this comment.
Hi @zioproto sorry for the late reply, I've tested and confirmed that this private_fqdn will be empty string when we set aks's private_cluster_enabled to false.
There was a problem hiding this comment.
OK. Do you think is necessary to add a comment to the function assertOutputNotEmpty to clarify that this function can be used only when testing with private clusters ?
Let me know if it makes sense to add this comment in the code.
If in the future there will be a scenario with public clusters we want to make sure that assertOutputNotEmpty is not used because it will always fail. Thank you !
There was a problem hiding this comment.
I think it's ok since output's description has described this output only works for private cluster, if we change this example to provision a public cluster in the future and somehow we forget to remove this assertOutputNotEmpty from our e2e test, the failed test will warn us and the we can figure out what's wrong quickly by reading description.
| o, ok := output[name].(string) | ||
| assert.True(t, ok) | ||
| assert.NotEqual(t, "", o) | ||
| } |
There was a problem hiding this comment.
Is this testing passing when private cluster is not enabled and the value in the tf state is:
"private_fqdn": "",
??
There was a problem hiding this comment.
@lonegunmanb did you have an opportunity to look at this comment ? thanks :)
|
MAIN BRANCH PUSH DETECTED DUE TO #253, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
MAIN BRANCH PUSH DETECTED DUE TO #249, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
@lonegunmanb do you want to rebase this one on the current master ? |
|
MAIN BRANCH PUSH DETECTED DUE TO #245, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
1 similar comment
|
MAIN BRANCH PUSH DETECTED DUE TO #, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
MAIN BRANCH PUSH DETECTED DUE TO #259, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
MAIN BRANCH PUSH DETECTED DUE TO #262, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
MAIN BRANCH PUSH DETECTED DUE TO #260, THIS PR NEED TO BE UPDATED TO TRIGGER CI. |
|
LGTM |
2 participants
Add new outputs:
cluster_fqdn,cluster_portal_fqdnandcluster_private_fqdnFixes #250