First version of Azure NetApp Files CLI for GA #9387
Conversation
tjprescott
left a comment
tjprescott
left a comment
There was a problem hiding this comment.
Requires command review as a new module and due to declaring the commands are GA.
|
@tjprescott Comments from command review incorporated in _help.py and code. The account set command was able to be removed by incorporating the subgroup commands as suggested. |
|
@tjprescott As I have seen every time, all tests involving volume creation, specifically vnet, fail. These were newly run locally in live mode and verified with a playback with all passing (there is also a temporary test, a placeholder for a new test which I will clean up but I was keen to push the new changes for command and code review). |
|
@tjprescott I have tidied the subgroup test so there is now a test for each of the two new subgroups. From my side then I believe I have addressed all comments from the review to this point. Just for info, I will be on vacation from Wednesday next week for two weeks. It would be great to get an agreed code base before then but if not I will be passing this on to one of my team. |
| doc_string_source='azure.mgmt.netapp.models#NetAppAccountPatch', | ||
| exception_handler=netappfiles_exception_handler) | ||
|
|
||
| with self.command_group('netappfiles account active-directory', netappfiles_accounts_sdk) as g: |
There was a problem hiding this comment.
This should be netappfiles account ad to remain consistent with the existing ad command group for Active Directory.
|
|
||
|
|
||
| 1.0.0 | ||
| ++++++++++++++++++ |
There was a problem hiding this comment.
Should be 2.0.0. Shouldn't matter much longer, but still.
There was a problem hiding this comment.
No clear on why? Isn't this the first release of the new command set, so 1.0.0?
There was a problem hiding this comment.
No, all GA modules in the CLI are 2.0. Weird historical reasons.
| from azure.cli.core.commands import CliCommandType | ||
| netappfiles_custom = CliCommandType(operations_tmpl='azure.cli.command_modules.netappfiles.custom#{}') | ||
| super(NetAppFilesCommandsLoader, self).__init__(cli_ctx=cli_ctx, | ||
| min_profile='2017-03-10-profile', |
There was a problem hiding this comment.
This line will need to be removed. Please ping @limingu for what the appropriate convention will be with the updated CLI core.
There was a problem hiding this comment.
ok, I've requested info from him
| c.argument('account_name', help='The name of the ANF account', id_part=None) | ||
|
|
||
| with self.argument_context('netappfiles') as c: | ||
| c.argument('pool_name', options_list=['--pool-name', '-p'], required=True, help='The name of the ANF pool') |
There was a problem hiding this comment.
All of these things under a single context (i.e. 'netappfiles') should really be grouped together.
with self.argument_context('netappfiles') as c:
c.argument('resource_group',...)
c.argument('tags', …)
c.argument('account_name', …)
c.argument('pool_name', …)|
|
||
| def load_arguments(self, _): | ||
| with self.argument_context('netappfiles') as c: | ||
| c.argument('resource_group', options_list=['--resource-group', '-g'], required=True, help='The name of the resource group') |
There was a problem hiding this comment.
This should use the standard resource_group_name_type from the CLI core.
| c.argument('tags', arg_type=tags_type) | ||
|
|
||
| with self.argument_context('netappfiles') as c: | ||
| c.argument('account_name', options_list=['--account-name', '-a'], required=True, help='The name of the ANF account') |
There was a problem hiding this comment.
required=True/False should not be necessary anywhere in this file. It is inferred from the command signatures.
| c.argument('pool_name', id_part='child_name_1', options_list=['--pool-name', '--name', '-n', '-p'], required=True, help='The name of the ANF pool') | ||
|
|
||
| with self.argument_context('netappfiles pool list') as c: | ||
| c.argument('account_name', options_list=['--account-name', '--name', '-n', '-a'], help='The name of the ANF account', id_part=None) |
There was a problem hiding this comment.
There's a lot of duplication of metadata throughout. Not crucial, but you could simplify the maintenance of your module using CLIArgumentTypes. Here's a pattern used in Network.
account_name_type = CLIArgumentType(options_list=['--account-name', '-a'], id_part='name', help='Name of the ANF account.')
with self.argument_context('netappfiles') as c:
c.argument('account_name', account_name_type)
with self.argument_context('netappfiles account') as c:
c.argument('account_name', account_name_type, options_list=['--account-name', '-a', '-n', '--name'])
with self.argument_context('netappfiles pool list') as c:
c.argument('account_name', account_name_type, id_part=None)This will allow you to change the help text for account name in only one place instead of several, for example.
There was a problem hiding this comment.
ok, will update this
| c.argument('volume_name', options_list=['--volume-name', '-n', '-v'], required=True, help='The name of the ANF volume', id_part=None) | ||
|
|
||
| with self.argument_context('netappfiles') as c: | ||
| c.argument('tag', options_list=['--tags'], required=False, help='A list of space separated tags to apply to the account') |
There was a problem hiding this comment.
This should also use the standard tags_type and should likely be grouped together with other calls under this scope.
| exception_handler=netappfiles_exception_handler) | ||
|
|
||
| with self.command_group('netappfiles mount-target', netappfiles_mount_targets_sdk) as g: | ||
| g.command('list', 'list') |
There was a problem hiding this comment.
Unless you plan on having additional "mount-target" commands, this should be netappfiles list-mount-targets.
There was a problem hiding this comment.
ok, will update because I see it is little effort to change and it is more likely that this will be removed rather than new ones added
There was a problem hiding this comment.
@tjprescott Ok, didn't work quite as expected. Currently the command.py contains a command_group and a command but this change removes the command group. Not sure how this should change
_with self.command_group('netappfiles mount-target', netappfiles_mount_targets_sdk) as g:
g.command('list', 'list')_
| rg = '{rg}' | ||
|
|
||
| volume = self.create_volume(account_name, pool_name, volume_name, rg) | ||
| snapshot = self.cmd("az netappfiles snapshot create -g %s -a %s -p %s -v %s -s %s -l %s --file-system-id %s" % (rg, account_name, pool_name, volume_name, snapshot_name, LOCATION, volume['fileSystemId'])).get_output_in_json() |
There was a problem hiding this comment.
In the future, you might wish to refactor your tests to use the authoring patterns described in the docs:
https://github.com/Azure/azure-cli/blob/dev/doc/authoring_tests.md#sample-5-get-more-from-resourcegrouppreparer
The kwargs mechanism is more concise and makes your tests more readable. Not crucial for this PR, but a strong suggestion for future maintainability since new tests will likely borrow the pattern shown here.
There was a problem hiding this comment.
ok, I will not change this now but will add a comment in the test as a refactoring consideration for future changes.
|
@tjprescott made all the changes locally and will push once I get feedback from @limingu on the remaining item. Thanks. |
|
All changes made I think. |
|
I see failures here https://travis-ci.org/Azure/azure-cli/jobs/540766170. I'm not sure about the cause "FAIL: no_ids_for_list_commands". Trying to run locally. |
|
@leonardbf looks like you either need to re-record your tests or that you have randomness in your tests such that they won't play back. There is guidance for fixing "no --ids in list commands" here: It is at the very end of the section. |
Yeah I have fixed the ids issue. I hadn't added the id_part=None. As for the randomness, I am aware that some of the tests use the random name generation offered but I have tested this before to see if this was the problem. The mount_target test for instance does not have any but still fails. Also they do not fail in record or playback locally, only in the azure pipeline so I'm really not sure how to fix this. It is always the vnet command that fails it seems and has done so in every version of the CLI (extension) so far. |
|
As well as the possible issue with vnet API version (as emailed) I continue to get the naming failure in the help https://travis-ci.org/Azure/azure-cli/jobs/541009105. All have been removed except these 7 for account despite them working locally. Note that I changed the ordering for pool and volume but that did not work for account so there seems to be an additional consideration. I'm not certain what all the rules are that govern the correct order for this list. |
leonardbf
requested review from
adewaleo,
jaredmoo,
lewu-msft,
limingu,
marstr,
samkreter,
v-Ajnava and
yugangw-msft
|
Why is sql module affected by these changes? |
|
@jaredmoo sql should not be affected. Perhaps there are some changes due to my merge from azure dev into my fork to pull latest due to failing tests (network version). |
|
@tjprescott I've pulled from azure dev and my tests now pass. However there are other failures which seem to be outside of my changes. |
|
Hi, Lenny is on a vacation and I was asked to take over this. Is anything that needs to be done on our end? |
|
@tomasm9 you need to resolve the conflicts and get the CI to pass for us to be able to merge this. |
5 participants
Previously the Azure NetApp Files CLI was an extension only.
R4 standard of RP for GA
Replaces #9365 which was incorrectly applied to master.
This checklist is used to make sure that common guidelines for a pull request are followed.
The PR has modified HISTORY.rst describing any customer-facing, functional changes. Note that this does not include changes only to help content. (see Modifying change log).
I adhere to the Command Guidelines.