-
Notifications
You must be signed in to change notification settings - Fork 476
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
Refactor output for removed addon_profile
block. Deprecate legacy dot index syntax in outputs.
#188
Conversation
addon_profile
block.addon_profile
block. Deprecate legacy dot index syntax in outputs.
outputs.tf
Outdated
} | ||
|
||
output "admin_password" { | ||
value = length(azurerm_kubernetes_cluster.main.kube_admin_config) > 0 ? azurerm_kubernetes_cluster.main.kube_admin_config.0.password : "" | ||
value = try(azurerm_kubernetes_cluster.main.kube_admin_config[0].password, "") | ||
} | ||
|
||
output "addon_profile" { |
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.
I understand you want to keep the addon_profile
output name to avoid breaking changes.
However in #157 you bumped the azure provider from version 2.x to 3.x so breaking changes are expected, and folks should read upgrade notes.
Keeping the addon_profile
forever will be super confusing for folks starting using this module directly with the version 3.x of the provider, because they will have to understand where the legacy of this naming comes from.
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.
I tested creating in my project the following output.tf
file:
output aci_connector_linux {
value = module.aks.addon_profile.aci_connector_linux
}
output aci_connector_linux_enabled {
value = module.aks.addon_profile.aci_connector_linux_enabled
}
output azure_policy_enabled {
value = module.aks.addon_profile.azure_policy_enabled
}
output http_application_routing_enabled {
value = module.aks.addon_profile.http_application_routing_enabled
}
output ingress_application_gateway {
value = module.aks.addon_profile.ingress_application_gateway
}
output ingress_application_gateway_enabled {
value = module.aks.addon_profile.ingress_application_gateway_enabled
}
output key_vault_secrets_provider {
value = module.aks.addon_profile.key_vault_secrets_provider
}
output key_vault_secrets_provider_enabled {
value = module.aks.addon_profile.key_vault_secrets_provider_enabled
}
output oms_agent {
value = module.aks.addon_profile.oms_agent
}
output oms_agent_enabled {
value = module.aks.addon_profile.oms_agent_enabled
}
output open_service_mesh_enabled {
value = module.aks.addon_profile.open_service_mesh_enabled
}
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.
I would drop the addon_profile
string in the outputs and add information about this change in the Changelog file
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.
yes, addon_profile
makes no sense with the updated provider. I added the export in order to get hold of the identity of the AGIC, in order to be able to do role assignments:
principal_id = module.aks.addon_profile.ingress_application_gateway[0].ingress_application_gateway_identity[0].object_id
scope = module.aks.addon_profile.ingress_application_gateway[0].gateway_id
as long as we still get hold of those values anything is fine.
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.
Agreed!
LGTM |
This patch will fix both #180 and #182.
As
addon_profile
was added by @davidkarlsen, would you please give this pr a review to see whether the new output object can work with your code? Thanks!