Create initial CLI Extension for ANF resource provider #527

leonardbf · 2019-02-19T12:35:42Z

From NetApp internal ticket NFSAAS-1708 create initial CLI Extension for ANF resource provider

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

General Guidelines

Have you run ./scripts/ci/test_static.sh locally? (pip install pylint flake8 required)
Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

My extension description/summary conforms to the Extension Summary Guidelines.

NFSAAS-1708 add initial version of ANF CLI extension

azuresdkci · 2019-02-19T12:35:54Z

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/leonardbf/azure-cli-extensions.git@master#subdirectory=src/$EXT&egg=$EXT"

NFSAAS-1708 lint fixes

NFSAAS-1708 code owner

tjprescott · 2019-02-19T16:00:42Z

Are you working with anyone in the CLI team on this extension?

williexu · 2019-02-19T18:21:02Z

@tjprescott I've been working with the team :).

williexu · 2019-02-19T18:41:19Z

.github/CODEOWNERS

+/src/sqlvm-preview/ @yareyes
+
+/src/anf-preview/ @williexu


Can you change this to reference whoever on your team will be the prime codeowner/reviewer?

williexu · 2019-02-19T18:43:02Z

src/anf-preview/README.rst

+===================================================================
+
+This package is for the Azure NetApp File (ANF)' module.
+i.e. 'az anf'


Might want to add a little more info- i.e. how to install the extension, basic usage, link to docs for the service, etc..

ok. Install and some examples added. We have no docs to link to yet.

williexu · 2019-02-19T18:45:05Z

src/anf-preview/azext_anf_preview/_params.py

+
+def load_arguments(self, _):
+    with self.argument_context('anf') as c:
+        c.argument('resource_group', options_list=['--resource-group', '-g'], required=True, help='The name of the resource group')


You dont need this. The CLI has a default argument context for resource_group

Yes I'd seen this and fully expected to be able to use either --resource-group or -g. However what I found was that without these lines in the _param file that specifying the command with -g resulted in 'argument --resource-group is required'.

I see the problem, some of your resource_group parameter names are resource_group, while some are resource_group_name. They should all be resource_group_name to take advantage of the CLI's context. Please change the names to all match.

Yes, that does indeed make it available for the custom commands. However list, show and delete still require the full --resource-group and cannot use -g without this in the _params.

Interesting, I believe for the most part, the swagger def name for resource group is resourceGroupName, as indicated: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/common-types/resource-management/v1/types.json#L291
A team member of mine has raised the following PR to address this: Azure/azure-rest-api-specs#5256

If changing the swagger and generating new sdks is impossible for whatever reason, you will also be able to use: https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/commands/parameters.py#L236-L241

williexu · 2019-02-19T18:48:27Z

src/anf-preview/azext_anf_preview/_params.py

+        c.argument('volume_name', options_list=['--volume-name', '-v'], required=True, help='The name of the ANF volume')
+
+    with self.argument_context('anf') as c:
+        c.argument('snapshot_name', options_list=['--snapshot-name', '-s'], required=True, help='The name of the ANF snapshot')


By convention, the CLI also has the shorthand option -n for the target resource of the command. It would be nice if these parameters did the same. For example, for az anf account commands, --account-name should also be referred to by -n.

ok. I will add the -n with a resource specific argument_context.

williexu · 2019-02-19T18:49:23Z

src/anf-preview/azext_anf_preview/_params.py

+        c.argument('account_name', options_list=['--account-name', '-a'], required=True, help='The name of the ANF account')
+
+    with self.argument_context('anf') as c:
+        c.argument('pool_name', options_list=['--pool-name', '-p'], required=True, help='The name of the ANF pool')


Which of these resources can be referred to by a ARM id?

Is this required or nice to have?

Added --ids for each resource in focus

williexu · 2019-02-19T18:52:23Z

src/anf-preview/setup.py

+from codecs import open
+from setuptools import setup, find_packages
+
+VERSION = "0.0.1"


If this is still in development, and the goal is to hold off on releasing, this is fine. If the goal is to release for preview soon, this will need to be bumped to 0.1.0.

williexu · 2019-02-19T18:56:59Z

src/anf-preview/setup.py

+
+DEPENDENCIES = [
+    'azure-mgmt-netapp==0.1.0',
+    'azure-cli-core'


Remove these dependencies.
azure-cli-core will be taken from the CLI's installation dependency.
For azure-mgmt-netapp, please vendor the sdk in the extension under a vendored_sdks folder.

ok, will remove and add sdk

williexu · 2019-02-19T19:27:36Z

Please also add metadata to the extension: https://github.com/Azure/azure-cli/blob/dev/doc/extensions/metadata.md

NFSAAS-1708 update from review comments

NFSAAS-1708 update with code review comments

NFSAAS-1708 updates from review comments

leonardbf · 2019-02-20T17:32:23Z

Ready for re-review.

williexu

One more change, otherwise looks good.

williexu · 2019-02-22T14:16:28Z

src/anf-preview/azext_anf_preview/_params.py

+
+def load_arguments(self, _):
+    with self.argument_context('anf') as c:
+        c.argument('resource_group', options_list=['--resource-group', '-g'], required=True, help='The name of the resource group')


I see the problem, some of your resource_group parameter names are resource_group, while some are resource_group_name. They should all be resource_group_name to take advantage of the CLI's context. Please change the names to all match.

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

williexu

Interesting, I believe for the most part, the swagger def name for resource group is resourceGroupName, as indicated: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/common-types/resource-management/v1/types.json#L291
A team member of mine has raised the following PR to address this: Azure/azure-rest-api-specs#5256

If changing the swagger and generating new sdks is impossible for whatever reason, you will also be able to use: https://github.com/Azure/azure-cli/blob/dev/src/azure-cli-core/azure/cli/core/commands/parameters.py#L236-L241

@leonardbf one last request before merging this PR, could you use the argument type above?

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

leonardbf · 2019-02-28T12:28:53Z

Ok. Argument type included @williexu

leonardbf and others added 2 commits February 19, 2019 12:29

NFSAAS-1708 add initial version of anf cli extension

b64c7c2

Merge pull request #1 from leonardbf/NFSAAS-1708-initial-cli-extension

e1c85e3

NFSAAS-1708 add initial version of ANF CLI extension

leonardbf and others added 4 commits February 19, 2019 13:37

NFSAAS-1708 lint fixes

51733d1

Merge pull request #2 from leonardbf/NFSAAS-1708-initial-cli-extension

faa9bd8

NFSAAS-1708 lint fixes

NFSAAS-1708 code owner

f1781de

Merge pull request #3 from leonardbf/NFSAAS-1708-initial-cli-extension

6e07811

NFSAAS-1708 code owner

williexu suggested changes Feb 19, 2019

View reviewed changes

leonardbf and others added 10 commits February 20, 2019 15:30

NFSAAS-1708 update from review comments

5d7a03a

Merge pull request #4 from leonardbf/NFSAAS-1708-initial-cli-extension

2ea3977

NFSAAS-1708 update from review comments

NFSAAS-1708 update from review comments

46deb54

Merge pull request #5 from leonardbf/NFSAAS-1708-initial-cli-extension

6b6cac0

NFSAAS-1708 update from review comments

NFSAAS-1708 update with code review comments

b3bb6e1

Merge pull request #6 from leonardbf/NFSAAS-1708-initial-cli-extension

5c84c92

NFSAAS-1708 update with code review comments

NFSAAS-1708 updates from review comments

a7becd6

Merge pull request #7 from leonardbf/NFSAAS-1708-initial-cli-extension

3f11bc3

NFSAAS-1708 updates from review comments

NFSAAS-1708 updates from review comments

a8e625a

Merge pull request #8 from leonardbf/NFSAAS-1708-initial-cli-extension

d4d3dd3

NFSAAS-1708 updates from review comments

williexu suggested changes Feb 22, 2019

View reviewed changes

leonardbf added 6 commits February 25, 2019 12:35

NFSAAS-1708 updates from review comments (#9)

9ec6de4

Nfsaas 1708 initial cli extension (#10)

898a90d

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

Nfsaas 1708 initial cli extension (#11)

a1722a1

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

Nfsaas 1708 initial cli extension (#13)

c92c760

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

Nfsaas 1708 initial cli extension (#14)

e9d77d1

* NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments * NFSAAS-1708 updates from review comments

williexu suggested changes Feb 26, 2019

View reviewed changes

Merge branch 'master' into master

e1e41ee

williexu approved these changes Feb 28, 2019

View reviewed changes

williexu merged commit 5a82cfc into Azure:master Feb 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create initial CLI Extension for ANF resource provider #527

Create initial CLI Extension for ANF resource provider #527

leonardbf commented Feb 19, 2019

azuresdkci commented Feb 19, 2019

tjprescott commented Feb 19, 2019

williexu commented Feb 19, 2019

williexu Feb 19, 2019

williexu Feb 19, 2019

leonardbf Feb 20, 2019

williexu Feb 19, 2019

leonardbf Feb 19, 2019

williexu Feb 22, 2019

leonardbf Feb 23, 2019

williexu Feb 23, 2019

williexu Feb 19, 2019

leonardbf Feb 19, 2019

williexu Feb 19, 2019

leonardbf Feb 19, 2019

leonardbf Feb 20, 2019

williexu Feb 19, 2019

williexu Feb 19, 2019

leonardbf Feb 19, 2019

williexu commented Feb 19, 2019

leonardbf commented Feb 20, 2019

williexu left a comment •

edited

Loading

williexu Feb 22, 2019

williexu left a comment •

edited

Loading

leonardbf commented Feb 28, 2019

		/src/sqlvm-preview/ @yareyes

		/src/anf-preview/ @williexu

Create initial CLI Extension for ANF resource provider #527

Create initial CLI Extension for ANF resource provider #527

Conversation

leonardbf commented Feb 19, 2019

From NetApp internal ticket NFSAAS-1708 create initial CLI Extension for ANF resource provider

General Guidelines

azuresdkci commented Feb 19, 2019

tjprescott commented Feb 19, 2019

williexu commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williexu commented Feb 19, 2019

leonardbf commented Feb 20, 2019

williexu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williexu left a comment • edited Loading

Choose a reason for hiding this comment

leonardbf commented Feb 28, 2019

williexu left a comment •

edited

Loading

williexu left a comment •

edited

Loading