New `docker daemon` command by tiborvass · Pull Request #13771 · moby/moby

tiborvass · 2015-06-05T17:38:28Z

This patch creates a new cli package that allows to combine both client
and daemon commands (there is only one daemon command: docker daemon).

The -d and --daemon top-level flags are deprecated and a special
message is added to prompt the user to use docker daemon.

Providing top-level daemon-specific flags for client commands result
in an error message prompting the user to use docker daemon.

This patch does not break any old but correct usages.

Signed-off-by: Tibor Vass tibor@docker.com

Thanks to @rhatdan and @shishir-a412ed for their huge patience and their initial work.

Ref #6055 #9255 #10535 #11418

icecrime · 2015-06-05T17:45:56Z

Design was approved in the preceding PR, and it seems to be in line with the earlier discussions, so I suggest we directly go to code review.

thaJeztah · 2015-06-08T17:19:49Z

ping @albers for reviewing completion scripts :-) ❤️

albers · 2015-06-10T16:36:20Z

contrib/completion/bash/docker

The preceding three options are not boolean and should call _filedir

I don't know much about _filedir would you mind showing me a fix?

See the completion for --pidfile|-p|--tlscacert|--tlscert|--tlskey) in _docker_daemon().
By the way. if you would prefer to concentrate on real coding stuff, I could take over the bash completion part. Just give me a ping.

--tlscacert|--tlscert|--tlskey should also be removed from _docker_daemon() as they are already treated globally in _docker_docker()

albers · 2015-06-10T17:07:04Z

@thaJeztah I added some comments and I offer to take over the bash completion part. It's a bit more complex than usual and quite hacky.
I cannot comment on the zsh part because I'm not yet familiar with it.

thaJeztah · 2015-06-10T17:10:53Z

Thanks so much @albers, I knew this was quite a big change, so happy to have you on board here!

@tiborvass what's the easiest way for @albers to carry the bash completion?

tiborvass · 2015-06-10T17:12:36Z

@thaJeztah I can remove it or leave it as-is for now until we have some more review on the docker side of things.

albers · 2015-06-11T13:39:29Z

I think it's better to remove the completion part here. It will leave the remaining part more focussed and my follow up PR more self-contained.
I will also address your addition of the zfs storage driver in another PR.

thaJeztah · 2015-06-11T16:18:36Z

Thanks @albers!

calavera · 2015-06-18T21:30:39Z

api/client/help.go

@tiborvass can we remove this file?

calavera · 2015-06-18T21:50:27Z

root@d# ./docker daemon
INFO[0000] [graphdriver] using prior storage driver "aufs"
INFO[0000] Listening for HTTP on unix (/var/run/docker.sock)
WARN[0000] Running modprobe bridge nf_nat failed with message: , error: exit status 1
WARN[0000] Your kernel does not support swap memory limit.
INFO[0000] Loading containers: start.
............
INFO[0000] Loading containers: done.
INFO[0000] Daemon has completed initialization

😻 LGTM after a rebase 😉

thaJeztah · 2015-06-18T23:08:01Z

contrib/vagrant-docker/README.md

does docker daemon -H=tcp://0.0.0.0:2375 also work?

Aside from the technical aspect, IMHO the =argsyntax is confusing when used on a short option. It is very common for the long form options but I cannot recall having seen this elsewhere before.
I recommend using either -H tcp://0.0.0.0:2375 or --host=tcp://0.0.0.0:2375

duglin · 2015-06-18T23:51:27Z

rebase needed

duglin · 2015-06-18T23:51:36Z

and probably a squash

tiborvass · 2015-07-24T00:07:29Z

@calavera that shows you're not testing this PR :)

$ DOCKER_CLIENTONLY=1 ./hack/make.sh binary
$ docker daemon -H 0.0.0.0:3456
docker: 'daemon' is not a docker command.
See 'docker --help'.

calavera · 2015-07-24T00:09:40Z

oh, much better! thanks

calavera · 2015-07-24T00:15:00Z

The new changes also LGTM

Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com> Signed-off-by: Tibor Vass <tibor@docker.com>

Signed-off-by: Tibor Vass <tibor@docker.com>

duglin · 2015-07-24T01:59:39Z

cli/cli.go

what if its not an initErr ?

then it moves on to if len(args) > 0

well yea, I got that. Just expected it to do something with it. I guess we'll deal with it at line 93.

I kept the logic from before: https://github.com/docker/docker/blob/37209190c76de66531acba848f1537da2899e32d/api/client/cli.go#L90

duglin · 2015-07-24T02:25:01Z

way too much code to review :-) but it seems good. We can fix any bugs that pop-up later.
LGTM!

New `docker daemon` command

jessfraz · 2015-07-24T02:30:50Z

no going back now ;)

shishir-a412ed · 2015-07-24T03:20:41Z

@tiborvass
👏 👏 👏 👏

thaJeztah · 2015-07-24T06:32:41Z

Oh yes!!

I did a quick scan of the docs changes and the changes LGTM, I hope we got them all :-)

thaJeztah · 2015-07-24T06:33:54Z

