Skip to content
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

Merged
merged 28 commits into from
Aug 18, 2021

Conversation

charliedmcb
Copy link
Contributor

Description

Testing Guide

History Notes

[Component Name 1] BREAKING CHANGE: az command a: Make some customer-facing breaking change.
[Component Name 2] az command b: Add some customer-facing feature.


This checklist is used to make sure that common guidelines for a pull request are followed.

@charliedmcb charliedmcb changed the title Adding Autoupgrader to CLI [WIP] Adding Autoupgrader to CLI Jul 13, 2021
@yonzhan yonzhan requested a review from jiasli July 13, 2021 01:35
@yonzhan yonzhan added this to the Jul 2021 (2021-08-03) milestone Jul 13, 2021
@yonzhan
Copy link
Collaborator

yonzhan commented Jul 13, 2021

acs

@yonzhan yonzhan requested a review from kairu-ms July 13, 2021 01:37
@charliedmcb charliedmcb changed the title [WIP] Adding Autoupgrader to CLI Adding Autoupgrader to CLI Jul 15, 2021
@zhoxing-ms zhoxing-ms changed the title Adding Autoupgrader to CLI [ACS] Support auto upgrader to CLI Jul 26, 2021
@zhoxing-ms zhoxing-ms changed the title [ACS] Support auto upgrader to CLI [ACS] Add auto upgrader to CLI Jul 26, 2021
@@ -202,6 +203,7 @@ def load_arguments(self, _):
validator=validate_load_balancer_idle_timeout)
c.argument('outbound_type', arg_type=get_enum_type([CONST_OUTBOUND_TYPE_LOAD_BALANCER,
CONST_OUTBOUND_TYPE_USER_DEFINED_ROUTING]))
c.argument('auto_upgrade_channel', arg_type=get_enum_type([CONST_RAPID_UPGRADE_CHANNEL, CONST_STABLE_UPGRADE_CHANNEL, CONST_PATCH_UPGRADE_CHANNEL, CONST_NODE_IMAGE_UPGRADE_CHANNEL, CONST_NONE_UPGRADE_CHANNEL]))
Copy link
Contributor

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

arg_type=get_enum_type(...),..., default='xxx'

Copy link
Contributor Author

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.

Copy link

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?

Copy link
Contributor Author

@charliedmcb charliedmcb Aug 20, 2021

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.

Copy link

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

Copy link
Contributor Author

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?)

Copy link
Contributor Author

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.

Copy link

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 )

@zhoxing-ms zhoxing-ms changed the title [ACS] Add auto upgrader to CLI [ACS] az aks create/update: Add parameter --auto-upgrade-channel to support auto upgrade Jul 26, 2021
@zhoxing-ms
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jsntcy jsntcy closed this Jul 26, 2021
@jsntcy jsntcy reopened this Jul 26, 2021
@zhoxing-ms
Copy link
Contributor

@charliedmcb Could you please resolve the CLI style issue?

@zhoxing-ms
Copy link
Contributor

Please solve these comments and CI issues in time today, otherwise this PR will miss the release of this sprint and can only be released in the next sprint (09-07)

Charlie McBride added 15 commits August 15, 2021 03:12
…b/addingAutoupgraderToCLI""""

This reverts commit 8d8f80f.
@charliedmcb
Copy link
Contributor Author

Plan to have this ready for review tomorrow

@zhoxing-ms zhoxing-ms merged commit 6918b29 into Azure:dev Aug 18, 2021
@andyliuliming
Copy link
Member

@zhoxing-ms after this commit, all the "update" scenario will be broken for our AKS.

FumingZhang added a commit to FumingZhang/azure-cli that referenced this pull request Aug 20, 2021
…hannel` to support auto upgrade (Azure#18825)"

This reverts commit 6918b29.
zhoxing-ms pushed a commit that referenced this pull request Aug 20, 2021
…atest RP (#19293)

* Revert "update new recording file (#19258)"

This reverts commit 71635be.

* Revert "[ACS] `az aks create/update`: Add parameter `--auto-upgrade-channel` to support auto upgrade (#18825)"

This reverts commit 6918b29.
@zhoxing-ms zhoxing-ms changed the title [ACS] az aks create/update: Add parameter --auto-upgrade-channel to support auto upgrade {ACS} az aks create/update: Add parameter --auto-upgrade-channel to support auto upgrade Aug 20, 2021
charliedmcb pushed a commit to charliedmcb/azure-cli that referenced this pull request Aug 20, 2021
… crash with latest RP (Azure#19293)"

This reverts commit 1660877.
andyliuliming pushed a commit that referenced this pull request Aug 27, 2021
…grade-channel to support auto upgrade (with fix) (#19297)

redo auto upgrade channel change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants