Use spf13/cobra for docker import by vdemeester · Pull Request #23269 · moby/moby

vdemeester · 2016-06-05T15:31:18Z

Moves import command to api/client/image/import.go and use cobra 🐍.

/cc @dnephin @thaJeztah @LK4D4 @cpuguy83

🐸

Signed-off-by: Vincent Demeester [email protected]

vdemeester · 2016-06-05T16:48:03Z

Hum… there is a weird error on DockerSuite.TestEventsImageImport... looking into it

dnephin · 2016-06-05T16:57:38Z

min args is set to 1, but it looks like this assumes 2.

I think this needs if len(args) > 1

Yeah definitely.

vdemeester · 2016-06-05T17:45:57Z

Hum, so cobra is eating - arguments (without saying anything which is worrying me a bit) and thus docker import - is failing with "… requires a last 1 argument(s)".

/cc @dnephin

dnephin · 2016-06-05T18:05:10Z

hmm, I'll look into it later today. Might be a bug in https://github.com/dnephin/cobra/blob/fork/command.go#L355-L393

vdemeester · 2016-06-05T18:06:52Z

@dnephin yeah I was looking into that. Note that it's just the "args" passed on the Args function of the command. The "args" passed to the Run are ok ; I'll update the PR with a small temporary fix

vdemeester · 2016-06-05T18:15:28Z

Added a FIXME 👼

dnephin · 2016-06-05T19:37:29Z

@vdemeester I pushed a fix for this. If you update the vendor to 3bfe23ade4a4657b00fb065297461ee282dfb822 it should work correctly.

The fix is here: spf13/cobra@c76d2fa

vdemeester · 2016-06-05T20:16:40Z

@dnephin are you sure on the hash, it removes the SetFlagErrorFunc function and such 😓

dnephin · 2016-06-05T23:03:59Z

hmm, it shouldn't remove anything.

Try 75205f23b3ea70dc7ae5e900d074e010c23c37e9. I rebased it on spf13/master again.

This should be right: https://github.com/dnephin/cobra/commits/fork

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

ping @vdemeester this needs a rebase now 😢

Signed-off-by: Vincent Demeester <[email protected]>

dnephin · 2016-06-06T16:32:47Z

LGTM

thaJeztah · 2016-06-07T11:07:02Z


 # cli
-clone git github.com/spf13/cobra acf60156558542e78c6f3695f74b0f871614ff55 https://github.com/dnephin/cobra.git
+clone git github.com/spf13/cobra 75205f23b3ea70dc7ae5e900d074e010c23c37e9 https://github.com/dnephin/cobra.git


should we put this fork under the "docker" organization if we're unsure our changes get accepted before 1.12?

ping @dnephin I would be fine for it 👼

That would be fine with me. There seems to be very little activity on my PRs, so I think it's unlikely they will be merged before 1.12.

thaJeztah · 2016-06-07T11:07:09Z

LGTM, thanks!

At the time the pull request: Use spf13/cobra for docker rename moby#23290 was created, there was an issue in cobra so the test `DockerSuite.TestRenameInvalidName` was updated. Now the issue in cobra has been fixed in: Use spf13/cobra for docker import moby#23269 so the test related to `docker rename` (`DockerSuite.TestRenameInvalidName`) needs to be reverted back. This fix is related to spf13/cobra moby#23211. Signed-off-by: Yong Tang <[email protected]>

Use spf13/cobra for docker import

vdemeester added the area/cli Client label Jun 5, 2016

vdemeester added this to the 1.12.0 milestone Jun 5, 2016

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

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

dnephin mentioned this pull request Jun 5, 2016

Convert commands to spf13/cobra #23211

Closed

43 tasks

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

dnephin reviewed Jun 5, 2016
View reviewed changes

vdemeester force-pushed the migrate-import-to-cobra branch from 93de9af to 54575ed Compare June 5, 2016 17:08

vdemeester force-pushed the migrate-import-to-cobra branch 2 times, most recently from ea61385 to 7622fbf Compare June 5, 2016 18:11

vdemeester reviewed Jun 5, 2016
View reviewed changes

Comment thread api/client/image/import.go Outdated

Copy link
Copy Markdown

Member Author

vdemeester Jun 5, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a FIXME 👼

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

vdemeester force-pushed the migrate-import-to-cobra branch from 7622fbf to 52d2c26 Compare June 6, 2016 08:42

vdemeester mentioned this pull request Jun 6, 2016

Use spf13/cobra for docker tag #23278

Merged

vdemeester force-pushed the migrate-import-to-cobra branch from 52d2c26 to 040e6eb Compare June 6, 2016 10:04

vdemeester added 3 commits June 6, 2016 13:58

Update cobra to 75205f23b3ea70dc7ae5e900d074e010c23c37e9

ca01e57

Signed-off-by: Vincent Demeester <[email protected]>

Migrate import command to cobra

9d2e8ab

Signed-off-by: Vincent Demeester <[email protected]>

Fix TestDiffEmptyArgClientError O:)

7878705

Signed-off-by: Vincent Demeester <[email protected]>

vdemeester force-pushed the migrate-import-to-cobra branch from 660f875 to 7878705 Compare June 6, 2016 11:58

thaJeztah reviewed Jun 7, 2016
View reviewed changes

thaJeztah merged commit f061f55 into moby:master Jun 7, 2016

vdemeester deleted the migrate-import-to-cobra branch June 7, 2016 11:07

This was referenced Jun 7, 2016

Fix failing test of docker rename #23341

Closed

Fix TestRenameInvalidName #23340

Merged

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

Merge pull request moby#23269 from vdemeester/migrate-import-to-cobra

7300075

Use spf13/cobra for docker import

Conversation

vdemeester commented Jun 5, 2016

Uh oh!

vdemeester commented Jun 5, 2016

Uh oh!

dnephin Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Jun 5, 2016

Uh oh!

dnephin commented Jun 5, 2016

Uh oh!

vdemeester commented Jun 5, 2016

Uh oh!

vdemeester Jun 5, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jun 5, 2016

Uh oh!

vdemeester commented Jun 5, 2016

Uh oh!

dnephin commented Jun 5, 2016

Uh oh!

thaJeztah commented Jun 6, 2016

Uh oh!

dnephin commented Jun 6, 2016

Uh oh!

thaJeztah Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

vdemeester Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin Jun 7, 2016

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants