Skip to content

[AKS] aks create/update: add --vnet-subnet_id validationChenyzh vnet validator - #1243

Merged
Feiyue Yu (qwordy) merged 14 commits into
Azure:masterfrom
Czhang0727:chenyzh_vnetValidator
Mar 16, 2020
Merged

Feiyue Yu (qwordy) merged 14 commits into
Azure:masterfrom
Czhang0727:chenyzh_vnetValidator

Conversation

@Czhang0727

Copy link
Copy Markdown
Contributor

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:

@azuresdkci

Copy link
Copy Markdown

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/Czhang0727/azure-cli-extensions.git@chenyzh_vnetValidator#subdirectory=src/$EXT&egg=$EXT"

@Czhang0727

CHENYI ZHANG (Czhang0727) commented Feb 4, 2020

Copy link
Copy Markdown
Contributor Author

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/Czhang0727/azure-cli-extensions.git@chenyzh_vnetValidator#subdirectory=src/$EXT&egg=$EXT"

Tested in docker, expected result received.

bash-5.0# az aks create -g chenyzh --vnet-subnet-id dumy -n cztest --ssh-key-value .ssh/id_rsa.pub 
The behavior of this command has been altered by the following extension: aks-preview
--vnet-subnet-id is not a valid Azure resource ID.

bash-5.0# az aks create -g chenyzh --vnet-subnet-id /subscriptions/YourSubscriptionId/resourceGroups/YourResourceGroupName/providers/Microsoft.Network/virtualNetworks/Name -n cztest --ssh-key-value ~/.ssh/id_rsa.pub
The behavior of this command has been altered by the following extension: aks-preview
Resource group 'chenyzh' could not be found.

bash-5.0# az aks create -g chenyzh --vnet-subnet-id "" -n cztest --ssh-key-value ~/.ssh/id_rsa.pub
The behavior of this command has been altered by the following extension: aks-preview
Resource group 'chenyzh' could not be found.

@azuresdkci

Copy link
Copy Markdown

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/Czhang0727/azure-cli-extensions.git@chenyzh_vnetValidator#subdirectory=src/$EXT&egg=$EXT"

@Czhang0727

Copy link
Copy Markdown
Contributor Author

Did not realize "close and comment" will actually close entire PR while Azure DevOps just close the comment, re-open for continuing the review.

@Czhang0727

Copy link
Copy Markdown
Contributor Author

Re-try validation as one of check is expire

@azuresdkci

Copy link
Copy Markdown

If this PR is for a new extension or change to an existing extension, use the following to try out the changes in this PR:

docker run -it microsoft/azure-cli:latest
export EXT=<NAME>
pip install --upgrade --target ~/.azure/cliextensions/$EXT "git+https://github.com/Czhang0727/azure-cli-extensions.git@chenyzh_vnetValidator#subdirectory=src/$EXT&egg=$EXT"

@yonzhan

Copy link
Copy Markdown
Collaborator

add to S165.

1 similar comment
@yonzhan

Copy link
Copy Markdown
Collaborator

add to S165.

@mmyyrroonn

Copy link
Copy Markdown
Contributor

CHENYI ZHANG (@Czhang0727) Hello. I remembered that I merged a same PR. Is that in CLI main repo?

@mmyyrroonn Myron (mmyyrroonn) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Feiyue Yu (@qwordy) please leave your comments and sign off

@Czhang0727

Copy link
Copy Markdown
Contributor Author

CHENYI ZHANG (@Czhang0727) Hello. I remembered that I merged the same PR. Is that in CLI main repo?

myronfanqiu Hi, I already checked in the same update in azure-cli as recommended. The update is already checked in.
Azure/azure-cli#12038

I added a little bouns as I saw there is a styling warning with an un-necessary return.

@Czhang0727

Copy link
Copy Markdown
Contributor Author

Feiyue Yu (@qwordy), any chance we can merge the change?

c.argument('pod_cidr')
c.argument('service_cidr')
c.argument('vnet_subnet_id')
c.argument('vnet_subnet_id', type=str, validator=validate_vnet_subnet_id)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it has a help message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes it does, it keeps same as it was to be in _help.py

        - name: --vnet-subnet-id
          type: string
          short-summary: The ID of a subnet in an existing VNet into which to deploy the cluster.

@Czhang0727

Copy link
Copy Markdown
Contributor Author

merge conflict for check-in

@Czhang0727

Copy link
Copy Markdown
Contributor Author

Feiyue Yu (@qwordy) Hi, would it possible to have the file merged and checked in?
I found this PR is stuck here for too long, and new PRs are keep conflicts the fixing there.

@qwordy

Copy link
Copy Markdown
Member

We are very sorry to leave this PR unmerged for a long time. Can you resolve the conflicts again?

@Czhang0727

Copy link
Copy Markdown
Contributor Author

Feiyue Yu (@qwordy) Resolved conflict and verified through the pipeline.
Let me know if there is still any blocker for check in

@qwordy
Feiyue Yu (qwordy) merged commit 4f6e630 into Azure:master Mar 16, 2020
Emmanuel Auffray (ManuInNZ) pushed a commit to ManuInNZ/azure-cli-extensions that referenced this pull request Apr 11, 2020
…validator (Azure#1243)

* Adding VNetSubnet Validator, raise Error if we got an invalid resource id

* addining valid test case

* adding null / empty value test

* Format fixing

* fixing style issues

* remove white space at tail

* Update _params.py

remove tailing space

* Update _params.py

add line for lint

Co-authored-by: Chenyi Zhang <Chenyi.Zhang@microsoft.com>
Co-authored-by: Chenyi Zhang <chenyzh@microsoft.com>
Co-authored-by: Feiyue Yu <fey@microsoft.com>
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.

6 participants