-
Notifications
You must be signed in to change notification settings - Fork 95
Moved admin 'ls' and 'set' commands under 'volume' #1055
Conversation
Rename 'vmdkops_admin ls" to ‘vmdkops_admin volume ls’ and rename ‘vmdkops_admin set’ to ‘ vmdkops_admin volume set’ This is a part of #1043 As I work through :multi-ESX tenancy code, some pieces ask for fix or implementation and make it easier for me to proceed. I do not want to bundle them all in a large PR, so I am submitting individual PRs which are useful on their own. This is one of them, Test: make test-esx locally. Manual validation. CI
@@ -119,24 +119,48 @@ def commands(): | |||
defined. For example, `tenant create` has a callback, but if a user runs the program | |||
like: `./vmdkops_admin.py tenant` they will get the following error: | |||
``` | |||
usage: vmdkops_admin.py tenant [-h] {rm,create,set,ls,get} ... | |||
usage: vmdkops_admin.py tenant [-h] {rm,create,volume,get} ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comments in L109-L122 still use "vmdkops_admin.py tenant...", which need to be replaced with "vmdkops_admin.py vm-group" since we have renamed "vmdkops_admin.py tenant" command to "vmdk_ops_admin.py vm-group".
In L122, the subcommand for "vmdkops_admin.py vm-group" are create, update, rm, ls , vm , access. So L122 should be change to something like this:
usage: vmdkops_admin.py vm-group [-h] {create, update, rm, ls, vm, access} ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lipingxue - cleaning up 'tenant->vm-group' terminology conversion is out of scope for this PR. The PR is targeting 'ls' and 'set' commands conversion to 'volume ls' and 'volume set'.
I just fixed a couple of locations I noticed with old "tenant" in messages. for common good. If you feel it should not be there I will be happy to rollback these, but I do not plan to put tenant-related cleanup here.
Besides , the lines you've referred to is the data structure explanation for arg parse support, it is not documentation for the actual usage.
Please do comment/suggest on the 'ls' and 'set' commands conversion to 'volume ls' and 'volume set'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@msterin can you do dummy push to restart CI CC/ @shuklanirdesh82 |
let me restart this specific run. (https://ci.vmware.run/vmware/docker-volume-vsphere/1731) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
tests passed but the last (concurrent) timed out. Since this PR impacts only admin CLI, merging. |
Rename 'vmdkops_admin ls" to
vmdkops_admin volume ls
and rename ‘vmdkops_admin set’ to
vmdkops_admin volume set
This is a part of #1043
As I work through multi-ESX tenancy code, some pieces ask for fix or implementation and make it easier for me to proceed. I do not want to bundle them all in a large PR, so I am submitting individual PRs which are useful on their own. This is one of them,
Test: make test-esx locally. Manual validation. CI (in progress)
//CC @pshahzeb @lipingxue