-
Notifications
You must be signed in to change notification settings - Fork 3.3k
[AKS] Honor addon names defined in Azure CLI #15376
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
Conversation
yungezz
left a comment
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.
could you pls add test to cover this?
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.
for != part, better to lower() before compare
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.
Here my intention is to change the keys back to 'logAnalyticsWorkspaceResourceID'. The intention of key != 'logAnalyticsWorkspaceResourceID' is to skip if the key is already be 'logAnalyticsWorkspaceResourceID'.
For example, if the key is 'loganalyticsworkspaceresourceid' or 'LogAnalyticsWorkspaceResourceID', the value will be pop out and write back with key 'logAnalyticsWorkspaceResourceID'. But if the key is already 'logAnalyticsWorkspaceResourceID', it skips and nothing changed.
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.
can you assign 'logAnalyticsWorkspaceResourceID' to a variable and use the variable instead of using the hard-coded string in multiple places?
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.
@fengzhou-msft We're already doing that pattern in azure-cli-extension. I'll change that in another pr.
|
hi @kaiqizhang would you pls address comments? today is Code complete date, would you like to catch this release? |
Oh, sorry for late. Will do it soon. |
|
hi @kaiqizhang just want to double confirm, this change will not cause existing users' breaking change right? little concern because it changed existing output |
It's a long story. 2 months ago, we did a breaking change on the server side. All the keys are converted to lower cases. Based on the IcM at that time and discussion in the team, we reverted the change. Some customer cares about the case because they're using json query in their script. With this change in Azure CLI, customers can change the case back if it's necessary for them. (customer can still use different case in Terraform, Postman, etc.) |
Description
AKS server treats addon names and addon config keys as case insensitive, and store them in DB as it is in input. For example, we have both
logAnalyticsWorkspaceResourceIDandloganalyticsworkspaceresourceidin our DB and both are valid. However, in the public document we're only giving examples oflogAnalyticsWorkspaceResourceID.This change makes Azure CLI always convert the keys to the name defined in Azure CLI. (The name defined in Azure CLI is the same as public document), so that customer get the right name back.
Testing Guide
az aks enable-addons -a kube-dashboard -g testrg -n testclusterHistory Notes
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.