ping @albers - hope you are able to work on the completion scripts now 😄

albers · 2015-07-24T06:54:10Z

It will take about one week until I can get back to it. I'm reopening #13934 to indicate that it's in progress.

thaJeztah · 2015-07-24T08:27:07Z

Thanks for the heads-up, @albers!

rhatdan · 2015-07-24T11:40:47Z

Finally, this might have been one of the Oldest Patchsets/requests. Thanks.

thaJeztah · 2015-07-24T15:14:31Z

Thank you @rhatdan, IIRC, you started this PR-relay originally 👍

albers · 2015-07-31T18:42:57Z

@sdurrheimer Hi Steve, docker -d was replaced by the new docker daemon command. Maybe you could add this to zsh completion?

thaJeztah · 2015-07-31T20:09:36Z

Think we need the fish completions updated as well; I think those are auto-generated

Replace the deprecated `-d` command line flag with the new command name `daemon`. See moby/moby#13771 for details.

GordonTheTurtle added the status/0-triage label Jun 5, 2015

tiborvass mentioned this pull request Jun 5, 2015

Replacing -d/--daemon with daemon command. Implementation of 'docker daemon' command #10535

Closed

icecrime added status/1-design-review and removed status/0-triage labels Jun 5, 2015

icecrime added status/2-code-review and removed status/1-design-review labels Jun 5, 2015

thaJeztah added this to the 1.8.0 milestone Jun 5, 2015

tiborvass mentioned this pull request Jun 8, 2015

Fixes Issue 9677: Docker client errors out, when daemon flags are passed to docker subcommands #11418

Closed

tiborvass force-pushed the daemon-cli branch 3 times, most recently from 4b1f64b to e03479c Compare June 9, 2015 20:48

albers reviewed Jun 10, 2015
View reviewed changes

albers mentioned this pull request Jun 14, 2015

[WIP] Add docker daemon to bash completion #13934

Closed

calavera reviewed Jun 18, 2015
View reviewed changes

thaJeztah reviewed Jun 18, 2015
View reviewed changes

icecrime assigned tiborvass Jul 7, 2015

thaJeztah mentioned this pull request Jul 8, 2015

Define Docker's deprecation policy #13900

Merged

Shishir Mahajan and others added 2 commits July 23, 2015 20:31

Add and modify tests for legacy and new daemon invokations

e7fc632

Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com> Signed-off-by: Tibor Vass <tibor@docker.com>

Update docs to use the new docker daemon command

e246f1e

Signed-off-by: Tibor Vass <tibor@docker.com>

tiborvass force-pushed the daemon-cli branch from d38f085 to e246f1e Compare July 24, 2015 00:31

duglin reviewed Jul 24, 2015
View reviewed changes

jessfraz pushed a commit that referenced this pull request Jul 24, 2015

Merge pull request #13771 from tiborvass/daemon-cli

7674f21

New `docker daemon` command

jessfraz merged commit 7674f21 into moby:master Jul 24, 2015

tiborvass added impact/changelog impact/cli impact/deprecation and removed status/2-code-review labels Jul 24, 2015

tiborvass deleted the daemon-cli branch July 24, 2015 15:27

albers mentioned this pull request Jul 31, 2015

Fixes to bash completion for docker daemon #15208

Merged

albers mentioned this pull request Aug 10, 2015

Add missing storage drivers to bash completion #15452

Merged

gesellix added a commit to gesellix/boot2docker that referenced this pull request Aug 12, 2015

use new 'daemon' command

3887b87

Replace the deprecated `-d` command line flag with the new command name `daemon`. See moby/moby#13771 for details.

gesellix mentioned this pull request Aug 12, 2015

use new 'daemon' command boot2docker/boot2docker#1034

Merged

tiborvass removed their assignment Nov 3, 2015

Conversation

tiborvass commented Jun 5, 2015

Uh oh!

icecrime commented Jun 5, 2015

Uh oh!

thaJeztah commented Jun 8, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albers commented Jun 10, 2015

Uh oh!

thaJeztah commented Jun 10, 2015

Uh oh!

tiborvass commented Jun 10, 2015

Uh oh!

albers commented Jun 11, 2015

Uh oh!

thaJeztah commented Jun 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calavera commented Jun 18, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jun 18, 2015

Uh oh!

duglin commented Jun 18, 2015

Uh oh!

tiborvass commented Jul 24, 2015

Uh oh!

calavera commented Jul 24, 2015

Uh oh!

calavera commented Jul 24, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jul 24, 2015

Uh oh!

jessfraz commented Jul 24, 2015

Uh oh!

shishir-a412ed commented Jul 24, 2015

Uh oh!

thaJeztah commented Jul 24, 2015

Uh oh!

thaJeztah commented Jul 24, 2015

Uh oh!

albers commented Jul 24, 2015

Uh oh!

thaJeztah commented Jul 24, 2015

Uh oh!

rhatdan commented Jul 24, 2015

Uh oh!

thaJeztah commented Jul 24, 2015

Uh oh!

albers commented Jul 31, 2015

Uh oh!

thaJeztah commented Jul 31, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone