Use spf13/cobra for `docker run` and `docker create` by dnephin · Pull Request #23159 · moby/moby

dnephin · 2016-06-01T05:24:50Z

This PR builds off of #22769

As requested in #22769 (comment) this is a PoC to show that cobra and pflag will work for a more involved command like docker run.

thaJeztah · 2016-06-01T08:43:56Z

ping @duglin @LK4D4 @vdemeester PTAL

vdemeester · 2016-06-01T14:31:21Z

Code looks good 👍 (and I like the TODOs on the flags 👼)
Tried it a little bit, seems fine but didn't have time for a complete test of the run command.

thaJeztah · 2016-06-01T14:43:50Z

Hm, looks like I found a really weird issue;

This fails:

docker run -v /bla/bla:/foo/bar:rw,Z -dit -p 127.0.0.1:80:80 alpine sh

So does this:

docker run -it --rm alpine sh

But using "busybox" works;

docker run -v /bla/bla:/foo/bar:rw,Z -dit -p 127.0.0.1:80:80 busybox sh

docker run -it --rm busybox sh

For alpine it sends an empty command;

{
  "AttachStderr": false,
  "AttachStdin": false,
  "AttachStdout": false,
  "Cmd": null,
  "Domainname": "",
  "Entrypoint": null,
  "Env": [

  ],
..

vdemeester · 2016-06-01T14:44:14Z

Hum some weirdies though (tried it out with @thaJeztah ) :

root@fb4c2eda9e89:/go/src/github.com/docker/docker# ./bundles/latest/binary-client/docker run -v /var:/foo:rw,Z -dit -p 127.0.0.1:80:80 busybox sh
b5424362a55dc975347e3ebb097db45ba8389f69d84e00586e230b50160af160
root@fb4c2eda9e89:/go/src/github.com/docker/docker# ./bundles/latest/binary-client/docker run -v /var:/foo:rw,Z -dit -p 127.0.0.1:80:80 alpine sh
Unable to find image 'alpine:latest' locally
latest: Pulling from library/alpine
d0ca440e8637: Pull complete 
Digest: sha256:9dd95238d7f6ac0fabafd7b52e52d8a7c63a5e5b390d27e13cb5da8ddd2f0d98
Status: Downloaded newer image for alpine:latest
./bundles/latest/binary-client/docker: Error response from daemon: No command specified.
See './bundles/latest/binary-client/docker run --help'.
root@fb4c2eda9e89:/go/src/github.com/docker/docker# ./bundles/latest/binary-client/docker run -v /var:/foo:rw,Z -dit -p 127.0.0.1:80:80 alpine sh
./bundles/latest/binary-client/docker: Error response from daemon: No command specified.
See './bundles/latest/binary-client/docker run --help'.

Looks like the command is not passed (busybox has a default command, alpine does not)

thaJeztah · 2016-06-01T14:46:19Z

Ok we (@vdemeester) found the difference; it appears to fail if there's no default command in the image. Doing this works:

FROM alpine
CMD sh

docker build -t frommie .
docker run -it --rm frommie sh

thaJeztah · 2016-06-01T14:54:25Z

Some more testing, and this works ok

export FOOBAR=blabla

docker run \
    -v /bla/bla:/foo/bar:rw,Z \
    -dit \
    -p 127.0.0.1:80:80 \
    -v $(docker volume create --name foobar):/foobar \
    -u 123:456 \
    --label bla=foo \
    -l bla=bar \
    -e FOOBAR \
    -e something=something \
    --memory 300m \
    --security-opt apparmor:unconfined \
    --security-opt seccomp=unconfined \
    --name cobra \
    busybox sh

dnephin · 2016-06-01T15:10:38Z

Oops, sorry about that. I had a fix for that issue and I forgot to push it before opening the PR.

That issue is now fixed.

dnephin · 2016-06-01T21:19:05Z

docker create is implemented now as well, so more of the tests should pass

duglin · 2016-06-02T02:28:01Z

Is this something that should be in a generic spot so it can be reused by other commands?

Doesn't seem to be used anywhere else. I just moved this, it already existed before my change.

hmm interesting.

I think this is trying to mimic the error message that is normally raised when a flag or value is incorrect. These two commands do a bunch of validation on the flag values after the initial parsing, where as I think many commands don't do any additional validation.

dnephin · 2016-06-02T15:02:44Z

The build is green!

cpuguy83 · 2016-06-02T17:04:25Z

needs rebase

Move container options into a struct so that tests should pass. Remove unused FlagSet arg from Parse Disable interspersed args on docker run Signed-off-by: Daniel Nephin <[email protected]>

Return the correct status code on flag parsins errors. Signed-off-by: Daniel Nephin <[email protected]>

thaJeztah · 2016-06-03T18:46:47Z

ow; flaky/faulty test on WindowsTP5 (not related); guess this is the issue with Windows not liking filenames named #1

17:54:09 ----------------------------------------------------------------------
17:54:09 FAIL: docker_cli_build_test.go:6760: DockerSuite.TestBuildDockerignoreComment
17:54:09 
17:54:09 docker_cli_build_test.go:6791:
17:54:09     c.Fatal(err)
17:54:09 ... Error: failed to build the image: Sending build context to Docker daemon 5.632 kB

17:54:09 Step 1 : FROM busybox
17:54:09  ---> 3f1b2939d14c
17:54:09 Step 2 : ADD . /tmp/
17:54:09  ---> 78175a922461
17:54:09 Removing intermediate container e893b2ece047
17:54:09 Step 3 : RUN sh -c "(ls -la /tmp/#1)"
17:54:09  ---> Running in 14e3ac5d6f7e
17:54:09 The command 'cmd /S /C sh -c "(ls -la /tmp/#1)"' returned a non-zero code: 255

thaJeztah · 2016-06-03T18:48:56Z

Opened #23246 for the test

thaJeztah · 2016-06-03T20:29:29Z

Windows test will be fixed in #23248

dnephin · 2016-06-03T22:18:42Z

it's green!

thaJeztah · 2016-06-03T23:13:52Z

LGTM

vdemeester · 2016-06-04T10:54:41Z

Arf, needs a rebase 😓

thaJeztah · 2016-06-04T10:58:45Z

Ah, probably because this one was merged #23241. I hoped this one would get merged first

vdemeester · 2016-06-04T11:59:23Z

LGTM 👼
Let's carry this to merge it quick 😝

Carry #23159 : Use spf13/cobra for `docker run` and `docker create`

Carry moby#23159 : Use spf13/cobra for `docker run` and `docker create`

dnephin added the area/cli Client label Jun 1, 2016

GordonTheTurtle added platform/windows status/0-triage labels Jun 1, 2016

thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 1, 2016

dnephin force-pushed the integrate_cobra.docker_run branch from 4825a42 to 3a7de8c Compare June 1, 2016 15:10

icecrime removed the platform/windows label Jun 1, 2016

dnephin force-pushed the integrate_cobra.docker_run branch from 25db6b0 to b93d092 Compare June 1, 2016 21:21

duglin reviewed Jun 2, 2016
View reviewed changes

dnephin force-pushed the integrate_cobra.docker_run branch 2 times, most recently from 02cbfad to ef96863 Compare June 2, 2016 04:30

dnephin changed the title ~~PoC: use spf13/cobra for docker run~~ Use spf13/cobra for docker run Jun 2, 2016

dnephin changed the title ~~Use spf13/cobra for docker run~~ Use spf13/cobra for docker run and docker create Jun 2, 2016

dnephin removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 2, 2016

vdemeester added status/2-code-review and removed status/0-triage labels Jun 2, 2016

dnephin force-pushed the integrate_cobra.docker_run branch from ef96863 to 2096767 Compare June 2, 2016 17:25

dnephin mentioned this pull request Jun 2, 2016

Convert commands to spf13/cobra #23211

Closed

43 tasks

Convert 'docker run' to a cobra command and to use pflags

ebac370

Move container options into a struct so that tests should pass. Remove unused FlagSet arg from Parse Disable interspersed args on docker run Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the integrate_cobra.docker_run branch from 2096767 to f0a8522 Compare June 3, 2016 17:26

Convert 'docker create' to use cobra and pflag

aaac5fd

Return the correct status code on flag parsins errors. Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the integrate_cobra.docker_run branch from f0a8522 to aaac5fd Compare June 3, 2016 17:39

thaJeztah added this to the 1.12.0 milestone Jun 3, 2016

vdemeester mentioned this pull request Jun 4, 2016

Carry #23159 : Use spf13/cobra for docker run and docker create #23253

Merged

thaJeztah closed this in #23253 Jun 4, 2016

thaJeztah added a commit that referenced this pull request Jun 4, 2016

Merge pull request #23253 from vdemeester/carry-pr-23159

6b4a46f

Carry #23159 : Use spf13/cobra for `docker run` and `docker create`

dnephin deleted the integrate_cobra.docker_run branch June 4, 2016 17:05

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#23253 from vdemeester/carry-pr-23159

8ad0a0f

Carry moby#23159 : Use spf13/cobra for `docker run` and `docker create`

Conversation

dnephin commented Jun 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Jun 1, 2016

Uh oh!

vdemeester commented Jun 1, 2016

Uh oh!

thaJeztah commented Jun 1, 2016

Uh oh!

vdemeester commented Jun 1, 2016

Uh oh!

thaJeztah commented Jun 1, 2016

Uh oh!

thaJeztah commented Jun 1, 2016

Uh oh!

dnephin commented Jun 1, 2016

Uh oh!

dnephin commented Jun 1, 2016

Uh oh!

duglin Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

duglin Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Jun 2, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jun 2, 2016

Uh oh!

cpuguy83 commented Jun 2, 2016

Uh oh!

thaJeztah commented Jun 3, 2016

Uh oh!

thaJeztah commented Jun 3, 2016

Uh oh!

thaJeztah commented Jun 3, 2016

Uh oh!

dnephin commented Jun 3, 2016

Uh oh!

thaJeztah commented Jun 3, 2016

Uh oh!

vdemeester commented Jun 4, 2016

Uh oh!

thaJeztah commented Jun 4, 2016

Uh oh!

vdemeester commented Jun 4, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dnephin commented Jun 1, 2016 •

edited

Loading