Use spf13/cobra for the cli by dnephin · Pull Request #22769 · moby/moby

dnephin · 2016-05-16T19:19:14Z

https://github.com/spf13/cobra is a widely used framework for creating clis. I believe migrating to this framework comes with many advantages:

reduced duplication - currently usage strings exist in two places. Some of them are oddly centralized in a separate package, away from the command itself. There is also a lot of boilerplate for setting up and parsing args. Using cobra this is handled by the framework.
better organization - commands are split into separate packages for each group of commands. This structure seems to be a common convention used by many go clis. Separate packages means that compilation and unit tests will be faster. There is also a better structure for each command. The command definition, flags, options and implementation are all clearly separated instead of mixed into a single function.
less code to maintain - we can benefit from all the features in cobra without adding them to the docker cli framework
aliases and command migration - each command is an object that can be re-used, so if we wanted to migrate commands to a new structure, and keep backwards compatibility, this would allow us to accomplish such a migration very easily. (Ex: both docker images and docker image ls could exist by calling addCommand() on two different commands).

What I did

Moved more of the inspector/inspect functionality into api/client/inspect so that it can be used from multiple packages. This is necessary because with cobra the commands are in different packages, not a single package.
Created a new class CobraAdaptor that acts as an adaptor between the existing docker cli command delegation and the cobra command delegation. This allows us to migrate a few commands at a time, instead of doing it all in a single massive PR.
Use the existing DockerCli as a data object, and expose Out(), Err() and Client() so they can be used from separate packages.
Convert the volume commands to use cobra commands.

aaronlehmann · 2016-05-16T22:29:49Z

It feels a little weird to me that the struct is called CobraAdaptor but the package name is cobradaptor (one fewer a). I don't have anything against the portmanteau but I'd rather not have two confusingly similar names.

Fair enough, I'll make these consistent

cpuguy83 · 2016-05-17T04:24:03Z

MHBauer · 2016-05-19T21:25:01Z

Does it also get rid of the reflection that ends up leaving the code oddly disconnected?

cpuguy83 · 2016-05-19T21:55:49Z

Reflection is already gone :)

dnephin · 2016-05-20T21:17:21Z

A few tests are breaking because pflag doesn't do this: 0e9c40e

Personally that behaviour feels wrong. Do we want to continue to support that quote trimming?

LK4D4 · 2016-05-23T18:31:23Z

@dnephin it actually maps to other cli stuff in linux. But I dunno, maybe we should ask @cyphar

dnephin · 2016-05-23T18:38:09Z

@LK4D4 really? What other CLI removes quotes from the middle of an argument?

Normally the shell strips them off for you, but I don't think the application does anything with them.

LK4D4 · 2016-05-23T18:57:35Z

@dnephin yeah, maybe it's shell

thaJeztah · 2016-05-23T19:04:11Z

I had a quick look at some features cobra offers, and it looks pretty neat. We may want to look into the auto-generation of man-pages and completion scripts if this is accepted, so "+1" from me

cpuguy83 · 2016-05-23T20:33:01Z

Design is OK for me.

LK4D4 · 2016-05-23T20:59:35Z

@dnephin I think it's okay to start to work on rebase and CI issues.

cyphar · 2016-05-24T05:26:38Z

@dnephin @LK4D4 Yeah, I agree we can remove the quote trimming code. I'm not actually sure why I added it, but shells should be doing the trimming for us. :P

thaJeztah · 2016-05-26T14:19:16Z

ping @docker/core-engine-maintainers PTAL

Signed-off-by: Daniel Nephin <[email protected]>

duglin · 2016-05-31T21:46:08Z

@dnephin I gotta ask about something really odd with the old cli. If someone typed:

docker run -wl /tmp foo=bar ubuntu pwd

it would do some really weird parsing and connect the /tmp with -w and then foo=bar with -l.
Did you have to do something special to keep that odd parsing or does cobra automatically do that weird thing too?

