From 667dcb0e8e550d1a80ee67c6d71979e8cfabea1f Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 16 May 2016 17:20:29 -0400 Subject: [PATCH] Update usage and help to (almost) match the existing docker behaviour Signed-off-by: Daniel Nephin --- api/client/volume/cmd.go | 11 +++++++++ api/client/volume/create.go | 9 +++++++- api/client/volume/list.go | 5 ++++ cli/cobraadaptor/adaptor.go | 28 +++++++++++++++-------- cli/required.go | 14 ++++++++++++ cli/usage.go | 1 - integration-cli/docker_cli_help_test.go | 9 ++++---- integration-cli/docker_cli_volume_test.go | 14 ++++++------ opts/opts.go | 5 ++++ 9 files changed, 73 insertions(+), 23 deletions(-) diff --git a/api/client/volume/cmd.go b/api/client/volume/cmd.go index 6c17a9cec8..1a8f74f7e7 100644 --- a/api/client/volume/cmd.go +++ b/api/client/volume/cmd.go @@ -1,9 +1,12 @@ package volume import ( + "fmt" + "github.com/spf13/cobra" "github.com/docker/docker/api/client" + "github.com/docker/docker/cli" ) // NewVolumeCommand returns a cobra command for `volume` subcommands @@ -11,6 +14,14 @@ func NewVolumeCommand(dockerCli *client.DockerCli) *cobra.Command { cmd := &cobra.Command{ Use: "volume", Short: "Manage Docker volumes", + // TODO: remove once cobra is patched to handle this + RunE: func(cmd *cobra.Command, args []string) error { + fmt.Fprintf(dockerCli.Err(), "\n%s", cmd.UsageString()) + if len(args) > 0 { + return cli.StatusError{StatusCode: 1} + } + return nil + }, } cmd.AddCommand( newCreateCommand(dockerCli), diff --git a/api/client/volume/create.go b/api/client/volume/create.go index 4c6d0a56b5..89b37796d6 100644 --- a/api/client/volume/create.go +++ b/api/client/volume/create.go @@ -6,6 +6,7 @@ import ( "golang.org/x/net/context" "github.com/docker/docker/api/client" + "github.com/docker/docker/cli" "github.com/docker/docker/opts" runconfigopts "github.com/docker/docker/runconfig/opts" "github.com/docker/engine-api/types" @@ -20,12 +21,18 @@ type createOptions struct { } func newCreateCommand(dockerCli *client.DockerCli) *cobra.Command { - var opts createOptions + opts := createOptions{ + driverOpts: *opts.NewMapOpts(nil, nil), + } cmd := &cobra.Command{ Use: "create", Short: "Create a volume", RunE: func(cmd *cobra.Command, args []string) error { + // TODO: remove once cobra is patched to handle this + if err := cli.AcceptsNoArgs(args, cmd); err != nil { + return err + } return runCreate(dockerCli, opts) }, } diff --git a/api/client/volume/list.go b/api/client/volume/list.go index 541c2c1ed6..3510063ff9 100644 --- a/api/client/volume/list.go +++ b/api/client/volume/list.go @@ -8,6 +8,7 @@ import ( "golang.org/x/net/context" "github.com/docker/docker/api/client" + "github.com/docker/docker/cli" "github.com/docker/engine-api/types" "github.com/docker/engine-api/types/filters" "github.com/spf13/cobra" @@ -34,6 +35,10 @@ func newListCommand(dockerCli *client.DockerCli) *cobra.Command { Aliases: []string{"list"}, Short: "List volumes", RunE: func(cmd *cobra.Command, args []string) error { + // TODO: remove once cobra is patched to handle this + if err := cli.AcceptsNoArgs(args, cmd); err != nil { + return err + } return runList(dockerCli, opts) }, } diff --git a/cli/cobraadaptor/adaptor.go b/cli/cobraadaptor/adaptor.go index 07ff8124b0..35b77b47e1 100644 --- a/cli/cobraadaptor/adaptor.go +++ b/cli/cobraadaptor/adaptor.go @@ -18,17 +18,21 @@ type CobraAdaptor struct { // NewCobraAdaptor returns a new handler func NewCobraAdaptor(clientFlags *cliflags.ClientFlags) CobraAdaptor { - var rootCmd = &cobra.Command{ - Use: "docker", - } - rootCmd.SetUsageTemplate(usageTemplate) - stdin, stdout, stderr := term.StdStreams() dockerCli := client.NewDockerCli(stdin, stdout, stderr, clientFlags) + var rootCmd = &cobra.Command{ + Use: "docker", + SilenceUsage: true, + SilenceErrors: true, + } + rootCmd.SetUsageTemplate(usageTemplate) + rootCmd.SetHelpTemplate(helpTemplate) + rootCmd.SetOutput(stdout) rootCmd.AddCommand( volume.NewVolumeCommand(dockerCli), ) + return CobraAdaptor{ rootCmd: rootCmd, dockerCli: dockerCli, @@ -64,20 +68,24 @@ func (c CobraAdaptor) Command(name string) func(...string) error { return nil } -var usageTemplate = `Usage: {{if .Runnable}}{{if .HasFlags}}{{appendIfNotPresent .UseLine "[OPTIONS]"}}{{else}}{{.UseLine}}{{end}}{{end}}{{if .HasSubCommands}}{{ .CommandPath}} COMMAND {{end}}{{if gt .Aliases 0}} +var usageTemplate = `Usage: {{if not .HasSubCommands}}{{if .HasLocalFlags}}{{appendIfNotPresent .UseLine "[OPTIONS]"}}{{else}}{{.UseLine}}{{end}}{{end}}{{if .HasSubCommands}}{{ .CommandPath}} COMMAND{{end}} + +{{with or .Long .Short }}{{. | trim}}{{end}}{{if gt .Aliases 0}} Aliases: - {{.NameAndAliases}} -{{end}}{{if .HasExample}} + {{.NameAndAliases}}{{end}}{{if .HasExample}} Examples: -{{ .Example }}{{end}}{{ if .HasLocalFlags}} +{{ .Example }}{{end}}{{if .HasFlags}} Options: -{{.LocalFlags.FlagUsages | trimRightSpace}}{{end}}{{ if .HasAvailableSubCommands}} +{{.Flags.FlagUsages | trimRightSpace}}{{end}}{{ if .HasAvailableSubCommands}} Commands:{{range .Commands}}{{if .IsAvailableCommand}} {{rpad .Name .NamePadding }} {{.Short}}{{end}}{{end}}{{end}}{{ if .HasSubCommands }} Run '{{.CommandPath}} COMMAND --help' for more information on a command.{{end}} ` + +var helpTemplate = ` +{{if or .Runnable .HasSubCommands}}{{.UsageString}}{{end}}` diff --git a/cli/required.go b/cli/required.go index 6b83fadde1..db1a98bd52 100644 --- a/cli/required.go +++ b/cli/required.go @@ -21,3 +21,17 @@ func MinRequiredArgs(args []string, min int, cmd *cobra.Command) error { cmd.Short, ) } + +// AcceptsNoArgs returns an error message if there are args +func AcceptsNoArgs(args []string, cmd *cobra.Command) error { + if len(args) == 0 { + return nil + } + + return fmt.Errorf( + "\"%s\" accepts no argument(s).\n\nUsage: %s\n\n%s", + cmd.CommandPath(), + cmd.UseLine(), + cmd.Short, + ) +} diff --git a/cli/usage.go b/cli/usage.go index 1ef6a35dec..9ddc17326a 100644 --- a/cli/usage.go +++ b/cli/usage.go @@ -48,7 +48,6 @@ var DockerCommandUsage = []Command{ {"unpause", "Unpause all processes within a container"}, {"update", "Update configuration of one or more containers"}, {"version", "Show the Docker version information"}, - {"volume", "Manage Docker volumes"}, {"wait", "Block until a container stops, then print its exit code"}, } diff --git a/integration-cli/docker_cli_help_test.go b/integration-cli/docker_cli_help_test.go index b301aeef2e..5e27a6e663 100644 --- a/integration-cli/docker_cli_help_test.go +++ b/integration-cli/docker_cli_help_test.go @@ -249,7 +249,8 @@ func testCommand(cmd string, newEnvs []string, scanForHome bool, home string) er // If a line starts with 4 spaces then assume someone // added a multi-line description for an option and we need // to flag it - if strings.HasPrefix(line, " ") { + if strings.HasPrefix(line, " ") && + !strings.HasPrefix(strings.TrimLeft(line, " "), "--") { return fmt.Errorf("Help for %q should not have a multi-line option", cmd) } @@ -260,7 +261,7 @@ func testCommand(cmd string, newEnvs []string, scanForHome bool, home string) er // Options should NOT end with a space if strings.HasSuffix(line, " ") { - return fmt.Errorf("Help for %q should not end with a space", cmd) + return fmt.Errorf("Help for %q should not end with a space: %s", cmd, line) } } @@ -326,8 +327,8 @@ func testCommand(cmd string, newEnvs []string, scanForHome bool, home string) er return fmt.Errorf("Bad output from %q\nstdout:%q\nstderr:%q\nec:%d\nerr:%q\n", args, out, stderr, ec, err) } // Should have just short usage - if !strings.Contains(stderr, "\nUsage:\t") { - return fmt.Errorf("Missing short usage on %q\n", args) + if !strings.Contains(stderr, "\nUsage:") { + return fmt.Errorf("Missing short usage on %q\n:%#v", args, stderr) } // But shouldn't have full usage if strings.Contains(stderr, "--help=false") { diff --git a/integration-cli/docker_cli_volume_test.go b/integration-cli/docker_cli_volume_test.go index 6756fafe77..8835e1d60f 100644 --- a/integration-cli/docker_cli_volume_test.go +++ b/integration-cli/docker_cli_volume_test.go @@ -25,7 +25,7 @@ func (s *DockerSuite) TestVolumeCliCreateOptionConflict(c *check.C) { c.Assert(err, check.NotNil, check.Commentf("volume create exception name already in use with another driver")) c.Assert(out, checker.Contains, "A volume named test already exists") - out, _ = dockerCmd(c, "volume", "inspect", "--format='{{ .Driver }}'", "test") + out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Driver }}", "test") _, _, err = dockerCmdWithError("volume", "create", "--name", "test", "--driver", strings.TrimSpace(out)) c.Assert(err, check.IsNil) } @@ -39,11 +39,11 @@ func (s *DockerSuite) TestVolumeCliInspect(c *check.C) { out, _ := dockerCmd(c, "volume", "create") name := strings.TrimSpace(out) - out, _ = dockerCmd(c, "volume", "inspect", "--format='{{ .Name }}'", name) + out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Name }}", name) c.Assert(strings.TrimSpace(out), check.Equals, name) dockerCmd(c, "volume", "create", "--name", "test") - out, _ = dockerCmd(c, "volume", "inspect", "--format='{{ .Name }}'", "test") + out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Name }}", "test") c.Assert(strings.TrimSpace(out), check.Equals, "test") } @@ -212,7 +212,7 @@ func (s *DockerSuite) TestVolumeCliRm(c *check.C) { func (s *DockerSuite) TestVolumeCliNoArgs(c *check.C) { out, _ := dockerCmd(c, "volume") // no args should produce the cmd usage output - usage := "Usage: docker volume [OPTIONS] [COMMAND]" + usage := "Usage: docker volume COMMAND" c.Assert(out, checker.Contains, usage) // invalid arg should error and show the command usage on stderr @@ -224,7 +224,7 @@ func (s *DockerSuite) TestVolumeCliNoArgs(c *check.C) { _, stderr, _, err = runCommandWithStdoutStderr(exec.Command(dockerBinary, "volume", "--no-such-flag")) c.Assert(err, check.NotNil, check.Commentf(stderr)) c.Assert(stderr, checker.Contains, usage) - c.Assert(stderr, checker.Contains, "flag provided but not defined: --no-such-flag") + c.Assert(stderr, checker.Contains, "unknown flag: --no-such-flag") } func (s *DockerSuite) TestVolumeCliInspectTmplError(c *check.C) { @@ -268,7 +268,7 @@ func (s *DockerSuite) TestVolumeCliCreateLabel(c *check.C) { out, _, err := dockerCmdWithError("volume", "create", "--label", testLabel+"="+testValue, "--name", testVol) c.Assert(err, check.IsNil) - out, _ = dockerCmd(c, "volume", "inspect", "--format='{{ .Labels."+testLabel+" }}'", testVol) + out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Labels."+testLabel+" }}", testVol) c.Assert(strings.TrimSpace(out), check.Equals, testValue) } @@ -295,7 +295,7 @@ func (s *DockerSuite) TestVolumeCliCreateLabelMultiple(c *check.C) { c.Assert(err, check.IsNil) for k, v := range testLabels { - out, _ = dockerCmd(c, "volume", "inspect", "--format='{{ .Labels."+k+" }}'", testVol) + out, _ = dockerCmd(c, "volume", "inspect", "--format={{ .Labels."+k+" }}", testVol) c.Assert(strings.TrimSpace(out), check.Equals, v) } } diff --git a/opts/opts.go b/opts/opts.go index 05d5497161..e1d3c2db15 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -100,6 +100,11 @@ func (opts *ListOpts) Len() int { return len((*opts.values)) } +// Type returns a string name for this Option type +func (opts *ListOpts) Type() string { + return "list" +} + // NamedOption is an interface that list and map options // with names implement. type NamedOption interface {