Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restructure CLI Commands #26025

Merged
merged 3 commits into from Sep 19, 2016

Conversation

@dnephin
Copy link
Member

commented Aug 25, 2016

Fixes #13509
Fixes #8829
Fixes #8756

  • create a new docker image and docker container command
  • add the image and container commands as subcommands of the new command groups with a few commands renamed:
    • images -> image ls (with aliases)
    • rmi -> image rm (with aliases)
    • ps -> container ls (with aliases)
  • add image inspect and container inspect to match the other command groups
  • add 3-letter allises for all commands groups
    • docker con
    • docker img
    • docker srv
    • docker net
    • docker vol

@dnephin dnephin force-pushed the dnephin:cli-new-command-structure branch 2 times, most recently from 99adf5c to 475948a Aug 25, 2016

@icecrime

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

Absolutely LGTM on design, 🎉, 💯, etc.

My only nit, but I won't fight for it, is that I'm skeptical about the need for short aliases (docker con, docker vol, etc.).

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

commented Aug 25, 2016

docker con

image

@MHBauer

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

Not big on the short aliases, but I like the thought for everything else.

@vdemeester

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

Not a huge fan of short aliases but other than that, just like @icecrime, absolutely LGTM on design 😎 🎉

@dnephin

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2016

My rational for the short aliases is this:

It's going to be difficult to convince people to use docker container rm instead of docker rm. It's just much longer to type. Adding the short aliases make the situation a lot better because it's just 3 extra characters, instead of 9!

If we don't make it easy to use the new canonical commands we're never going to be able to remove them.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

You think we'll be able to remove something like docker rm or docker run? I doubt that.

@dnephin

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2016

docker run no, we aren't even hiding it, it's staying as a permanent alias.

The rest, not any time soon. Maybe in a year or two if we're good about converting all the docs to the new form, and communicating the new canonical way of doing things.

@stevvooe

This comment has been minimized.

Copy link
Contributor

commented Aug 27, 2016

I love this, but if we do it, we should take this chance to fix the output of inspect to default to be pretty and composite. This was a HUGE point of contention during the implementation of swarm services. Making inspect output text composed of multiple API calls will address this problem and let us really grow the usability of inspect, while addressing long standing issues.

NewPullCommand(dockerCli),
NewPushCommand(dockerCli),
NewSaveCommand(dockerCli),
NewSearchCommand(dockerCli),

This comment has been minimized.

Copy link
@stevvooe

stevvooe Aug 27, 2016

Contributor

I don't think we should carry over search to docker image. We aren't searching the local image store, we are searching remote registries. Future extension to search won't happen under docker image.

This comment has been minimized.

Copy link
@icecrime

icecrime Aug 28, 2016

Contributor

That's a good point. Maybe it belongs with login and logout under another top-level command?

This comment has been minimized.

Copy link
@dnephin

dnephin Aug 29, 2016

Author Member

Makes sense. I'll move the search command file to the correct package, and move it back to the top level.

This comment has been minimized.

Copy link
@stevvooe

stevvooe Aug 30, 2016

Contributor

There may be a docker registry command, but that is a whole 'notha can o wyrms!