btw - I say its weird because normally I don't think parsers do that. For example:
ps -C ps -O pid,state
is not the same as
ps -CO ps pid,state

thaJeztah · 2016-05-31T21:49:48Z

@GordonTheTurtle died, so stay tuned while we're figuring that out, so don't merge before it's back

dnephin · 2016-05-31T21:49:55Z

@thaJeztah just pushed a commit that marks -h as deprecated, so it's removed from the usage text, and it even prints "Flag shorthand -h has been deprecated, please use --help" if someone tries to use it.

thaJeztah · 2016-05-31T21:50:18Z

@dnephin thanks! I'll give it a spin locally

calavera · 2016-05-31T21:52:04Z

@duglin I think that behavior comes from the pflags library, and they should be compatible, but I haven't tested it.

dnephin · 2016-05-31T21:52:39Z

@duglin hmm, I haven't encountered that yet, and I don't know if that works with cobra and pflags. Note this only converts the docker volume command for now, so I haven't tackled docker run yet.

Let me experiment and see how that works.

duglin · 2016-05-31T21:54:35Z

ah that would explain why it still parses in that weird way :-)

duglin · 2016-05-31T21:55:46Z

actually, if this just moves over "volume" I'd prefer if we did some other command (like "run") too because if we encounter something really nasty that would prevent us from migrating all commands then I'm not sure we should move over just some of them.

LK4D4 · 2016-05-31T22:01:27Z

I agree with @duglin . Run and would be great tests to see what can be broken.

dnephin · 2016-05-31T22:05:32Z

run is going to be the largest piece of work. I've done some experimentation and haven't hit any major problems, but not everything is immediately obvious (like the parsing behaviour you described).

One known "limitation" (which is really by design) is that the very old flags that use single dash but multi character (-foo) are not supported. However maybe those have already been removed? I can't seem to find an example of that now, and I know they were deprecated a very long time ago.

I can start working on a PR to convert run, but it would be nice to keep it as a separate PR as this is already quite large.

I don't think we'll encounter any serious problems, we can always patch cobra/pflags to do what we need them to do.

thaJeztah · 2016-05-31T22:10:20Z

@dnephin I think all single-dash flags have been removed https://github.com/docker/docker/blob/master/docs/deprecated.md#old-command-line-options

dnephin · 2016-05-31T22:25:48Z

I'm going to create a separate PR to show docker run using cobra as a PoC. I'm not sure how many tests will break because of the single quote usage, but it should at least give us a good idea of what issues we can expect.

- adds support for usage strings on flag errors - adds support for arg validation Signed-off-by: Daniel Nephin <[email protected]>

Signed-off-by: Daniel Nephin <[email protected]>

Also re-use context. Signed-off-by: Daniel Nephin <[email protected]>

Signed-off-by: Daniel Nephin <[email protected]>

tiborvass · 2016-06-01T00:14:50Z

@duglin I think the weird behavior of docker run -wl /tmp foo=bar ubuntu pwd qualifies as a bug. We should error out because you're not supposed to merge non-boolean short flags.

LK4D4 · 2016-06-01T21:18:19Z

LGTM
Let's test run in other PR

dnephin added the area/cli Client label May 16, 2016

GordonTheTurtle added the status/0-triage label May 16, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels May 16, 2016

dnephin force-pushed the integrate_cobra branch from b44ef11 to 8101266 Compare May 16, 2016 19:33

aaronlehmann reviewed May 16, 2016
View reviewed changes

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

dnephin force-pushed the integrate_cobra branch from 8101266 to 49af78f Compare May 20, 2016 22:00

LK4D4 added status/2-code-review and removed status/1-design-review labels May 23, 2016

thaJeztah mentioned this pull request May 24, 2016

Feature/betterment of people's lives: Generated docs #22931

Closed

dnephin force-pushed the integrate_cobra branch from 49af78f to 125a525 Compare May 24, 2016 22:40

justincormack mentioned this pull request May 25, 2016

deprecate '-h' shorthand and remove '-help' #22980

Merged

dnephin force-pushed the integrate_cobra branch 2 times, most recently from 5c6ddac to 7f5732c Compare May 25, 2016 22:45

dnephin changed the title ~~PoC: use spf13/cobra for the cli~~ Use spf13/cobra for the cli May 26, 2016

thaJeztah mentioned this pull request May 26, 2016

Always unset the value for daemon reload for predictable behaviors #22532

Closed

dnephin added 3 commits May 31, 2016 14:41

Refactor cli inspector to support new inspects.

6bc3e23

Signed-off-by: Daniel Nephin <[email protected]>

Migrate volume commands to cobra.

69264be

Signed-off-by: Daniel Nephin <[email protected]>

Update usage and help to (almost) match the existing docker behaviour

667dcb0

Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the integrate_cobra branch from 4e087a9 to c8136e6 Compare May 31, 2016 21:48

dnephin added 4 commits May 31, 2016 15:43

Update cobra vendor

3d624ed

- adds support for usage strings on flag errors - adds support for arg validation Signed-off-by: Daniel Nephin <[email protected]>

Support usage messages on bad flags.

54b5cce

Signed-off-by: Daniel Nephin <[email protected]>

Use Args in cobra.Command to validate args.

fc5a451

Also re-use context. Signed-off-by: Daniel Nephin <[email protected]>

Make the -h flag deprecated.

9b2bb64

Signed-off-by: Daniel Nephin <[email protected]>

dnephin force-pushed the integrate_cobra branch from c8136e6 to 9b2bb64 Compare May 31, 2016 22:45

dnephin mentioned this pull request Jun 1, 2016

Use spf13/cobra for docker run and docker create #23159

Closed

thaJeztah added this to the 1.12.0 milestone Jun 1, 2016

LK4D4 merged commit 28676fc into moby:master Jun 1, 2016

dnephin deleted the integrate_cobra branch June 1, 2016 21:19

dnephin mentioned this pull request Jun 2, 2016

Convert commands to spf13/cobra #23211

Closed

43 tasks

vdemeester mentioned this pull request Jun 4, 2016

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

Merged

thaJeztah mentioned this pull request Dec 28, 2016

docker 1.13.0 CLI seems to break previous flag parsing behavior #29761

Closed

Conversation

dnephin commented May 16, 2016

Uh oh!

aaronlehmann May 16, 2016

Choose a reason for hiding this comment

Uh oh!

dnephin May 17, 2016

Choose a reason for hiding this comment

Uh oh!

cpuguy83 commented May 17, 2016

Uh oh!

MHBauer commented May 19, 2016

Uh oh!

cpuguy83 commented May 19, 2016

Uh oh!

dnephin commented May 20, 2016

Uh oh!

LK4D4 commented May 23, 2016

Uh oh!

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

Uh oh!

Uh oh!

LK4D4 commented May 23, 2016

Uh oh!

thaJeztah commented May 23, 2016

Uh oh!

cpuguy83 commented May 23, 2016

Uh oh!

LK4D4 commented May 23, 2016

Uh oh!

cyphar commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented May 26, 2016

Uh oh!

duglin commented May 31, 2016

Uh oh!

thaJeztah commented May 31, 2016

Uh oh!

dnephin commented May 31, 2016

Uh oh!

thaJeztah commented May 31, 2016

Uh oh!

calavera commented May 31, 2016

Uh oh!

dnephin commented May 31, 2016

Uh oh!

duglin commented May 31, 2016

Uh oh!

duglin commented May 31, 2016

Uh oh!

LK4D4 commented May 31, 2016

Uh oh!

dnephin commented May 31, 2016

Uh oh!

thaJeztah commented May 31, 2016

Uh oh!

dnephin commented May 31, 2016

Uh oh!

tiborvass commented Jun 1, 2016

Uh oh!

LK4D4 commented Jun 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

dnephin commented May 23, 2016 •

edited

Loading

cyphar commented May 24, 2016 •

edited

Loading