From 60d16e20ac2479aada99a22da67cc8d4e9d49ec8 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 7 Aug 2025 17:42:47 +0200 Subject: [PATCH] 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 (cherry picked from commit 6bd8a4b2b5d8854bf68256ee80964a9031ae931b) 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 39d6d86943..f4c80b3a48 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 b3e663423b..a75a66e660 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -168,19 +168,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 ad2cc096f2..57c4b8ce26 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) +}