Design LGTM.

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 ----------------------------------------------------------------------

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

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.

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.

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

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

@dnephin needs a rebase 🙇

This is still green, and ready for review!

LGTM

docs-review, cc @thaJeztah

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 😊

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?

@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

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

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

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

; 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?

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

ping

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 😢

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.