Basic framework for writing and running CLI plugins #1564

ijc · 2018-12-12T11:15:03Z

- What I did

This is a small part of #1534, I wanted to get the basic framework out so that potential consumers could start to play with it and give feedback on how it works for them as well as getting some early feedback on the code.

NB this also includes the changes for #1558 since I was working on top of that.

There is still lots to do:

I've left off the docker cli-plugin * stuff since it is still being discussed (although some bits of the internal scaffolding here for listing and finding plugins are more related to that than the actual functionality added here, but I didn't tease those apart). Not doing this any more, at least not in this round (CLI Plugins Design #1534 (comment)).
List broken plugins as a new section in docker help.
There are no docs;
Although there are some here there are insufficient tests (both unit & e2e);
docker someplugin --help produces the top-level help instead of calling the plugin for some reason (this means one of the new e2e tests currently fails), looks like docker someplugin --version has the same problem, I'm investigating.
Expose plugins in docker info
Plugin search path configurable via ~/.docker/config.json
Apply the package naming the maintainers decide upon.
All the stuff around exposing configuration files, credential handling, table formatting, using dial-stdio etc isn't here (Some of those are nops in practice and just need checking that they are usable externally, otherwise I plan to do these bits in one or more followups, somewhat based on/prioritise by the experiences of folks trying to use this). edit: to clarify, I don't intend on doing this bit here but rather in a followup.

- How to verify it

The unit and e2e tests cover some of it.

Plugin path can be configured with the "cliPluginsExtraDirs" key in ~/.docker/config.json so for having a play (using a local config dir for isolation) you can also make -f docker.Makefile build plugins then things like:

$ mkdir .docker-plugin-test
$ echo "{\"cliPluginsExtraDirs\": [\"`pwd`/build/plugins-linux-amd64/\"]}" > .docker-plugin-test/config.json
$ $ jq . .docker-plugin-test/config.json
{
  "cliPluginsExtraDirs": [
    ".../gocode/src/github.com/docker/cli/build/plugins-linux-amd64"
  ]
}
$ ./build/docker --config=.docker-plugin-test helloworld

to have a play.

- Description for the changelog

Basic support for writing and running external CLI commands

ijc · 2018-12-12T11:27:52Z

I was expecting e2e to fail but looks like I forgot to shellcheck and lint too, will sort those out.

codecov-io · 2018-12-12T15:03:33Z

Codecov Report

Merging #1564 into master will decrease coverage by 0.06%.
The diff coverage is 51.52%.

@@            Coverage Diff             @@
##           master    #1564      +/-   ##
==========================================
- Coverage    56.1%   56.04%   -0.07%     
==========================================
  Files         300      306       +6     
  Lines       20601    20848     +247     
==========================================
+ Hits        11559    11685     +126     
- Misses       8209     8324     +115     
- Partials      833      839       +6

vdemeester

Couple of comment/questions:

why not github.com/docker/cli/plugins as a package name ?
could additionnal lookup path be available through the user configuration (aka $HOME/.docker/config.json) ?

cli-plugins/manager/candidate_test.go

cli-plugins/manager/plugin.go

cmd/docker/docker.go

guillaumerose · 2018-12-13T11:01:26Z

Just a comment related to the Docker Desktop integration:
Docker Desktop doesn't change env variables, it's too hard (multiple shells possible, etc.). Would it be possible to also have a parameter representing DOCKER_CLI_PLUGIN_EXTRA_DIRS in ~/.docker/config.json ?

ijc · 2018-12-13T12:33:02Z

@vdemeester

why not github.com/docker/cli/plugins as a package name ?

To avoid confusion with docker plugins which is about daemon side plugins. Happy to take suggestions for better names though.

could additionnal lookup path be available through the user configuration (aka $HOME/.docker/config.json) ?

Sure.

@guillaumerose WRT that setting and Desktop, we should try hard to avoid needing Desktop to set anything like this (whether in a file or the env) by making sure the default set of paths is sensible for the Desktop OSes such that desktop can install the base set of plugins in the right place without any special overrides.

vdemeester · 2018-12-13T13:13:36Z

To avoid confusion with docker plugins which is about daemon side plugins. Happy to take suggestions for better names though.

@ijc the docker plugins "code" is in github.com/docker/cli/cli/command/plugin, so I don't really think there would be any confusion (especially if we add a README.md in github.com/docker/cli/plugins to make it clear). cc @thaJeztah for thoughs

cli-plugins/examples/helloworld/main.go

docker/cli#1564 Since this pushes the actual command down to `docker-app app`, adjust the e2e tests with: $ sed -Eie 's/icmd\.RunCommand\(dockerApp,/icmd.RunCommand(dockerApp, "app",/g' e2e/*.go $ sed -Eie 's/exec.Command\(dockerApp,/exec.Command(dockerApp, "app",/g' e2e/*.go $ sed -Eie 's/\[\]string\{dockerApp,/\[\]string\{dockerApp, "app",/g' e2e/*.go (which might be precisely equivalent to `sed -Eie 's/dockerApp,/dockerApp, "app",/g' e2e/*.go`) Finally, the idiom in docker/cli (for better or worse) is to include an "Error: " prefix in the error string itself, rather than injecting it when printing. Since CLI plugins follow the behaviour of the CLI here it is necessary to prepend "Error: " to some messages. I've only done exactly those necessary to pass the e2e tests, a fuller audit is very likely required. Signed-off-by: Ian Campbell <ijc@docker.com>

ijc · 2018-12-13T16:52:41Z

I did a quick port of docker app to use the framework, mostly to serve as an example of how this might be used: docker/app#433.

cli-plugins/plugin/plugin.go

e2e/cli-plugins/run_test.go

ijc · 2018-12-18T11:31:23Z

I've force pushed quite a big update, rebasing on the latest code (so #1558 is no longer included since it is merged), sorted out the issues around --help, tidied and refactored a bunch of stuff, etc.

I'm going to update the todo in the first comment to reflect what's done and add some new entries people requested.

docker/cli#1564 Since this pushes the actual command down to `docker-app app`, adjust the e2e tests with: $ sed -Eie 's/icmd\.RunCommand\(dockerApp,/icmd.RunCommand(dockerApp, "app",/g' e2e/*.go $ sed -Eie 's/exec.Command\(dockerApp,/exec.Command(dockerApp, "app",/g' e2e/*.go $ sed -Eie 's/\[\]string\{dockerApp,/\[\]string\{dockerApp, "app",/g' e2e/*.go (which might be precisely equivalent to `sed -Eie 's/dockerApp,/dockerApp, "app",/g' e2e/*.go`) Finally, the idiom in docker/cli (for better or worse) is to include an "Error: " prefix in the error string itself, rather than injecting it when printing. Since CLI plugins follow the behaviour of the CLI here it is necessary to prepend "Error: " to some messages. I've only done exactly those necessary to pass the e2e tests, a fuller audit is very likely required. Signed-off-by: Ian Campbell <ijc@docker.com>

Makefile

cli-plugins/examples/badmeta/main.go

Signed-off-by: Ian Campbell <ijc@docker.com>

On Windows `foo.exe`, `foo.eXe` and `foo.EXE` are equally executable. Signed-off-by: Ian Campbell <ijc@docker.com>

This allows passing argument to plugins, otherwise they are caught by the parse loop, since cobra does not know about each plugin at this stage (to avoid having to always scan for all plugins) this means that e.g. `docker plugin --foo` would accumulate `plugin` as an arg to the `docker` command, then choke on the unknown `--foo`. This allows unknown global args only, unknown arguments on subcommands (e.g. `docker ps --foo`) are still correctly caught. Add an e2e test covering this case. Signed-off-by: Ian Campbell <ijc@docker.com>

A static global initialiser happens before the arguments are parsed, so we need to calculate the path later. Signed-off-by: Ian Campbell <ijc@docker.com>

They were listed twice in `docker --help` (but not `docker help`), since the stubs were added in both `tryRunPluginHelp` and the `setHelpFunc` closure. Calling `AddPluginStubCommands` earlier in `setHelpFunc` before the call to `tryRunPluginHelp` is sufficient. Also it is no longer necessary to add just valid plugins (`tryRunPluginHelp` handles invalid plugins correctly) so remove that logic (which was in any case broken for e.g. `docker --help`). Update the e2e test to check for duplicate entries and also to test `docker --help` which was previously missed. Signed-off-by: Ian Campbell <ijc@docker.com>

thaJeztah · 2019-01-30T14:32:18Z

Summarizing things that I think can be handled in follow-ups (but some of these should be done before GA);

Basic framework for writing and running CLI plugins #1564 (comment) handling of docker someplugin --version. The --version / -v flag is handled by the plugin root-command, and therefore shows the docker version information (Docker version 19.03.0-dev, build c8f1f1ee). We should fix this to be handled by the plugin.
Related to the above; consider making --version / -v part of the pluging "API", so that a plugin can advertise its version (both when calling the plugin directly (docker-myplugin --version), and when calling through the docker cli (docker myplugin --version). Perhaps we should add a standard function and/or define a standard format (docker-myplugin version X, commit deadbeef (ACME, Inc.))
Basic framework for writing and running CLI plugins #1564 (comment) install location for plugins; choose between ~/.docker/plugins or ~/.docker/cli-plugins
Basic framework for writing and running CLI plugins #1564 (comment) decide on the output format in docker --help: make plugin-command stand out from built-in commands, or make them appear as regular commands with a non-obtrusive indicator (*)
Basic framework for writing and running CLI plugins #1564 (comment) decide on the location to show version and vendor for plugins in in docker info
Basic framework for writing and running CLI plugins #1564 (comment) small nit: use displayutils.Ellipsis() instead of a custom implementation.
Basic framework for writing and running CLI plugins #1564 (comment) small nit: hardening config.Path() to disallow directory traversal
Basic framework for writing and running CLI plugins #1564 (comment) small nit: fix usage output of the sample plugin

Future follow-ups (not discussed here);

Extend the plugin framework to allow generating YAML-docs (https://github.com/docker/cli/tree/master/docs/yaml)
Extend the plugin framework to allow generating man-pages (https://github.com/docker/cli/tree/master/man)
Provide guidelines for bash-completion scripts (perhaps generated for now), and work out how we can integrate those with the main bash-completion scripts, while remaining separate.

thaJeztah · 2019-01-30T14:34:31Z

@docker/core-cli-maintainers looking at the list above, I'm inclined to move this forward, and take care of the listed issues separate; WDYT?

silvin-lubecki · 2019-01-30T23:45:32Z

@thaJeztah I'm ok with merging this first plugin base and iterate with follow-ups on the issues you listed 👍

vdemeester · 2019-01-31T07:38:58Z

Same as @silvin-lubecki given we do those follow-up before next release 👼

ijc · 2019-01-31T10:49:56Z

This WFM (of course) and I will commit to addressing the bullets in #1564 next (my first act would be to transliterate that comment into an issue I think).

@thaJeztah I've looked over the outstanding comments and I think your list covers them all. Have I missed anything which you don't want to defer?

thaJeztah

LGTM

silvin-lubecki

LGTM!!! Thank you @ijc for this awesome work!

ijc · 2019-02-01T10:28:48Z

@albers do you have any advice on how we might go about integrating completion for plugins? I was wondering perhaps if contrib/completion/bash/docker could maybe iterate over some set of plugin_completions.d where plugins could drop their snippets?

WDYT?

albers · 2019-03-08T19:58:58Z

do you have any advice on how we might go about integrating completion for plugins?

@ijc I think this is possible in the way you outlined.
I'll give it a try.

ijc · 2019-03-11T11:40:56Z

Awesome, thanks @albers! Please give a shout if there is any way I can help

ijc requested review from thaJeztah and vdemeester as code owners December 12, 2018 11:15

GordonTheTurtle added the status/0-triage label Dec 12, 2018

ijc force-pushed the plugins branch from 6bcf51b to 461332d Compare December 12, 2018 15:03

vdemeester reviewed Dec 13, 2018

View reviewed changes

cli-plugins/manager/candidate_test.go Outdated Show resolved Hide resolved

cli-plugins/manager/plugin.go Outdated Show resolved Hide resolved

cmd/docker/docker.go Outdated Show resolved Hide resolved

ijc force-pushed the plugins branch from 461332d to 21dfe81 Compare December 13, 2018 13:36

ijc commented Dec 13, 2018

View reviewed changes

cli-plugins/examples/helloworld/main.go Outdated Show resolved Hide resolved

ijc force-pushed the plugins branch 2 times, most recently from 3aeb78c to 8858366 Compare December 13, 2018 16:06

ijc mentioned this pull request Dec 13, 2018

WIP: refactor as a CLI plugin docker/app#433

Closed

cquon reviewed Dec 14, 2018

View reviewed changes

cli-plugins/plugin/plugin.go Outdated Show resolved Hide resolved

ijc commented Dec 14, 2018

View reviewed changes

cli-plugins/plugin/plugin.go Outdated Show resolved Hide resolved

ijc commented Dec 14, 2018

View reviewed changes

e2e/cli-plugins/run_test.go Outdated Show resolved Hide resolved

ijc force-pushed the plugins branch 3 times, most recently from 5a7f245 to bebdb71 Compare December 18, 2018 11:29

silvin-lubecki reviewed Dec 18, 2018

View reviewed changes

Makefile Outdated Show resolved Hide resolved

silvin-lubecki reviewed Dec 18, 2018

View reviewed changes

cli-plugins/examples/badmeta/main.go Outdated Show resolved Hide resolved

Ian Campbell added 4 commits January 30, 2019 13:45

Refactor code which deals with Windows' .exe suffix

63f3ad1

Signed-off-by: Ian Campbell <ijc@docker.com>

Check for .exe case insensitively

1337895

On Windows `foo.exe`, `foo.eXe` and `foo.EXE` are equally executable. Signed-off-by: Ian Campbell <ijc@docker.com>

Ensure plugins default search path obeys --config

0a89eb5

A static global initialiser happens before the arguments are parsed, so we need to calculate the path later. Signed-off-by: Ian Campbell <ijc@docker.com>

ijc force-pushed the plugins branch from f78032f to 1c92e28 Compare January 30, 2019 13:45

ijc force-pushed the plugins branch from 1c92e28 to baabf6e Compare January 30, 2019 13:55

thaJeztah added the impact/changelog label Jan 30, 2019

thaJeztah added this to the 19.03.0 milestone Jan 30, 2019

thaJeztah approved these changes Jan 31, 2019

View reviewed changes

silvin-lubecki approved these changes Jan 31, 2019

View reviewed changes

silvin-lubecki merged commit 2e5639d into docker:master Jan 31, 2019

ijc deleted the plugins branch January 31, 2019 17:19

ijc mentioned this pull request Feb 1, 2019

Add a field to the config file for plugin use. #1652

Merged

This was referenced Feb 1, 2019

cli-plugins: use system dial-stdio to contact the engine. #1654

Merged

Followup to #1564 (cli plugins) #1661

Open

ijc mentioned this pull request Feb 18, 2019

Should cli plugins install to ~/.docker/plugins #1677

Closed

thaJeztah added the area/plugins label Feb 21, 2019

tonistiigi mentioned this pull request Mar 5, 2019

Drop experimental tag for manifest commands #1355

Closed

zappy-shu mentioned this pull request Mar 6, 2019

hardening config.Path() to disallow directory traversal #1720

Merged

andrewhsu mentioned this pull request May 21, 2019

docker cli experimental flag for plugins #1897

Closed

tianon mentioned this pull request Mar 22, 2023

Prefer loading docker-init from an appropriate "libexec" directory moby/moby#45198

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic framework for writing and running CLI plugins #1564

Basic framework for writing and running CLI plugins #1564

ijc commented Dec 12, 2018 •

edited

Loading

ijc commented Dec 12, 2018

codecov-io commented Dec 12, 2018 •

edited

Loading

vdemeester left a comment

guillaumerose commented Dec 13, 2018

ijc commented Dec 13, 2018

vdemeester commented Dec 13, 2018

ijc commented Dec 13, 2018

ijc commented Dec 18, 2018

thaJeztah commented Jan 30, 2019

thaJeztah commented Jan 30, 2019

silvin-lubecki commented Jan 30, 2019

vdemeester commented Jan 31, 2019

ijc commented Jan 31, 2019

thaJeztah left a comment

silvin-lubecki left a comment

ijc commented Feb 1, 2019

albers commented Mar 8, 2019

ijc commented Mar 11, 2019

Basic framework for writing and running CLI plugins #1564

Basic framework for writing and running CLI plugins #1564

Conversation

ijc commented Dec 12, 2018 • edited Loading

ijc commented Dec 12, 2018

codecov-io commented Dec 12, 2018 • edited Loading

Codecov Report

vdemeester left a comment

Choose a reason for hiding this comment

guillaumerose commented Dec 13, 2018

ijc commented Dec 13, 2018

vdemeester commented Dec 13, 2018

ijc commented Dec 13, 2018

ijc commented Dec 18, 2018

thaJeztah commented Jan 30, 2019

thaJeztah commented Jan 30, 2019

silvin-lubecki commented Jan 30, 2019

vdemeester commented Jan 31, 2019

ijc commented Jan 31, 2019

thaJeztah left a comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

ijc commented Feb 1, 2019

albers commented Mar 8, 2019

ijc commented Mar 11, 2019

ijc commented Dec 12, 2018 •

edited

Loading

codecov-io commented Dec 12, 2018 •

edited

Loading