Skip to content

Conversation

@ShiqianTao
Copy link
Contributor

@ShiqianTao ShiqianTao commented Oct 17, 2022

[AKS] Add --disable-windows-outbound-nat for az aks nodepool add to add a Windows agent pool which the Windows OutboundNAT is disabled


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

Related command

az aks nodepool add --name npwin --os-type Windows --disable-windows-outbound-nat

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 pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

…indows agent pool which the Windows OutboundNAT is disabled
@ghost ghost requested review from wangzelin007 and zhoxing-ms October 17, 2022 06:01
@ghost ghost assigned zhoxing-ms Oct 17, 2022
@ghost ghost added this to the Oct 2022 (2022-11-01) milestone Oct 17, 2022
@ghost ghost added the Auto-Assign Auto assign by bot label Oct 17, 2022
@ghost ghost requested a review from yonzhan October 17, 2022 06:01
@ghost ghost added the AKS label Oct 17, 2022
@ShiqianTao
Copy link
Contributor Author

Hi @FumingZhang @AbelHu , could you help to review this PR? Thank you.

def validate_disable_windows_outbound_nat(namespace):
"""Validates disable_windows_outbound_nat can only be used on Windows."""
if namespace.disable_windows_outbound_nat:
if hasattr(namespace, 'os_type') and namespace.os_type != "Windows":
Copy link
Member

Choose a reason for hiding this comment

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

Should handle case-insensitive comparation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved. Thank you.

agentpool = self.set_up_gpu_properties(agentpool)
# set up custom ca trust
agentpool = self.set_up_custom_ca_trust(agentpool)
# set up custom ca trust
Copy link
Member

Choose a reason for hiding this comment

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

misleading comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Thank you.

@FumingZhang
Copy link
Member

FumingZhang commented Oct 17, 2022

Queued a pipeline to test the newly added case. Passed.

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

@ShiqianTao
Copy link
Contributor Author

Hi @zhoxing-ms , cloud you help to review and approve this PR? Thank you.

@yonzhan
Copy link
Collaborator

yonzhan commented Oct 17, 2022

@zhoxing-ms for awareness

@zhoxing-ms zhoxing-ms merged commit 9498f0e into Azure:main Oct 17, 2022
@azclibot
Copy link
Collaborator

azclibot commented Oct 17, 2022

[Release] Update index.json for extension [ aks-preview ] : https://dev.azure.com/azclitools/internal/_build/results?buildId=9171&view=results

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AKS Auto-Assign Auto assign by bot

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants