Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

The command behavior of "tenant rm" and "policy rm" are inconsistent #922

Closed
shaominchen opened this issue Feb 17, 2017 · 9 comments
Closed

Comments

@shaominchen
Copy link
Contributor

shaominchen commented Feb 17, 2017

When we run "vmdkops_admin policy rm MyPolicy", we don't need to specify the parameter name "--name". However, to run "vmdkops_admin tenant rm --name MyTenant", we must specify "--name".

I think we should keep the command behavior consistent. My suggestion is to fix the "tenant rm" command, because the behavior of "policy rm" command is more natural, and it's also consistent with other CLI tools, such as "docker volume rm" command.

@tusharnt
Copy link
Contributor

tusharnt commented Mar 2, 2017

Even tests need to be fixed.

@msterin
Copy link
Contributor

msterin commented Mar 6, 2017

The suggested change will leave 'tenant ' set inconsistent.
We should not do it- instead, if it is an issue, @shaominchen please enumerate here all admin command using --name or positional name, and mark those in need for change.

@shaominchen
Copy link
Contributor Author

@msterin I don't think this will leave "tenant set" inconsistent. For CUD operations, only D (rm) does not need "--name". Add & set still need "--name". This is just a convention for most command lines tools.

@pshahzeb
Copy link
Contributor

pshahzeb commented Mar 6, 2017

From https://docs.docker.com/engine/reference/commandline/docker/
I think many, if not all, docker commands use positional name instead of --name for all CUD operations. (only volume create requires --name)

I think we can go ahead with something consistent for all commands. Right now, only policy rm command takes positional name. All other commands require --name.

@msterin
Copy link
Contributor

msterin commented Mar 6, 2017

This is just a convention for most command lines tools.
I do not think it's true for "most" CLIs - can you give us examples ?
At any rate, It is certainly not true for most Docker subcommands, and it is super confusing IMO as the user has to think every time what is the command format.

A natural way is to use positionals. I am fine with using --name also (as Docker user to do but seems to have moved away from) as long as it's consistent across all commans.

So I agree with @pshahzeb here. And if we do it, we also need to change the related doc and test, and do it in 0.13 or GA , it will be more impacful later.

@shaominchen
Copy link
Contributor Author

@msterin Some examples:
"docker create" and "docker volume create" use "--name", but "docker rm" and "docker volume rm" use positional name;
"vmdkops_admin policy create" and "vmdkops_admin policy update" use "--name", but "vmdkops_admin policy rm" use positional name

Anyway, I agree that keep consistency (especially local consistency) is very important.

@pshahzeb
Copy link
Contributor

pshahzeb commented Mar 6, 2017

@shaominchen @msterin
I think correcting policy rm command to make use of --name would be more safe. Since it is a small change, would make all the commands consistent, and would cause less inconvenience to existing users as well

@shaominchen
Copy link
Contributor Author

@pshahzeb I agree.

@pshahzeb pshahzeb closed this as completed Mar 9, 2017
@shuklanirdesh82
Copy link
Contributor

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

pshahzeb pushed a commit that referenced this issue Mar 13, 2017
…mmands

1. Usage of --name option instead of positional name for admin policy rm command

2. Note for docker volume inspect command in case of zeroedthick and eagerzeroedthick disk formats
   It shows allocated size to be total size plus the size of replicas

Fixes #922 #961
msterin pushed a commit that referenced this issue Mar 14, 2017
…mmands (#1030)

1. Usage of --name option instead of positional name for admin policy rm command

2. Note for docker volume inspect command in case of zeroedthick and eagerzeroedthick disk formats
   It shows allocated size to be total size plus the size of replicas

Fixes #922 #961
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants