```
e2e/cli-plugins/flags_test.go:135:27: Using the variable on range scope `args` in function literal (scopelint)
res := icmd.RunCmd(run(args...))
^
```
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
exec.CombinedOutput should not be used here because:
- it redirects cmd Stdout and Stderr and we want it to be the tty
- it calls cmd.Run which we already did
While at it
- use pty.Start() as it is cleaner
- make sure we don't leave a zombie running, by calling Wait() in defer
- use test.Name() for containerName
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The docker-in-docker image now enables TLS by default (added in
docker-library/docker#166), which complicates testing in our
environment, and isn't needed for the tests we're running.
This patch sets the `DOCKER_TLS_CERTDIR` to an empty value to
disable TLS.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Add a test to verify that killing the docker CLI forwards
the signal to the container. Test-case for moby/moby 28872
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
I'm about to refactor the code which includes the Kubernetes support in a way
which relies on something vendoring `./cli/context/kubernetes/` in order to
trigger the inclusion of support for the Kubernetes endpoint in the final
binary.
In practice anything which is interested in Kubernetes must import that package
(e.g. `./cli/command/context.list` does for the `EndpointFromContext`
function). However if it was somehow possible to build without that import then
the `KUBERNETES ENDPOINT` column would be mysteriously empty.
Out of an abundance of caution add a specific check on the final binary.
Signed-off-by: Ian Campbell <ijc@docker.com>
This regressed in 3af168c7df ("Ensure plugins can use `PersistentPreRunE`
again.") but this wasn't noticed because the helloworld test plugin has it's
own `PersistentPreRunE` which has the effect of deferring the resolution of the
global variable. In the case where the hook isn't used the variable is resolved
during `newPluginCommand` which is before the global variable was set.
Initialize the plugin command with a stub function wrapping the call to the
(global) hook, this defers resolving the variable until after it has been set,
otherwise the initial value (`nil`) is used in the struct.
Signed-off-by: Ian Campbell <ijc@docker.com>
This was omitted when these tests were added.
Adding this means that the tests now see the `$DOCKER_HOST` configured (via
`$TEST_DOCKER_HOST`) where they didn't before. In some cases (specifically the
`test-e2e-connhelper-ssh` case) this results in additional output on stderr
when `-D` (debug) is used:
time="2019-04-03T11:10:27Z" level=debug msg="commandconn: starting ssh with [-l penguin 172.20.0.2 -- docker system dial-stdio]"
Address this by switching the affected test cases to use `-l info` instead of
`-D`, they all just require some option not specifically `-D`. Note that `info`
is the default log level so this is effectively a nop (which is good).
Signed-off-by: Ian Campbell <ijc@docker.com>
In the initial implementation I thought it would be good to not pass on the
deprecation to plugins (since they are new). However it turns out this causes
`docker helloworld -h` to print a spurious "pflag: help requested" line:
$ docker helloworld -h
pflag: help requested
See 'docker helloworld --help'.
Usage: docker helloworld [OPTIONS] COMMAND
A basic Hello World plugin for tests
...
Compared with:
$ docker ps -h
Flag shorthand -h has been deprecated, please use --help
Usage: docker ps [OPTIONS]
This is in essence because having the flag undefined hits a different path
within cobra, causing `c.execute()` to return early due to getting an error
(`flag.ErrHelp`) from `c.ParseFlags`, which launders the error through our
`FlagErrorFunc` which wraps it in a `StatusError` which in turn defeats an `if
err == flag.ErrHelp` check further up the call chain. If the flag is defined we
instead hit a path which returns a bare `flag.ErrHelp` without wrapping it.
I considered updating our `FlagErrorFunc` to not wrap `flag.ErrHelp` (and then
following the chain to the next thing) however while doing that I realised that
the code for `-h` (and `--help`) is deeply embedded into cobra (and its flags
library) such that actually using `-h` as a plugin argument meaning something
other than `help` is basically impossible/impractical. Therefore we may as well
have plugins behave identically to the monolithic CLI and support (deprecated)
the `-h` argument.
With this changed the help related blocks of `SetupRootCommand` and
`SetupPluginRootCommand` are now identical, so consolidate into
`setupCommonRootCommand`.
Tests are updated to check `-h` in a variety of scenarios, including the happy
case here.
Signed-off-by: Ian Campbell <ijc@docker.com>
In some cases this means switching to `icmd.Expected` rather than
`icmd.Success`, but this improves readability overall.
Signed-off-by: Ian Campbell <ijc@docker.com>
Since #1654 so far we've had problems with it not working on Windows (npipe
lacked the `CloseRead` method) and problems with using tcp with tls (the tls
connection also lacks `CloseRead`). Both of these were workedaround in #1718
which added a nop `CloseRead` method.
However I am now seeing hangs (on Windows) where the `system dial-stdio`
subprocess is not exiting (I'm unsure why so far).
I think the 3rd problem found with this is an indication that `dial-stdio` is
not quite ready for wider use outside of its initial usecase (support for
`ssh://` URLs to connect to remote daemons).
This change simply disables the `dial-stdio` path for all plugins. However
rather than completely reverting 891b3d953e ("cli-plugins: use `docker system
dial-stdio` to call the daemon") I've just disabled the functionality at the
point of use and left in a trap door environment variable so that those who
want to experiment with this mode (and perhaps fully debug it) have an easier
path do doing so.
The e2e test for this case is disabled unless the trap door envvar is set. I
also renamed the test to clarify that it is about cli plugins.
Signed-off-by: Ian Campbell <ijc@docker.com>
I regressed this in d4ced2ef77 ("allow plugins to have argument which match a
top-level flag.") by unconditionally overwriting any `PersistentRunE` that the
user may have supplied.
We need to ensure two things:
1. That the user can use `PersistentRunE` (or `PersistentRun`) for their own
purposes.
2. That our initialisation always runs, even if the user has used
`PersistentRun*`, since that will shadow the root.
To do this add a `PersistentRunE` to the helloworld plugin which logs (covers 1
above) and then use it when calling the `apiversion` subcommand (which covers 2
since that uses the client)
Signed-off-by: Ian Campbell <ijc@docker.com>
Previous commits fixed the first issue on #1661, this simply adds a test for
it. Note that this is testing the current behaviour, without regard for the
second issue in #1661 which proposes a different behaviour.
Signed-off-by: Ian Campbell <ijc@docker.com>
The issue with plugin options clashing with globals is that when cobra is
parsing the command line and it comes across an argument which doesn't start
with a `-` it (in the absence of plugins) distinguishes between "argument to
current command" and "new subcommand" based on the list of registered sub
commands.
Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra
parses up to the `plugin`, sees it isn't a registered sub-command of the
top-level docker (because it isn't, it's a plugin) so it accumulates it as an
argument to the top-level `docker` command. Then it sees the `-c`, and thinks
it is the global `-c` (for AKA `--context`) option and tries to treat it as
that, which fails.
In the specific case of the top-level `docker` subcommand we know that it has
no arguments which aren't `--flags` (or `-f` short flags) and so anything which
doesn't start with a `-` must either be a (known) subcommand or an attempt to
execute a plugin.
We could simply scan for and register all installed plugins at start of day, so
that cobra can do the right thing, but we want to avoid that since it would
involve executing each plugin to fetch the metadata, even if the command wasn't
going to end up hitting a plugin.
Instead we can parse the initial set of global arguments separately before
hitting the main cobra `Execute` path, which works here exactly because we know
that the top-level has no non-flag arguments.
One slight wrinkle is that the top-level `PersistentPreRunE` is no longer
called on the plugins path (since it no longer goes via `Execute`), so we
arrange for the initialisation done there (which has to be done after global
flags are parsed to handle e.g. `--config`) to happen explictly after the
global flags are parsed. Rather than make `newDockerCommand` return the
complicated set of results needed to make this happen, instead return a closure
which achieves this.
The new functionality is introduced via a common `TopLevelCommand` abstraction
which lets us adjust the plugin entrypoint to use the same strategy for parsing
the global arguments. This isn't strictly required (in this case the stuff in
cobra's `Execute` works fine) but doing it this way avoids the possibility of
subtle differences in behaviour.
Fixes#1699, and also, as a side-effect, the first item in #1661.
Signed-off-by: Ian Campbell <ijc@docker.com>
These won't pass right now due to https://github.com/docker/cli/issues/1699
("Plugins can't re-use the same flags as cli global flags") and the change in
935d47bbe9 ("Ignore unknown arguments on the top-level command."), but the
intention is to fix them now.
Signed-off-by: Ian Campbell <ijc@docker.com>
... in preference to `chsh`, since in recent alpine 3.9.2 images that can fail
with:
Password: chsh: PAM: Authentication token manipulation error
Which seems to relate to the use of `!` as the password for `root` in `/etc/shadow`gq
Signed-off-by: Ian Campbell <ijc@docker.com>
By default, exec uses the environment of the current process, however,
if `exec.Env` is not `nil`, the environment is discarded:
e73f489494/src/os/exec/exec.go (L57-L60)
> If Env is nil, the new process uses the current process's environment.
When adding a new environment variable, prepend the current environment,
to make sure it is not discarded.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Plugins are expected to be management commands ("docker <object> <verb>").
This patch modified the usage output to shown plugins in the "Management commands"
section.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- The placement of the vendor is now in the end of the line.
- A '*' is now added as suffix of plugins' top level commands.
Signed-off-by: Ulysses Souza <ulysses.souza@docker.com>
This means that plugins can use whatever methods the monolithic CLI supports,
which is good for consistency.
This relies on `os.Args[0]` being something which can be executed again to
reach the same binary, since it is propagated (via an envvar) to the plugin for
this purpose. This essentially requires that the current working directory and
path are not modified by the monolithic CLI before it launches the plugin nor
by the plugin before it initializes the client. This should be the case.
Previously the fake apiclient used by `TestExperimentalCLI` was not being used,
since `cli.Initialize` was unconditionally overwriting it with a real one
(talking to a real daemon during unit testing, it seems). This wasn't expected
nor desirable and no longer happens with the new arrangements, exposing the
fact that no `pingFunc` is provided, leading to a panic. Add a `pingFunc` to
the fake client to avoid this.
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>
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>
Previously a plugin which used these hooks would overwrite the top-level plugin
command's use of this hook, resulting in the dockerCli object not being fully
initialised.
Provide a function which plugins can use to chain to the required behaviour.
This required some fairly ugly arrangements to preserve state (which was
previously in-scope in `newPluginCOmmand`) to be used by the new function.
Signed-off-by: Ian Campbell <ijc@docker.com>
To achieve this we hook in at the beginning of our custom `HelpFunc` and detect
the plugin case by adding stub commands.
Signed-off-by: Ian Campbell <ijc@docker.com>
To do this we add a stub `cobra.Command` for each installed plugin (only when
invoking `help`, not for normal running).
This requires a function to list all available plugins so that is added here.
Signed-off-by: Ian Campbell <ijc@docker.com>
Sebastiaan van Stijn
Silvin Lubecki
Kir Kolyshkin
Ian Campbell
Sebastiaan van Stijn
Silvin Lubecki
Jean-Christophe Sirot
Akihiro Suda
Corey Quon
Ulysses Souza