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 entrypoint flags to service cli. #29228

Merged
merged 1 commit into from Mar 29, 2017

Conversation

@dnephin
Member

dnephin commented Dec 7, 2016

Fixes #29171

Adds an --entrypoint flag to docker service create and docker service update.

I believe this also fixes a (possibly unreported) bug in docker service update when the arg to --args was an invalid shlex string. The error was being ignored.

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Dec 7, 2016

Contributor

Design LGTM.

Contributor

icecrime commented Dec 7, 2016

Design LGTM.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 8, 2016

Member

Janky failure seems unrelated

01:02:48.101 
01:02:48.101 ----------------------------------------------------------------------
01:02:48.101 FAIL: docker_cli_authz_unix_test.go:320: DockerAuthzSuite.TestAuthZPluginAllowEventStream
01:02:48.101 
01:02:48.101 [d159ee652cdf6] waiting for daemon to start
01:02:48.101 [d159ee652cdf6] daemon started
01:02:48.102 docker_cli_authz_unix_test.go:379:
01:02:48.102     assertURIRecorded(c, s.ctrl.requestsURIs, fmt.Sprintf("/containers/%s/start", containerID))
01:02:48.102 docker_cli_authz_unix_test.go:477:
01:02:48.102     c.Fatalf("Expected to find URI '%s', recorded uris '%s'", uri, strings.Join(uris, ","))
01:02:48.102 ... Error: Expected to find URI '/containers/6f75dddae2b993199c481634ef5bed573ec609d7a7b3b864f37cbf3abc7d652d/start', recorded uris '/_ping,/info,/_ping,/v1.26/images/load?quiet=1,/_ping,/v1.26/events?since=1481155762,/_ping,/v1.26/containers/create,/v1.26/events?filters=%7B%22container%22%3A%7B%226f75dddae2b993199c481634ef5bed573ec609d7a7b3b864f37cbf3abc7d652d%22%3Atrue%7D%2C%22type%22%3A%7B%22container%22%3Atrue%7D%7D,/_ping,/v1.26/containers/6f75dddae2b993199c481634ef5bed573ec609d7a7b3b864f37cbf3abc7d652d/json'
01:02:48.104 
01:02:48.104 [d159ee652cdf6] exiting daemon
01:02:49.526 
01:02:49.526 ----------------------------------------------------------------------
Member

AkihiroSuda commented Dec 8, 2016

Janky failure seems unrelated

01:02:48.101 
01:02:48.101 ----------------------------------------------------------------------
01:02:48.101 FAIL: docker_cli_authz_unix_test.go:320: DockerAuthzSuite.TestAuthZPluginAllowEventStream
01:02:48.101 
01:02:48.101 [d159ee652cdf6] waiting for daemon to start
01:02:48.101 [d159ee652cdf6] daemon started
01:02:48.102 docker_cli_authz_unix_test.go:379:
01:02:48.102     assertURIRecorded(c, s.ctrl.requestsURIs, fmt.Sprintf("/containers/%s/start", containerID))
01:02:48.102 docker_cli_authz_unix_test.go:477:
01:02:48.102     c.Fatalf("Expected to find URI '%s', recorded uris '%s'", uri, strings.Join(uris, ","))
01:02:48.102 ... Error: Expected to find URI '/containers/6f75dddae2b993199c481634ef5bed573ec609d7a7b3b864f37cbf3abc7d652d/start', recorded uris '/_ping,/info,/_ping,/v1.26/images/load?quiet=1,/_ping,/v1.26/events?since=1481155762,/_ping,/v1.26/containers/create,/v1.26/events?filters=%7B%22container%22%3A%7B%226f75dddae2b993199c481634ef5bed573ec609d7a7b3b864f37cbf3abc7d652d%22%3Atrue%7D%2C%22type%22%3A%7B%22container%22%3Atrue%7D%7D,/_ping,/v1.26/containers/6f75dddae2b993199c481634ef5bed573ec609d7a7b3b864f37cbf3abc7d652d/json'
01:02:48.104 
01:02:48.104 [d159ee652cdf6] exiting daemon
01:02:49.526 
01:02:49.526 ----------------------------------------------------------------------
@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Dec 8, 2016

Member

SGTM 👍

I believe this also fixes a (possibly unreported) bug in docker service update when the arg to --args was an invalid shlex string. The error was being ignored.

It would be great if we can have an IT for the issue

Member

AkihiroSuda commented Dec 8, 2016

SGTM 👍

I believe this also fixes a (possibly unreported) bug in docker service update when the arg to --args was an invalid shlex string. The error was being ignored.

It would be great if we can have an IT for the issue

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Dec 8, 2016

Contributor

Suppose I create a service with:

docker service create --name myservice busybox top

Later I decide I want to change the command which is run, so I try to change the entrypoint:

docker service update --entrypoint httpd myservice

I think that would run httpd top, which is unintuitive.

