From 5a9902255613ba63e81d97f7581bff9ecb55fe46 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 7 Aug 2025 00:19:16 +0200 Subject: [PATCH 1/3] cli: remove HasCompletionArg utility It was only used in a single place and has no external consumers. Move it to where it's used to keep things together. Signed-off-by: Sebastiaan van Stijn --- cli/cobra.go | 10 ---------- cmd/docker/docker.go | 12 +++++++++++- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/cli/cobra.go b/cli/cobra.go index 11ddd1cce6..18fc48f05f 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -185,16 +185,6 @@ func DisableFlagsInUseLine(cmd *cobra.Command) { }) } -// HasCompletionArg returns true if a cobra completion arg request is found. -func HasCompletionArg(args []string) bool { - for _, arg := range args { - if arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd { - return true - } - } - return false -} - var helpCommand = &cobra.Command{ Use: "help [command]", Short: "Help about the command", diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index f6ee1753fd..13ce196aaf 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -451,7 +451,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { return err } - if cli.HasCompletionArg(args) { + if hasCompletionArg(args) { // We add plugin command stubs early only for completion. We don't // want to add them for normal command execution as it would cause // a significant performance hit. @@ -504,6 +504,16 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { return err } +// hasCompletionArg returns true if a cobra completion arg request is found. +func hasCompletionArg(args []string) bool { + for _, arg := range args { + if arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd { + return true + } + } + return false +} + type versionDetails interface { CurrentVersion() string ServerInfo() command.ServerInfo From 6bd8a4b2b5d8854bf68256ee80964a9031ae931b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 7 Aug 2025 17:42:47 +0200 Subject: [PATCH 2/3] cli: deprecate VisitAll, DisableFlagsInUseLine utilities These utilities were only used internally; create a local copy where used, and deprecate the ones in cli. Signed-off-by: Sebastiaan van Stijn --- cli-plugins/plugin/plugin.go | 15 +++++++++++- cli-plugins/plugin/plugin_test.go | 28 +++++++++++++++++++++++ cli/cobra.go | 14 ++++++++---- cli/cobra_test.go | 22 ------------------ cmd/docker/docker.go | 38 +++++++++++++++++++++++-------- cmd/docker/docker_test.go | 19 ++++++++++++++++ 6 files changed, 99 insertions(+), 37 deletions(-) create mode 100644 cli-plugins/plugin/plugin_test.go diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index ee5c32d38b..2d7547ee47 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -175,11 +175,24 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta newMetadataSubcommand(plugin, meta), ) - cli.DisableFlagsInUseLine(cmd) + visitAll(cmd, + // prevent adding "[flags]" to the end of the usage line. + func(c *cobra.Command) { c.DisableFlagsInUseLine = true }, + ) return cli.NewTopLevelCommand(cmd, dockerCli, opts, cmd.Flags()) } +// visitAll traverses all commands from the root. +func visitAll(root *cobra.Command, fns ...func(*cobra.Command)) { + for _, cmd := range root.Commands() { + visitAll(cmd, fns...) + } + for _, fn := range fns { + fn(root) + } +} + func newMetadataSubcommand(plugin *cobra.Command, meta metadata.Metadata) *cobra.Command { if meta.ShortDescription == "" { meta.ShortDescription = plugin.Short diff --git a/cli-plugins/plugin/plugin_test.go b/cli-plugins/plugin/plugin_test.go new file mode 100644 index 0000000000..8b2b299fb1 --- /dev/null +++ b/cli-plugins/plugin/plugin_test.go @@ -0,0 +1,28 @@ +package plugin + +import ( + "slices" + "testing" + + "github.com/spf13/cobra" +) + +func TestVisitAll(t *testing.T) { + root := &cobra.Command{Use: "root"} + sub1 := &cobra.Command{Use: "sub1"} + sub1sub1 := &cobra.Command{Use: "sub1sub1"} + sub1sub2 := &cobra.Command{Use: "sub1sub2"} + sub2 := &cobra.Command{Use: "sub2"} + + root.AddCommand(sub1, sub2) + sub1.AddCommand(sub1sub1, sub1sub2) + + var visited []string + visitAll(root, func(ccmd *cobra.Command) { + visited = append(visited, ccmd.Name()) + }) + expected := []string{"sub1sub1", "sub1sub2", "sub1", "sub2", "root"} + if !slices.Equal(expected, visited) { + t.Errorf("expected %#v, got %#v", expected, visited) + } +} diff --git a/cli/cobra.go b/cli/cobra.go index 18fc48f05f..983e6caee3 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -167,19 +167,25 @@ func (tcmd *TopLevelCommand) Initialize(ops ...command.CLIOption) error { } // VisitAll will traverse all commands from the root. -// This is different from the VisitAll of cobra.Command where only parents -// are checked. +// +// Deprecated: this utility was only used internally and will be removed in the next release. func VisitAll(root *cobra.Command, fn func(*cobra.Command)) { + visitAll(root, fn) +} + +func visitAll(root *cobra.Command, fn func(*cobra.Command)) { for _, cmd := range root.Commands() { - VisitAll(cmd, fn) + visitAll(cmd, fn) } fn(root) } // DisableFlagsInUseLine sets the DisableFlagsInUseLine flag on all // commands within the tree rooted at cmd. +// +// Deprecated: this utility was only used internally and will be removed in the next release. func DisableFlagsInUseLine(cmd *cobra.Command) { - VisitAll(cmd, func(ccmd *cobra.Command) { + visitAll(cmd, func(ccmd *cobra.Command) { // do not add a `[flags]` to the end of the usage line. ccmd.DisableFlagsInUseLine = true }) diff --git a/cli/cobra_test.go b/cli/cobra_test.go index e1d4bcb2d2..9ca87ea5c8 100644 --- a/cli/cobra_test.go +++ b/cli/cobra_test.go @@ -10,28 +10,6 @@ import ( is "gotest.tools/v3/assert/cmp" ) -func TestVisitAll(t *testing.T) { - root := &cobra.Command{Use: "root"} - sub1 := &cobra.Command{Use: "sub1"} - sub1sub1 := &cobra.Command{Use: "sub1sub1"} - sub1sub2 := &cobra.Command{Use: "sub1sub2"} - sub2 := &cobra.Command{Use: "sub2"} - - root.AddCommand(sub1, sub2) - sub1.AddCommand(sub1sub1, sub1sub2) - - // Take the opportunity to test DisableFlagsInUseLine too - DisableFlagsInUseLine(root) - - var visited []string - VisitAll(root, func(ccmd *cobra.Command) { - visited = append(visited, ccmd.Name()) - assert.Assert(t, ccmd.DisableFlagsInUseLine, "DisableFlagsInUseLine not set on %q", ccmd.Name()) - }) - expected := []string{"sub1sub1", "sub1sub2", "sub1", "sub2", "root"} - assert.DeepEqual(t, expected, visited) -} - func TestVendorAndVersion(t *testing.T) { // Non plugin. assert.Equal(t, vendorAndVersion(&cobra.Command{Use: "test"}), "") diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 13ce196aaf..1bd4e6207b 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -163,8 +163,11 @@ func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { cmd.SetOut(dockerCli.Out()) commands.AddCommands(cmd, dockerCli) - cli.DisableFlagsInUseLine(cmd) - setValidateArgs(dockerCli, cmd) + visitAll(cmd, + setValidateArgs(dockerCli), + // prevent adding "[flags]" to the end of the usage line. + func(c *cobra.Command) { c.DisableFlagsInUseLine = true }, + ) // flags must be the top-level command flags, not cmd.Flags() return cli.NewTopLevelCommand(cmd, dockerCli, opts, cmd.Flags()) @@ -265,14 +268,29 @@ func setHelpFunc(dockerCli command.Cli, cmd *cobra.Command) { }) } -func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) { - // The Args is handled by ValidateArgs in cobra, which does not allows a pre-hook. - // As a result, here we replace the existing Args validation func to a wrapper, - // where the wrapper will check to see if the feature is supported or not. - // The Args validation error will only be returned if the feature is supported. - cli.VisitAll(cmd, func(ccmd *cobra.Command) { +// visitAll traverses all commands from the root. +func visitAll(root *cobra.Command, fns ...func(*cobra.Command)) { + for _, cmd := range root.Commands() { + visitAll(cmd, fns...) + } + for _, fn := range fns { + fn(root) + } +} + +// The Args is handled by ValidateArgs in cobra, which does not allows a pre-hook. +// As a result, here we replace the existing Args validation func to a wrapper, +// where the wrapper will check to see if the feature is supported or not. +// The Args validation error will only be returned if the feature is supported. +func setValidateArgs(dockerCLI versionDetails) func(*cobra.Command) { + return func(ccmd *cobra.Command) { // if there is no tags for a command or any of its parent, // there is no need to wrap the Args validation. + // + // FIXME(thaJeztah): can we memoize properties of the parent? + // visitAll traverses root -> all childcommands, and hasTags + // goes the reverse (cmd -> visit all parents), so we may + // end traversing two directions. if !hasTags(ccmd) { return } @@ -283,12 +301,12 @@ func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) { cmdArgs := ccmd.Args ccmd.Args = func(cmd *cobra.Command, args []string) error { - if err := isSupported(cmd, dockerCli); err != nil { + if err := isSupported(cmd, dockerCLI); err != nil { return err } return cmdArgs(cmd, args) } - }) + } } func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command, subcommand string, envs []string) error { diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index 48d37131c4..99e9583cfb 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -14,6 +14,7 @@ import ( "github.com/docker/cli/cli/debug" platformsignals "github.com/docker/cli/cmd/docker/internal/signals" "github.com/sirupsen/logrus" + "github.com/spf13/cobra" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) @@ -108,3 +109,21 @@ func TestUserTerminatedError(t *testing.T) { assert.Equal(t, getExitCode(context.Cause(notifyCtx)), 143) } + +func TestVisitAll(t *testing.T) { + root := &cobra.Command{Use: "root"} + sub1 := &cobra.Command{Use: "sub1"} + sub1sub1 := &cobra.Command{Use: "sub1sub1"} + sub1sub2 := &cobra.Command{Use: "sub1sub2"} + sub2 := &cobra.Command{Use: "sub2"} + + root.AddCommand(sub1, sub2) + sub1.AddCommand(sub1sub1, sub1sub2) + + var visited []string + visitAll(root, func(ccmd *cobra.Command) { + visited = append(visited, ccmd.Name()) + }) + expected := []string{"sub1sub1", "sub1sub2", "sub1", "sub2", "root"} + assert.DeepEqual(t, expected, visited) +} From 5a381189566471c9ee96c90ccb3b68009a7c3694 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 15 Aug 2025 14:33:42 +0200 Subject: [PATCH 3/3] cmd/docker: fix some minor linting issues Signed-off-by: Sebastiaan van Stijn --- cmd/docker/docker.go | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 1bd4e6207b..fccd0756ea 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -339,19 +339,19 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command // signals to the subprocess because the shared // pgid makes the TTY a controlling terminal. // - // The plugin should have it's own copy of this + // The plugin should have its own copy of this // termination logic, and exit after 3 retries - // on it's own. + // on its own. if dockerCli.Out().IsTerminal() { return } - // Terminate the plugin server, which will - // close all connections with plugin - // subprocesses, and signal them to exit. + // Terminate the plugin server, which closes + // all connections with plugin subprocesses, + // and signal them to exit. // - // Repeated invocations will result in EINVAL, - // or EBADF; but that is fine for our purposes. + // Repeated invocations result in EINVAL or EBADF, + // but that is fine for our purposes. if srv != nil { _ = srv.Close() } @@ -369,15 +369,15 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command go func() { retries := 0 - force := false // catch the first signal through context cancellation <-ctx.Done() - tryTerminatePlugin(force) + tryTerminatePlugin(false) // register subsequent signals signals := make(chan os.Signal, exitLimit) signal.Notify(signals, platformsignals.TerminationSignals...) + force := false for range signals { retries++ // If we're still running after 3 interruptions @@ -458,7 +458,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { } }() } else { - fmt.Fprint(dockerCli.Err(), "Warning: Unexpected OTEL error, metrics may not be flushed") + _, _ = fmt.Fprint(dockerCli.Err(), "Warning: Unexpected OTEL error, metrics may not be flushed") } dockerCli.InstrumentCobraCommands(ctx, cmd) @@ -473,8 +473,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error { // We add plugin command stubs early only for completion. We don't // want to add them for normal command execution as it would cause // a significant performance hit. - err = pluginmanager.AddPluginCommandStubs(dockerCli, cmd) - if err != nil { + if err := pluginmanager.AddPluginCommandStubs(dockerCli, cmd); err != nil { return err } }