Admincli improvement #993
Admincli improvement #993
Conversation
1. Printing info message for default tenant command instead of showing an empty list 2. Removing usage of --name option for tenant rm command Testing: Updated testcases
esx_service/cli/vmdkops_admin.py
Outdated
|
|
||
| # Handling _DEFAULT tenant case separately to print info message | ||
| # instead of printing empty list | ||
| if (args.name == "_DEFAULT"): |
There was a problem hiding this comment.
Can we use a constant instead of hard-coded string? I'd suggest to move all the constants below from auth.py to auth_data_const.py so that they can be shared by other modules:
DEFAULT_TENANT = '_DEFAULT'
DEFAULT_TENANT_UUID = '11111111-1111-1111-1111-111111111111'
DEFAULT_DS = '_DEFAULT'
DEFAULT_DS_URL = DEFAULT_DS + "_URL"
esx_service/cli/vmdkops_admin.py
Outdated
| # Handling _DEFAULT tenant case separately to print info message | ||
| # instead of printing empty list | ||
| if (args.name == "_DEFAULT"): | ||
| print("All VMs that do not belong to any tenant belong to _DEFAULT tenant") |
There was a problem hiding this comment.
the message is confusing IMO. Should be something "'__DEFAULT' tenant contains all VMs which were not added to other tenants"
esx_service/cli/vmdkops_admin.py
Outdated
|
|
||
| # Handling _DEFAULT tenant case separately to print info message | ||
| # instead of printing empty list | ||
| if (args.name == "_DEFAULT"): |
esx_service/cli/vmdkops_admin.py
Outdated
| @@ -236,9 +236,8 @@ def commands(): | |||
| 'func': tenant_rm, | |||
| 'help': 'Delete a tenant', | |||
| 'args': { | |||
| '--name': { | |||
| 'name': { | |||
There was a problem hiding this comment.
why is it changed for 'tenant rm' ? I see 'tenant create' and 'tenant update' are stil using --name and 'tenant rm' now uses positional 'name'. This is inconsistent.
Please drop this change, and instead use #922 to enumerate all changes to --name or name whihc are needed to make all consistent. See #922 for more comments
There was a problem hiding this comment.
Let's discuss this in #922.
There was a problem hiding this comment.
So the decision is we only use positional name for "rm" type of command? @shaominchen
There was a problem hiding this comment.
@lipingxue We all agree that local consistency is more important. For AdminCLI commands, the only inconsistent command is "policy rm". So we should just fix that command to use "--name" instead of fixing all other commands to use positional name.
1. Info message for default tenant vm ls command. 2. Moving default tenant and privilege constants to auth_data_const.py Related changes for using these constants 3. Making use of --name option for policy rm and reverting the tenant rm to also make use of --name option. Testing: Updated test cases
| @@ -159,8 +159,9 @@ def commands(): | |||
| 'func': policy_rm, | |||
| 'help': 'Remove a storage policy', | |||
| 'args': { | |||
| 'name': { | |||
| 'help': 'Policy name' | |||
| '--name': { | |||
There was a problem hiding this comment.
sorry I am late. Can you make sure admin_cli information is in sync with the proposed changes?
e.g. https://github.com/vmware/docker-volume-vsphere/blob/master/docs/user-guide/admin-cli.md#remove-3
Info message for default tenant vm ls command
Moving default tenant and privilege constants to auth_data_const.py
Related changes for using these constants
Making use of --name option for policy rm
Testing:
Updated testcases
Resolving: #965
Fixes #922