Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
{ACS}
az aks create/update
: Add parameter--auto-upgrade-channel
to support auto upgrade #18825{ACS}
az aks create/update
: Add parameter--auto-upgrade-channel
to support auto upgrade #18825Changes from 1 commit
c662843
b30dd37
c8ed48e
c00c905
71456b1
ed4e903
da3ff65
1b21016
0a925aa
c7f92e3
4121a33
c1affbf
f6a3be7
ec9cd69
e5985c3
0ef331c
cd47cb0
8d8f80f
fb05ee7
effe159
33c2f04
0df7c06
8aa3f09
dbca938
432f0ee
49ef3ad
eb0e57f
90c3952
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If these enumerations are only used in
arg_type=get_enum_type()
and are not used elsewhere, I suggest that they do not need to be defined separately. You can just define it this way https://github.com/Azure/azure-cli/pull/18825/files#r687325877There 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.
Followed the style of defining the array at the top of the _params file, and using that in the get_enum_type() calls. Curious what you think.
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 mean, instead of defining these enumerations, we can write valid values directly to
arg_type=get_enum_type()
. Therefore, these enumerations can be deleted, such as #18825 (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.
Do we need to declare the default value here? Such as
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.
@xhl873, assuming the answer is no. There was no default before.
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 we use upgrade channel none as the default?
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.
So, nil is used for unspecified, and upgrade channel none is used if the customer wants to turn it off.
We could make upgrade channel none the default for create, since there is nothing it would be turning off, but for update adding upgrade channel none as the default would make any update call turn off autoupgrade for the customer unless they specified it on each call.
I talked with Xiahe about this as we agreed that just adding a default like this for create did not make sense.
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.
The updated model includes an enum value for unspecified
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 could see making unspecified the default. Although, rejecting nil in place of a new unspecified value at this point would still break the preview CLI, and current behavior of the REST API if customers were using it directly. (ok, because autoupgrader is not GA yet?)
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.
Also, using unspecified over nil adds another channel to track where nil could be used, but I get the argument around removing the reliance on nil values.
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.
Not sure it matters too much, we can convert the nil to unspecified in RP ( or keep using nil )
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.