Skip to content

Commit

Permalink
Ensure that plugins are only listed once in help outputs.
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
Ian Campbell committed Jan 30, 2019
1 parent c8f1f1e commit f78032f
Show file tree
Hide file tree
Showing 3 changed files with 47 additions and 24 deletions.
11 changes: 4 additions & 7 deletions cli-plugins/manager/cobra.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,15 @@ const (
CommandAnnotationPluginInvalid = "com.docker.cli.plugin-invalid"
)

// AddPluginCommandStubs adds a stub cobra.Commands for each plugin
// (optionally including invalid ones). The command stubs will have
// several annotations added, see `CommandAnnotationPlugin*`.
func AddPluginCommandStubs(dockerCli command.Cli, cmd *cobra.Command, includeInvalid bool) error {
// // AddPluginCommandStubs adds a stub cobra.Commands for each valid and invalid
// plugin. The command stubs will have several annotations added, see
// `CommandAnnotationPlugin*`.
func AddPluginCommandStubs(dockerCli command.Cli, cmd *cobra.Command) error {
plugins, err := ListPlugins(dockerCli, cmd)
if err != nil {
return err
}
for _, p := range plugins {
if !includeInvalid && p.Err != nil {
continue
}
vendor := p.Vendor
if vendor == "" {
vendor = "unknown"
Expand Down
22 changes: 11 additions & 11 deletions cmd/docker/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,6 @@ func setupHelpCommand(dockerCli *command.DockerCli, rootCmd, helpCmd *cobra.Comm

func tryRunPluginHelp(dockerCli command.Cli, ccmd *cobra.Command, cargs []string) error {
root := ccmd.Root()
pluginmanager.AddPluginCommandStubs(dockerCli, root, false)

cmd, _, err := root.Traverse(cargs)
if err != nil {
Expand All @@ -169,6 +168,17 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.
ccmd.Println(err)
return
}

// Add a stub entry for every plugin so they are
// included in the help output and so that
// `tryRunPluginHelp` can find them or if we fall
// through they will be included in the default help
// output.
if err := pluginmanager.AddPluginCommandStubs(dockerCli, ccmd.Root()); err != nil {
ccmd.Println(err)
return
}

if len(args) >= 1 {
err := tryRunPluginHelp(dockerCli, ccmd, args)
if err == nil { // Successfully ran the plugin
Expand All @@ -189,16 +199,6 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.
return
}

// Add a stub entry for every plugin so they are
// included in the help output. If we have no args
// then this is being used for `docker help` and we
// want to include broken plugins, otherwise this is
// `help «foo»` and we do not.
if err := pluginmanager.AddPluginCommandStubs(dockerCli, ccmd.Root(), len(args) == 0); err != nil {
ccmd.Println(err)
return
}

defaultHelpFunc(ccmd, args)
})
}
Expand Down
38 changes: 32 additions & 6 deletions e2e/cli-plugins/help_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,29 +34,55 @@ func TestGlobalHelp(t *testing.T) {
// - The `badmeta` plugin under the "Invalid Plugins" heading.
//
// Regexps are needed because the width depends on `unix.TIOCGWINSZ` or similar.
helloworldre := regexp.MustCompile(`^ helloworld\s+\(Docker Inc\.\)\s+A basic Hello World plugin for tests$`)
badmetare := regexp.MustCompile(`^ badmeta\s+invalid metadata: invalid character 'i' looking for beginning of object key string$`)
var helloworldcount, badmetacount int
for _, expected := range []*regexp.Regexp{
regexp.MustCompile(`^A self-sufficient runtime for containers$`),
regexp.MustCompile(`^Management Commands:$`),
regexp.MustCompile(`^ container\s+Manage containers$`),
regexp.MustCompile(`^Commands:$`),
regexp.MustCompile(`^ create\s+Create a new container$`),
regexp.MustCompile(`^ helloworld\s+\(Docker Inc\.\)\s+A basic Hello World plugin for tests$`),
helloworldre,
regexp.MustCompile(`^ ps\s+List containers$`),
regexp.MustCompile(`^Invalid Plugins:$`),
regexp.MustCompile(`^ badmeta\s+invalid metadata: invalid character 'i' looking for beginning of object key string$`),
badmetare,
nil, // scan to end of input rather than stopping at badmetare
} {
var found bool
for scanner.Scan() {
if expected.MatchString(scanner.Text()) {
text := scanner.Text()
if helloworldre.MatchString(text) {
helloworldcount++
}
if badmetare.MatchString(text) {
badmetacount++
}

if expected != nil && expected.MatchString(text) {
found = true
break
}
}
assert.Assert(t, found, "Did not find match for %q in `docker help` output", expected)
assert.Assert(t, expected == nil || found, "Did not find match for %q in `docker help` output", expected)
}
// We successfully scanned all the input
assert.Assert(t, scanner.Scan() == false)
assert.NilError(t, scanner.Err())
// Plugins should only be listed once.
assert.Assert(t, is.Equal(helloworldcount, 1))
assert.Assert(t, is.Equal(badmetacount, 1))

// Running with `--help` should produce the same.
res2 := icmd.RunCmd(run("--help"))
res2.Assert(t, icmd.Expected{
ExitCode: 0,
})
assert.Assert(t, is.Equal(res2.Stdout(), res.Stdout()))
assert.Assert(t, is.Equal(res2.Stderr(), ""))

// Running just `docker` (without help) should produce the same thing, except on Stderr
res2 := icmd.RunCmd(run())
// Running just `docker` (without `help` nor `--help`) should produce the same thing, except on Stderr.
res2 = icmd.RunCmd(run())
res2.Assert(t, icmd.Expected{
ExitCode: 0,
})
Expand Down

0 comments on commit f78032f

Please sign in to comment.