Use spf13/cobra for docker tag by yongtang · Pull Request #23278 · moby/moby

yongtang · 2016-06-05T18:08:30Z

This fix is part of the effort to convert commands to spf13/cobra #23211.

Thif fix coverted command docker tag to use spf13/cobra

Signed-off-by: Yong Tang [email protected]

vdemeester · 2016-06-05T20:44:56Z

No need for [OPTIONS] here 👼

Thanks @vdemeester the pull request has been updated with [OPTIONS] removed.

dnephin · 2016-06-05T23:27:42Z

I don't think we should remove these tests. I think we can fix the behaviour.

If you check out run, I use flags.SetInterspersed(false) in the NewRunCommand(). This will making it stop looking for flags once it finds the first non-flag arg.

Once that' s done we should get the -busybox:test arg as an arg, instead of failing flag validation, which should let us get the old error message out.

Thanks @dnephin. Let me update the pull request to address this issue.

yongtang · 2016-06-06T01:39:54Z

Thanks @dnephin. I added flags.SetInterspersed(false) though it is still shows:

docker tag a -b
"docker tag" requires exactly 2 argument(s).
See 'docker tag --help'.

Usage:  docker tag IMAGE[:TAG] [REGISTRYHOST/][USERNAME/]NAME[:TAG]

Tag an image into a repository

After some investigation, it seems that ExactArgs() is not passing the expected args.
So I changed

        if len(args) == number

to

        if len(cmd.Flags().Args()) == number

in ExactArgs().

Now the command behaves as expected.

Just want to make sure there is no unintended side effect with this change.

dnephin · 2016-06-06T01:43:28Z

I fixed it in cobra, and @vdemeester should be picking up the fix in one of the PRs, if you want to keep that change for now, maybe add a TODO saying it should be fixed in cobra?

yongtang · 2016-06-06T02:23:26Z

@dnephin I just added a TODO in the commit and updated the pull request. Thanks for the help!

vdemeester · 2016-06-06T08:44:58Z

Updated in #23269 😉

thaJeztah · 2016-06-06T11:53:28Z

Oh, sorry @yongtang this one needs a rebase as well

yongtang · 2016-06-06T12:44:50Z

Thanks @vdemeester I will keep an eye on #23269 and remove TODO if it get merged. And thanks @thaJeztah the PR has been rebased.

yongtang · 2016-06-07T02:05:27Z

The pull request has been rebased. It is likely there is a failing test. The failure is caused by an issue in cobra, which has been fixed (not yet merged into the docker yet). Once #23269 is merged the the failure will be fixed.

thaJeztah · 2016-06-07T12:31:04Z

@yongtang #23269 was merged, so you can rebase and fix the failure

yongtang · 2016-06-07T12:38:51Z

Thanks @thaJeztah the PR has been rebased and fixed.

This fix is part of the effort to convert commands to spf13/cobra moby#23211. Thif fix coverted command `docker tag` to use spf13/cobra Signed-off-by: Yong Tang <[email protected]>

dnephin · 2016-06-07T15:20:34Z

LGTM

cpuguy83 · 2016-06-07T15:51:18Z

LGTM

vdemeester · 2016-06-07T15:53:50Z

LGTM 🐵

thaJeztah · 2016-06-07T19:55:26Z

All green! Merging

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

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

vdemeester added this to the 1.12.0 milestone Jun 5, 2016

dnephin mentioned this pull request Jun 5, 2016

Convert commands to spf13/cobra #23211

Closed

43 tasks

vdemeester reviewed Jun 5, 2016
View reviewed changes

yongtang force-pushed the 23211-spf13-cobra-tag branch from 31d5515 to 62f1fda Compare June 5, 2016 21:26

dnephin reviewed Jun 5, 2016
View reviewed changes

yongtang force-pushed the 23211-spf13-cobra-tag branch from 62f1fda to 02fca22 Compare June 6, 2016 01:33

yongtang force-pushed the 23211-spf13-cobra-tag branch from 02fca22 to c1ca169 Compare June 6, 2016 02:22

yongtang force-pushed the 23211-spf13-cobra-tag branch from c1ca169 to 607d528 Compare June 6, 2016 12:39

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

yongtang force-pushed the 23211-spf13-cobra-tag branch from 607d528 to 1ad8f73 Compare June 7, 2016 02:03

yongtang force-pushed the 23211-spf13-cobra-tag branch from 1ad8f73 to ed658f1 Compare June 7, 2016 12:37

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

Use spf13/cobra for docker tag

ba7324f

This fix is part of the effort to convert commands to spf13/cobra moby#23211. Thif fix coverted command `docker tag` to use spf13/cobra Signed-off-by: Yong Tang <[email protected]>

yongtang force-pushed the 23211-spf13-cobra-tag branch from ed658f1 to ba7324f Compare June 7, 2016 15:13

vdemeester added status/4-merge and removed status/2-code-review labels Jun 7, 2016

thaJeztah merged commit 55a8bfa into moby:master Jun 7, 2016

yongtang deleted the 23211-spf13-cobra-tag branch June 8, 2016 02:04

Conversation

yongtang commented Jun 5, 2016

Uh oh!

vdemeester Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

yongtang Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

yongtang Jun 6, 2016

Choose a reason for hiding this comment

Uh oh!

yongtang commented Jun 6, 2016

Uh oh!

dnephin commented Jun 6, 2016

Uh oh!

yongtang commented Jun 6, 2016

Uh oh!

vdemeester commented Jun 6, 2016

Uh oh!

thaJeztah commented Jun 6, 2016

Uh oh!

yongtang commented Jun 6, 2016

Uh oh!

yongtang commented Jun 7, 2016

Uh oh!

thaJeztah commented Jun 7, 2016

Uh oh!

yongtang commented Jun 7, 2016

Uh oh!

dnephin commented Jun 7, 2016

Uh oh!

cpuguy83 commented Jun 7, 2016

Uh oh!

vdemeester commented Jun 7, 2016

Uh oh!

thaJeztah commented Jun 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants