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

Add docker daemon command instead of -d #6055

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
10 participants
@rhatdan
Copy link
Contributor

rhatdan commented May 27, 2014

QE Reports that you can put lots of bogus options on the cli before the command
which are ignored if you specify a command. We should report a usage error
if a user does this.

I believe a better long term solution would be to remove the -d options and add a daemon command. This would make the handling of options the same for all use
cases.

Docker-DCO-1.1-Signed-off-by: Dan Walsh dwalsh@redhat.com (github: rhatdan)

@tianon

This comment has been minimized.

Copy link
Member

tianon commented May 27, 2014

This solution seems very error prone to me.

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented May 27, 2014

What should happen when you specify an option to be a default one, like -G docker ?

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented May 27, 2014

tianon I agree which is why I suggested we drop -d and force the command to be daemon

docker daemon [OPTIONS]
docker run [OPTIONS]

Then you would not allow options before the command.

What should happen when you specify an option to be a default one, like -G docker ?

All I can check now is if the default changed, which tells me they used the option.

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented May 27, 2014

I think we should do multiple parse.

One to parse only --daemon and common options (like -H or -D)
and after do one parse for daemon only.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented May 27, 2014

@vieux that sounds like an excellent halfway point while we wait for docker daemon to become a real command :)

(which, last time we discussed, was expected to come as soon as upstream Go managed to get us a reliable method of actual daemonization/backgrounding)

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented May 27, 2014

But that is just a complication of not using the command daemon. We have a few weeks to simpify the code, then you are stuck with it forever.

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented May 27, 2014

Why can't we do "docker daemon" now?

@cyphar

This comment has been minimized.

Copy link
Contributor

cyphar commented May 28, 2014

+1 for docker daemon. If we rewrite the code for the general flags, it will be mostly wasted effort since we'd need to migrate the daemon-related flags to CmdDaemon anyway.

@crosbymichael crosbymichael changed the title Do not allow users to specify daemon options along with commands Add docker daemon command instead of -d May 30, 2014

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented May 30, 2014

I changed the title of this PR

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Jun 27, 2014

I have updated this pull request to substitute -d and --daemon with daemon command before flag.Parse()

This means existing installs will continue to work, but print a log message if they use -d or --daemon.

@cyphar

This comment has been minimized.

Copy link
Contributor

cyphar commented Jun 28, 2014

@rhatdan: can you put a deprecated notice, so that we can remove any mention of -d in 1.0.2 or 1.1.0?

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Jun 28, 2014

I can change the log message to say that -d/--daemon is depracated and will be removed in a future release.

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Jun 30, 2014

+1 to the change, however, you still need to update cli.md and presumably there are other docs that mention it - oh there'll be a stack of distro/stuff to ping.

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Jun 30, 2014

Most of those distro pings are directly in contrib/init in our repo. ;)

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Jul 1, 2014

Fixed up contrib/init and cli.md

@tianon

This comment has been minimized.

Copy link
Member

tianon commented Jul 1, 2014

👍 contrib LGTM

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Jul 1, 2014

Docs LGTM, 🎉

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Jul 1, 2014

**docker daemon** [OPTIONS]

# DESCRIPTION
Run the docker daemon. Docker daemon is usually run within the init system in

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

The Docker daemon is usually...


**-H**, **--host**=[unix:///var/run/docker.sock]: tcp://[host:port] to bind or
unix://[/path/to/socket] to use.
The socket(s) to bind to in daemon mode specified using one or more

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

The socket(s) to bind to in daemon mode are specified ...

tcp://host:port, unix:///path/to/socket, fd://* or fd://socketfd.

**--api-enable-cors**=*true*|*false*
Enable CORS headers in the remote API. Default is false.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

The default is false.

Enable CORS headers in the remote API. Default is false.

**-b**=""
Attach containers to a pre\-existing network bridge; use 'none' to disable container networking

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

full stop.

Attach containers to a pre\-existing network bridge; use 'none' to disable container networking

**--bip**=""
Use the provided CIDR notation address for the dynamically created bridge (docker0); Mutually exclusive of \-b

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

full stop.

Enable daemon mode. Default is false.

**--dns**=""
Force Docker to use specific DNS servers

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

full stop.

Use the provided CIDR notation address for the dynamically created bridge (docker0); Mutually exclusive of \-b

**-d**=*true*|*false*
Enable daemon mode. Default is false.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

The default is false.

Force Docker to use specific DNS servers

**-g**=""
Path to use as the root of the Docker runtime. Default is `/var/lib/docker`.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

The default...

Same for other default references in the page.

Disable Docker's addition of iptables rules. Default is true.

**--mtu**=VALUE
Set the containers network mtu. Default is `1500`.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

MTU

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

container's

Set the containers network mtu. Default is `1500`.

**-p**=""
Path to use for daemon PID file. Default is `/var/run/docker.pid`

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

Full stop.

Force the Docker runtime to use a specific storage driver.

**--selinux-enabled**=*true*|*false*
Enable selinux support. Default is false.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

SELinux


# EXAMPLES

For specific examples please see the man page for the specific Docker command.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

Two uses of "specific" remove/replace one.


# HISTORY
June 2014, Originally compiled by Dan Walsh (dwalsh at redhat dot com) based
on docker.io source material and internal work.

This comment has been minimized.

Copy link
@jamtur01

jamtur01 Jul 1, 2014

Contributor

docker.com

This comment has been minimized.

Copy link
@SvenDowideit

SvenDowideit Jul 1, 2014

Contributor

yeah, I'm going to make a mega PR cleaning these up today (but if you can change it here, that'll stop them from creeping back)

@jamtur01

This comment has been minimized.

Copy link
Contributor

jamtur01 commented Jul 1, 2014

Comments on commit.

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Sep 24, 2014

@crosbymichael @vieux do you guys want this? I do.

@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Sep 24, 2014

Not really, I'm not against as long as we keep -d

@bfirsh

This comment has been minimized.

Copy link
Contributor

bfirsh commented Sep 24, 2014

Makes a lot of sense to me. It clearly separates out the concerns of client and server.

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Sep 24, 2014

I can work it again, if it has a chance of upstreaming.

Use docker daemon command instead of -d
QE Reports that you can put lots of bogus options on the cli before the command
which are ignored if you specify a command. We should report a usage error
if a user does this.

I believe a better long term solution would be to remove the -d options and add a daemon command.
This would make the handling of options the same for all use cases.

This patch will still handle the -d option by substituting "daemon" command and print a warning
message to the user.

Docker-DCO-1.1-Signed-off-by: Dan Walsh <dwalsh@redhat.com> (github: rhatdan)

@rhatdan rhatdan force-pushed the rhatdan:options branch from af6ac83 to dda0a2f Oct 6, 2014

@SvenDowideit

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Oct 7, 2014

I would loooove to deprecate docker -d :)

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Oct 7, 2014

I have asked a couple of new guys coming to my team to look into it, unless someone else is working on it. I took a quick look and my original patches are way out of data at this point.

Although all of the docs stuff is still good. :^(

@petejkim

This comment has been minimized.

Copy link

petejkim commented Oct 8, 2014

i am seeing merge conflicts in the code?

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Oct 8, 2014

Yes this patch set needs to be rewritten. The patch no longer applies. I don't have time to rework it now, but am trying to get others to do it. If you want to take a stab, go for it.

@crosbymichael

This comment has been minimized.

Copy link
Member

crosbymichael commented Oct 17, 2014

Ok, since this is not able to be merged and it's not exactly clear whether this is something that we should have in docker it maybe better to discuss this in an issue before rebasing and rewriting this PR. Sorry for the long response.

@rhatdan

This comment has been minimized.

Copy link
Contributor Author

rhatdan commented Oct 20, 2014

Well I think it is something we should definitely be in docker. I just don't have the time to rewrite the patch set. The current split causes man pages and option handling to be buggy and not easy to understand when to use certain options. IE A lot of options are only valid for use with the -d option. Changing this to daemon would clear this up.

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.