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.

Sign up for GitHub

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

Jump to bottom

[AKS] Add option Windows2019, Windows2022 to --os-sku for az aks nodepool add #4549

Merged
merged 4 commits into from
May 12, 2022
Merged

[AKS] Add option Windows2019, Windows2022 to --os-sku for az aks nodepool add #4549

merged 4 commits into from
May 12, 2022

Conversation

ShiqianTao
Copy link
Contributor

@ShiqianTao ShiqianTao commented Mar 18, 2022 •
edited
Loading

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

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your PR is merged into master branch, a new PR will be created to update src/index.json automatically.
The precondition is to put your code inside this repo and upgrade the version in the PR but do not modify src/index.json.

@ghost ghost requested review from zhoxing-ms and wangzelin007 March 18, 2022 11:48
@ghost ghost assigned zhoxing-ms Mar 18, 2022
@ghost ghost added this to the Mar 2022 (2022-04-05) milestone Mar 18, 2022
@ghost ghost added the Auto-Assign Auto assign by bot label Mar 18, 2022
@ghost ghost requested a review from yonzhan March 18, 2022 11:48
@ghost ghost added the AKS label Mar 18, 2022
@yonzhan
Copy link
Collaborator

yonzhan commented Mar 18, 2022

aks

@ShiqianTao ShiqianTao marked this pull request as draft March 21, 2022 04:14
@wangzelin007 wangzelin007 changed the title [aks]os-sku supports Windows2019 and Windows2022 [AKS] parameter --os-sku supports Windows2019 and Windows2022 Mar 24, 2022
@wangzelin007 wangzelin007 changed the title [AKS] parameter --os-sku supports Windows2019 and Windows2022 [AKS] Add option Windows2019, Windows2022 to --os-sku for az aks nodepool add Mar 24, 2022
@PixelRobots
Copy link

Any update on this?

@ShiqianTao
Copy link
Contributor Author

Hi @PixelRobots.
It's pending REST API changing: Azure/azure-rest-api-specs#18706
After it, I will reactivate this Pull Request.

@PixelRobots
Copy link

Hi @PixelRobots. It's pending REST API changing: Azure/azure-rest-api-specs#18706 After it, I will reactivate this Pull Request.

Looks like it has been merged :) excited to try this feature.

@ShiqianTao
Copy link
Contributor Author

Hi @PixelRobots. It's pending REST API changing: Azure/azure-rest-api-specs#18706 After it, I will reactivate this Pull Request.

Looks like it has been merged :) excited to try this feature.

Hi @PixelRobots , We are also pending SDK updating, and the timeline is 2022/05/07. I will reactive this PR in 2022/05/07. Thank you.

@ShiqianTao ShiqianTao force-pushed the shtao/windows-os-sku branch 7 times, most recently from cb368bb to 52cf450 Compare May 11, 2022 14:18
@ShiqianTao ShiqianTao marked this pull request as ready for review May 11, 2022 14:50
@ShiqianTao ShiqianTao requested a review from FumingZhang as a code owner May 11, 2022 14:50
@ShiqianTao
Copy link
Contributor Author

@AbelHu @FumingZhang Could you help to review it? Thank you.

AbelHu
AbelHu reviewed May 12, 2022
View reviewed changes
src/aks-preview/azext_aks_preview/_help.py Outdated
@@ -1040,7 +1040,7 @@
short-summary: The OS Type. Linux or Windows.
- name: --os-sku
type: string
short-summary: The os-sku of the agent node pool. Ubuntu or CBLMariner.
short-summary: The os-sku of the agent node pool. Ubuntu or CBLMariner when os-type is Linux, default is Ubuntu if not set; Windows2019 or Windows2022 when os-type is Windows, default is Windows2019 if not set.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ShiqianTao Does cli code set the default value for Windows? If it is set by rp, we may change the default value in future so we need to mention it in the description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, AKS-RP controls the default value. Updated the description.

FumingZhang
FumingZhang reviewed May 12, 2022
View reviewed changes
src/aks-preview/azcli_aks_live_test/configs/cli_matrix_default.json Outdated
@@ -14,7 +14,8 @@
"test_aks_create_with_monitoring_aad_auth_msi",
"test_aks_create_with_monitoring_aad_auth_uai",
"test_aks_enable_monitoring_with_aad_auth_msi",
"test_aks_enable_monitoring_with_aad_auth_uai"
"test_aks_enable_monitoring_with_aad_auth_uai",
"test_aks_nodepool_add_with_ossku_windows2022"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this feature won't be added to azure-cli until GA, and the feature would also be removed when it's ready. So, I guess no need for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, removed it.

src/aks-preview/azext_aks_preview/_completers.py Outdated
def get_nodepool_ossku_completion_list(cmd, prefix, namespace, **kwargs): # pylint: disable=unused-argument
"""Return the list of allowed os-sku values when add nodepool"""

return ["Ubuntu", "CBLMariner", "Windows2019", "Windows2022"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if you could replace these strings with the consts. Also the ones in get_cluster_ossku_completion_list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good guidance. Updated.

src/aks-preview/HISTORY.rst
@@ -13,7 +13,11 @@ Pending
+++++++
* Update to use 2022-04-02-preview api version.

0.5.67 (NOT RELEASED)
0.5.68
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Li ma added a feature about api and released 0.5.57, need to resolve the conflicts here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Updated.

@ShiqianTao ShiqianTao force-pushed the shtao/windows-os-sku branch from 7f0ffd9 to 05ac536 Compare May 12, 2022 02:59
FumingZhang
FumingZhang approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@FumingZhang FumingZhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM:shipit:

@FumingZhang
Copy link
Member

Wait for previous release PR #4805.

@ShiqianTao
Copy link
Contributor Author

@zhoxing-ms Could you help to review and merge this PR? Thank you.

@zhoxing-ms zhoxing-ms merged commit de8d2e1 into Azure:main May 12, 2022
@ShiqianTao ShiqianTao mentioned this pull request Aug 30, 2022
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FumingZhang FumingZhang FumingZhang approved these changes

@wangzelin007 wangzelin007 wangzelin007 approved these changes

@zhoxing-ms zhoxing-ms zhoxing-ms approved these changes

@AbelHu AbelHu AbelHu approved these changes

@andyliuliming andyliuliming Awaiting requested review from andyliuliming andyliuliming is a code owner

@yonzhan yonzhan Awaiting requested review from yonzhan

Assignees

@zhoxing-ms zhoxing-ms

Labels
AKS Auto-Assign Auto assign by bot
Projects
None yet
Milestone
Mar 2022 (2022-04-06)
Development

Successfully merging this pull request may close these issues.
7 participants
@ShiqianTao @yonzhan @PixelRobots @FumingZhang @AbelHu @zhoxing-ms @wangzelin007