cmd.AddCommand(
NewBuildCommand(dockerCli),
NewHistoryCommand(dockerCli),
NewImportCommand(dockerCli),

This comment has been minimized.

Copy link
@stevvooe

stevvooe Aug 27, 2016

Contributor

This is a container operation, if I remember correctly.

This comment has been minimized.

Copy link
@stevvooe

stevvooe Aug 27, 2016

Contributor

Nevermind. It is docker export that is for containers.

@icecrime icecrime self-assigned this Aug 28, 2016

@OJFord

This comment has been minimized.

Copy link

commented Aug 28, 2016

My rational for the short aliases is this:
It's going to be difficult to convince people to use docker container rm instead of docker rm. It's just much longer to type.

Realistically people won't do that though, they'll type docker c<tab> rm. As long as the completion is the full "container" (i.e. "con" isn't an entry) it wouldn't bother me, but for this reason I don't think it's necessary.

@dnephin

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2016

we should take this chance to fix the output of inspect to default to be pretty and composite

I'm not sure that we can change inspect, but we could definitely introduce a new command that defaults to pretty print, and hide inspect from help. Do you know if there is an issue with a proposal for that change? I'd like to keep that discussion and change separate from this PR.

@dnephin dnephin force-pushed the dnephin:cli-new-command-structure branch from 475948a to d23b413 Aug 29, 2016

@nathanleclaire

This comment has been minimized.

Copy link
Contributor

commented Aug 29, 2016

Realistically people won't do that though, they'll type docker c rm. As long as the completion is the full "container" (i.e. "con" isn't an entry) it wouldn't bother me, but for this reason I don't think it's necessary.

My 2c -- I'm not really a fan of completion on subcommands, since it requires that you install and maintain additional completion scripts instead of being something that works out of the box. In docker container's case as well, keep in mind that it's competing with docker commit and others for the same completion, so by the time you've typed enough characters to differentiate container for completion, you've already typed docker con anyway.

I wouldn't really like to see docker ps go away pretty much ever since it is such deeply ingrained habit for me, is extremely short and frequently invoked and I don't really mind having porcelain around the "true" commands (git style, i.e. "every command is just a wrapper around rebase"). But I understand the motivation.

I'd definitely like to see more elaboration around where we draw the lines here, e.g., why would it be OK for docker service scale foo=3 to alias to docker service update --replicas 3 foo (porcelain wrapper like mentioned above) but not OK for docker ps to alias to docker container ls.

@icecrime icecrime referenced this pull request Aug 29, 2016

Merged

New Data Management commands #26108

2 of 2 tasks complete
@stevvooe

This comment has been minimized.

Copy link
Contributor

commented Aug 30, 2016

I'm not sure that we can change inspect, but we could definitely introduce a new command that defaults to pretty print, and hide inspect from help. Do you know if there is an issue with a proposal for that change? I'd like to keep that discussion and change separate from this PR.

Not that I know of. File away or ping me again and I'll cook something up.

I agree that this shouldn't be a part of this PR, if we frame it that way.

@dnephin dnephin force-pushed the dnephin:cli-new-command-structure branch from d23b413 to de52a08 Sep 8, 2016

@dnephin

This comment has been minimized.

Copy link
Member Author

commented Sep 8, 2016

This is now rebased, and no longer hides the commands. It only adds the new canonical commands.

@dnephin dnephin force-pushed the dnephin:cli-new-command-structure branch from de52a08 to 7916d26 Sep 9, 2016

Only hide commands if the env variable is set.
Better formatting for usage template.
Group commands in usage to management/operation commands.
Remove the word Docker from the description of management commands.

Signed-off-by: Daniel Nephin <dnephin@docker.com>

@dnephin dnephin force-pushed the dnephin:cli-new-command-structure branch from fed1b39 to a7c8bca Sep 19, 2016

@dnephin dnephin referenced this pull request Sep 19, 2016

Merged

Create system subcommand #26716

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

Just discussed with @dnephin that we're gonna do docs (and completion scripts) in a follow-up PR. I'll open a tracking issue

@thaJeztah

This comment has been minimized.

Copy link
Member

commented Sep 19, 2016

And, it's green!

@thaJeztah thaJeztah merged commit ad9ceff into moby:master Sep 19, 2016

7 checks passed

docker/dco-signed All commits signed
Details
documentation success
Details
experimental Jenkins build Docker-PRs-experimental 23915 has succeeded
Details
janky Jenkins build Docker-PRs 32505 has succeeded
Details
userns Jenkins build Docker-PRs-userns 14517 has succeeded
Details
win2lin Jenkins build Docker-PRs-Win2Lin 31245 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 3305 has succeeded
Details

@dnephin dnephin deleted the dnephin:cli-new-command-structure branch Sep 19, 2016

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2016

LGTM !

@icecrime

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2016

👏

@gittycat

This comment has been minimized.

Copy link

commented Sep 20, 2016

Is there any chance of differentiating between Legacy|old|standalone Swarm and "Swarm Mode" at least in the doc. For instance:

Management Commands:
  ...
  node        Manage Docker Swarm mode nodes
  service     Manage Docker services
  swarm       Manage Docker legacy Swarm
  ...

The old vs new swarm is a recurring area of misunderstanding over at Stackoverflow and elsewhere. The "swarm mode" should really have been named something else in my opinion. Sorry to bring this up here but since we're stuck with "Swarm" I would love the doc to at least acknowledge that it's dealing with two different technologies and use some naming to help distinguish them.

@dnephin

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2016

@gittycat docker swarm is not the legacy swarm, it's the new "swarm mode". Only swarm is the old swarm, which is not available through this CLI.

@stevvooe

This comment has been minimized.

Copy link
Contributor

commented Sep 23, 2016

@gittycat Rule of thumb: if there is a docker in front of your command, you're probably using Swarm-mode.

I usually refer to them as "Docker Services" for clarification.

@albers

This comment has been minimized.

Copy link
Member

commented Oct 25, 2016

ping @sdurrheimer there's a lot of work waiting for you here 😰, PTAL

@sdurrheimer

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

@albers I see 😲, thx for the ping.

@lpalgarvio

This comment has been minimized.

Copy link

commented Jan 19, 2017

kudos!

i was actually using custom make scripts to do just this with almost equal commands!!!

thanks for standardizing in 1.13! saved me from opening a ticket =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.