And if I realize the mistake and try to fix it, I think --entrypoint "" would reverse the change, but perhaps there should be a simpler way to clear an overridden entrypoint.

Contributor

aaronlehmann commented Dec 8, 2016

Suppose I create a service with:

docker service create --name myservice busybox top

Later I decide I want to change the command which is run, so I try to change the entrypoint:

docker service update --entrypoint httpd myservice

I think that would run httpd top, which is unintuitive.

And if I realize the mistake and try to fix it, I think --entrypoint "" would reverse the change, but perhaps there should be a simpler way to clear an overridden entrypoint.

@stevvooe

This comment has been minimized.

Show comment
Hide comment
@stevvooe

stevvooe Dec 8, 2016

Contributor

LGTM

Per @aaronlehmann's concern, it seems like setting --entrypoint should clear args.

Again, I am not sure what the application of --entrypoint is to update.

Contributor

stevvooe commented Dec 8, 2016

LGTM

Per @aaronlehmann's concern, it seems like setting --entrypoint should clear args.

Again, I am not sure what the application of --entrypoint is to update.

@dnephin dnephin added this to the 1.13.0 milestone Dec 8, 2016

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 9, 2016

Member

On docker run, the args are always cleared if the entrypoint is updated, but if we can detect if the entrypoint actually changed, it would make sense to not reset if it's not modified

Member

thaJeztah commented Dec 9, 2016

On docker run, the args are always cleared if the entrypoint is updated, but if we can detect if the entrypoint actually changed, it would make sense to not reset if it's not modified

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Dec 16, 2016

Member

Sorry, I'm not clear on how to remove the args. Is it []string{""} ?

Member

dnephin commented Dec 16, 2016

Sorry, I'm not clear on how to remove the args. Is it []string{""} ?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jan 12, 2017

Member

@dnephin needs a rebase 🙇

Member

vdemeester commented Jan 12, 2017

@dnephin needs a rebase 🙇

@thaJeztah thaJeztah modified the milestones: 1.13.1, 1.13.0 Jan 18, 2017

@vieux vieux modified the milestones: 1.14.0, 1.13.1 Jan 25, 2017

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Jan 30, 2017

Member

This is still green, and ready for review!

Member

dnephin commented Jan 30, 2017

This is still green, and ready for review!

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jan 31, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Jan 31, 2017

LGTM

@vdemeester

@dnephin could we have a test to make sure "it seems like setting --entrypoint should clear args." 👼

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Mar 15, 2017

Member

docs-review, cc @thaJeztah

Member

vdemeester commented Mar 15, 2017

docs-review, cc @thaJeztah

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 16, 2017

Member

This will need an update to the CLI reference to document the new flag https://github.com/docker/docker/blob/master/docs/reference/commandline/service_update.md#service-update

And some examples added https://github.com/docker/docker/blob/master/docs/reference/commandline/service_update.md#examples

I gave this a spin to see how it works, but stumbled upon some weird behavior;

docker service create --tty --name pinger busybox
docker service update --entrypoint="/bin/sh -c \"ping google.com\"" pinger
"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "/bin/sh",
        "-c",
        "ping google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},
docker service update --entrypoint="/bin/sh" --args="-c" --args="\"ping google.com\"" pinger

This doesn't work, because the -c gets lost

"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "/bin/sh"
    ],
    "Args": [
        "ping google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},
docker service update --entrypoint="/bin/sh -c" --args="\"ping google.com\"" pinger

This works;

"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "/bin/sh",
        "-c"
    ],
    "Args": [
        "ping google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},
docker service update --entrypoint="ping" --args="-n1000" --args="google.com" pinger

This also doesn't work; the -n1000 arg gets lost;

"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "ping"
    ],
    "Args": [
        "google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},

It looks like there's something wrong with the way --args is handled. Probably not related to this PR 😊

Member

thaJeztah commented Mar 16, 2017

This will need an update to the CLI reference to document the new flag https://github.com/docker/docker/blob/master/docs/reference/commandline/service_update.md#service-update

And some examples added https://github.com/docker/docker/blob/master/docs/reference/commandline/service_update.md#examples

I gave this a spin to see how it works, but stumbled upon some weird behavior;

docker service create --tty --name pinger busybox
docker service update --entrypoint="/bin/sh -c \"ping google.com\"" pinger
"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "/bin/sh",
        "-c",
        "ping google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},
docker service update --entrypoint="/bin/sh" --args="-c" --args="\"ping google.com\"" pinger

This doesn't work, because the -c gets lost

"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "/bin/sh"
    ],
    "Args": [
        "ping google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},
docker service update --entrypoint="/bin/sh -c" --args="\"ping google.com\"" pinger

This works;

"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "/bin/sh",
        "-c"
    ],
    "Args": [
        "ping google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},
docker service update --entrypoint="ping" --args="-n1000" --args="google.com" pinger

This also doesn't work; the -n1000 arg gets lost;

"ContainerSpec": {
    "Image": "busybox:latest@sha256:32f093055929dbc23dec4d03e09dfe971f5973a9ca5cf059cbfb644c206aa83f",
    "Command": [
        "ping"
    ],
    "Args": [
        "google.com"
    ],
    "TTY": true,
    "DNSConfig": {}
},

It looks like there's something wrong with the way --args is handled. Probably not related to this PR 😊

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 16, 2017

Member

This will need an update to the CLI reference to document the new flag

Now that #30514 is merged, the flags should be coming from the source code. Do we still need to do this?

Member

dnephin commented Mar 16, 2017

This will need an update to the CLI reference to document the new flag

Now that #30514 is merged, the flags should be coming from the source code. Do we still need to do this?

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 16, 2017

Member

@thaJeztah that's not the way --args works. It should be this:

docker service update --entrypoint="/bin/sh" --args="-c \"ping google.com\"" pinger
docker service update --entrypoint="ping" --args="-n1000 google.com" pinger

args is the same type of field as --entrypoint, it's a single string that will be parsed by shlex

Member

dnephin commented Mar 16, 2017

@thaJeztah that's not the way --args works. It should be this:

docker service update --entrypoint="/bin/sh" --args="-c \"ping google.com\"" pinger
docker service update --entrypoint="ping" --args="-n1000 google.com" pinger

args is the same type of field as --entrypoint, it's a single string that will be parsed by shlex

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 16, 2017

Member

Now that #30514 is merged, the flags should be coming from the source code. Do we still need to do this?

We should have a look at that; either have a script to update the USAGE in the Markdown, or remove the USAGE output; use case is that there's still a lot of people viewing the reference docs on GitHub

Member

thaJeztah commented Mar 16, 2017

Now that #30514 is merged, the flags should be coming from the source code. Do we still need to do this?

We should have a look at that; either have a script to update the USAGE in the Markdown, or remove the USAGE output; use case is that there's still a lot of people viewing the reference docs on GitHub

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 16, 2017

Member

that's not the way --args works. It should be this:

Yes I doubted about that; wondering why --args is a list, and not a string

Member

thaJeztah commented Mar 16, 2017

that's not the way --args works. It should be this:

Yes I doubted about that; wondering why --args is a list, and not a string

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 16, 2017

Member

We should definitely remove the usage in Markdown. That's what I did for the man pages.

Member

dnephin commented Mar 16, 2017

We should definitely remove the usage in Markdown. That's what I did for the man pages.

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 16, 2017

Member

; wondering why --args is a list, and not a string

Oh good point. Since ShlexOpt is just this type ShlexOpt []string, it "just works", because the []string can be cast to the ShelxOpt type.

It would probably be good to fix the code and make it more consistent, but maybe in a fresh PR?

Member

dnephin commented Mar 16, 2017

; wondering why --args is a list, and not a string

Oh good point. Since ShlexOpt is just this type ShlexOpt []string, it "just works", because the []string can be cast to the ShelxOpt type.

It would probably be good to fix the code and make it more consistent, but maybe in a fresh PR?

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 16, 2017

Member

It would probably be good to fix the code and make it more consistent, but maybe in a fresh PR?

Sure! I just noticed it, and wrongly assumed it would accept multiple args to be passed separately

Member

thaJeztah commented Mar 16, 2017

It would probably be good to fix the code and make it more consistent, but maybe in a fresh PR?

Sure! I just noticed it, and wrongly assumed it would accept multiple args to be passed separately

@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 24, 2017

Member

ping

Member

dnephin commented Mar 24, 2017

ping

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Mar 28, 2017

Member

Oh, sorry, I thought you were planning to add the flag to the CLI docs, while we haven't made changes yet on the CLI reference on GitHub. Looks like it needs a rebase now 😢

Member

thaJeztah commented Mar 28, 2017

Oh, sorry, I thought you were planning to add the flag to the CLI docs, while we haven't made changes yet on the CLI reference on GitHub. Looks like it needs a rebase now 😢

Add entrypoint flags to service cli.
Signed-off-by: Daniel Nephin <dnephin@docker.com>
@dnephin

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Mar 28, 2017

Member

Rebased.

We did the work to automate the cli reference so that we could remove the need to do this extra work. I would be happy to remove all the flags from the docs in another PR.

Member

dnephin commented Mar 28, 2017

Rebased.

We did the work to automate the cli reference so that we could remove the need to do this extra work. I would be happy to remove all the flags from the docs in another PR.

@thaJeztah

LGTM

@vdemeester vdemeester merged commit cf19bd4 into moby:master Mar 29, 2017

4 of 6 checks passed

windowsRS1 Jenkins build Docker-PRs-WoW-RS1 11910 has failed
Details
z Jenkins build Docker-PRs-s390x 818 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 32207 has succeeded
Details
janky Jenkins build Docker-PRs 40821 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 968 has succeeded
Details

@dnephin dnephin deleted the dnephin:add-entrypoint-to-service-cli branch Mar 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment