Skip to content

Commit 877a6ef

Browse files
thaJeztahthaJeztah
authored
Merge pull request docker#6273 from thaJeztah/cli_internalize_utils
cli: deprecate VisitAll, DisableFlagsInUseLine utilities, remove HasCompletionArg
2 parents 2bcf433 + 5a38118 commit 877a6ef

File tree

6 files changed

+121
-60
lines changed

6 files changed

+121
-60
lines changed

‎cli-plugins/plugin/plugin.go

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,11 +175,24 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta
175175
newMetadataSubcommand(plugin, meta),
176176
)
177177

178-
cli.DisableFlagsInUseLine(cmd)
178+
visitAll(cmd,
179+
// prevent adding "[flags]" to the end of the usage line.
180+
func(c *cobra.Command) { c.DisableFlagsInUseLine = true },
181+
)
179182

180183
return cli.NewTopLevelCommand(cmd, dockerCli, opts, cmd.Flags())
181184
}
182185

186+
// visitAll traverses all commands from the root.
187+
func visitAll(root *cobra.Command, fns ...func(*cobra.Command)) {
188+
for _, cmd := range root.Commands() {
189+
visitAll(cmd, fns...)
190+
}
191+
for _, fn := range fns {
192+
fn(root)
193+
}
194+
}
195+
183196
func newMetadataSubcommand(plugin *cobra.Command, meta metadata.Metadata) *cobra.Command {
184197
if meta.ShortDescription == "" {
185198
meta.ShortDescription = plugin.Short

‎cli-plugins/plugin/plugin_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
package plugin
2+
3+
import (
4+
"slices"
5+
"testing"
6+
7+
"github.com/spf13/cobra"
8+
)
9+
10+
func TestVisitAll(t *testing.T) {
11+
root := &cobra.Command{Use: "root"}
12+
sub1 := &cobra.Command{Use: "sub1"}
13+
sub1sub1 := &cobra.Command{Use: "sub1sub1"}
14+
sub1sub2 := &cobra.Command{Use: "sub1sub2"}
15+
sub2 := &cobra.Command{Use: "sub2"}
16+
17+
root.AddCommand(sub1, sub2)
18+
sub1.AddCommand(sub1sub1, sub1sub2)
19+
20+
var visited []string
21+
visitAll(root, func(ccmd *cobra.Command) {
22+
visited = append(visited, ccmd.Name())
23+
})
24+
expected := []string{"sub1sub1", "sub1sub2", "sub1", "sub2", "root"}
25+
if !slices.Equal(expected, visited) {
26+
t.Errorf("expected %#v, got %#v", expected, visited)
27+
}
28+
}

‎cli/cobra.go

Lines changed: 10 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -167,34 +167,30 @@ func (tcmd *TopLevelCommand) Initialize(ops ...command.CLIOption) error {
167167
}
168168

169169
// VisitAll will traverse all commands from the root.
170-
// This is different from the VisitAll of cobra.Command where only parents
171-
// are checked.
170+
//
171+
// Deprecated: this utility was only used internally and will be removed in the next release.
172172
func VisitAll(root *cobra.Command, fn func(*cobra.Command)) {
173+
visitAll(root, fn)
174+
}
175+
176+
func visitAll(root *cobra.Command, fn func(*cobra.Command)) {
173177
for _, cmd := range root.Commands() {
174-
VisitAll(cmd, fn)
178+
visitAll(cmd, fn)
175179
}
176180
fn(root)
177181
}
178182

179183
// DisableFlagsInUseLine sets the DisableFlagsInUseLine flag on all
180184
// commands within the tree rooted at cmd.
185+
//
186+
// Deprecated: this utility was only used internally and will be removed in the next release.
181187
func DisableFlagsInUseLine(cmd *cobra.Command) {
182-
VisitAll(cmd, func(ccmd *cobra.Command) {
188+
visitAll(cmd, func(ccmd *cobra.Command) {
183189
// do not add a `[flags]` to the end of the usage line.
184190
ccmd.DisableFlagsInUseLine = true
185191
})
186192
}
187193

188-
// HasCompletionArg returns true if a cobra completion arg request is found.
189-
func HasCompletionArg(args []string) bool {
190-
for _, arg := range args {
191-
if arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd {
192-
return true
193-
}
194-
}
195-
return false
196-
}
197-
198194
var helpCommand = &cobra.Command{
199195
Use: "help [command]",
200196
Short: "Help about the command",

‎cli/cobra_test.go

Lines changed: 0 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -10,28 +10,6 @@ import (
1010
is "gotest.tools/v3/assert/cmp"
1111
)
1212

13-
func TestVisitAll(t *testing.T) {
14-
root := &cobra.Command{Use: "root"}
15-
sub1 := &cobra.Command{Use: "sub1"}
16-
sub1sub1 := &cobra.Command{Use: "sub1sub1"}
17-
sub1sub2 := &cobra.Command{Use: "sub1sub2"}
18-
sub2 := &cobra.Command{Use: "sub2"}
19-
20-
root.AddCommand(sub1, sub2)
21-
sub1.AddCommand(sub1sub1, sub1sub2)
22-
23-
// Take the opportunity to test DisableFlagsInUseLine too
24-
DisableFlagsInUseLine(root)
25-
26-
var visited []string
27-
VisitAll(root, func(ccmd *cobra.Command) {
28-
visited = append(visited, ccmd.Name())
29-
assert.Assert(t, ccmd.DisableFlagsInUseLine, "DisableFlagsInUseLine not set on %q", ccmd.Name())
30-
})
31-
expected := []string{"sub1sub1", "sub1sub2", "sub1", "sub2", "root"}
32-
assert.DeepEqual(t, expected, visited)
33-
}
34-
3513
func TestVendorAndVersion(t *testing.T) {
3614
// Non plugin.
3715
assert.Equal(t, vendorAndVersion(&cobra.Command{Use: "test"}), "")

‎cmd/docker/docker.go

Lines changed: 50 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -163,8 +163,11 @@ func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand {
163163
cmd.SetOut(dockerCli.Out())
164164
commands.AddCommands(cmd, dockerCli)
165165

166-
cli.DisableFlagsInUseLine(cmd)
167-
setValidateArgs(dockerCli, cmd)
166+
visitAll(cmd,
167+
setValidateArgs(dockerCli),
168+
// prevent adding "[flags]" to the end of the usage line.
169+
func(c *cobra.Command) { c.DisableFlagsInUseLine = true },
170+
)
168171

169172
// flags must be the top-level command flags, not cmd.Flags()
170173
return cli.NewTopLevelCommand(cmd, dockerCli, opts, cmd.Flags())
@@ -265,14 +268,29 @@ func setHelpFunc(dockerCli command.Cli, cmd *cobra.Command) {
265268
})
266269
}
267270

268-
func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) {
269-
// The Args is handled by ValidateArgs in cobra, which does not allows a pre-hook.
270-
// As a result, here we replace the existing Args validation func to a wrapper,
271-
// where the wrapper will check to see if the feature is supported or not.
272-
// The Args validation error will only be returned if the feature is supported.
273-
cli.VisitAll(cmd, func(ccmd *cobra.Command) {
271+
// visitAll traverses all commands from the root.
272+
func visitAll(root *cobra.Command, fns ...func(*cobra.Command)) {
273+
for _, cmd := range root.Commands() {
274+
visitAll(cmd, fns...)
275+
}
276+
for _, fn := range fns {
277+
fn(root)
278+
}
279+
}
280+
281+
// The Args is handled by ValidateArgs in cobra, which does not allows a pre-hook.
282+
// As a result, here we replace the existing Args validation func to a wrapper,
283+
// where the wrapper will check to see if the feature is supported or not.
284+
// The Args validation error will only be returned if the feature is supported.
285+
func setValidateArgs(dockerCLI versionDetails) func(*cobra.Command) {
286+
return func(ccmd *cobra.Command) {
274287
// if there is no tags for a command or any of its parent,
275288
// there is no need to wrap the Args validation.
289+
//
290+
// FIXME(thaJeztah): can we memoize properties of the parent?
291+
// visitAll traverses root -> all childcommands, and hasTags
292+
// goes the reverse (cmd -> visit all parents), so we may
293+
// end traversing two directions.
276294
if !hasTags(ccmd) {
277295
return
278296
}
@@ -283,12 +301,12 @@ func setValidateArgs(dockerCli command.Cli, cmd *cobra.Command) {
283301

284302
cmdArgs := ccmd.Args
285303
ccmd.Args = func(cmd *cobra.Command, args []string) error {
286-
if err := isSupported(cmd, dockerCli); err != nil {
304+
if err := isSupported(cmd, dockerCLI); err != nil {
287305
return err
288306
}
289307
return cmdArgs(cmd, args)
290308
}
291-
})
309+
}
292310
}
293311

294312
func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command, subcommand string, envs []string) error {
@@ -321,19 +339,19 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command
321339
// signals to the subprocess because the shared
322340
// pgid makes the TTY a controlling terminal.
323341
//
324-
// The plugin should have it's own copy of this
342+
// The plugin should have its own copy of this
325343
// termination logic, and exit after 3 retries
326-
// on it's own.
344+
// on its own.
327345
if dockerCli.Out().IsTerminal() {
328346
return
329347
}
330348

331-
// Terminate the plugin server, which will
332-
// close all connections with plugin
333-
// subprocesses, and signal them to exit.
349+
// Terminate the plugin server, which closes
350+
// all connections with plugin subprocesses,
351+
// and signal them to exit.
334352
//
335-
// Repeated invocations will result in EINVAL,
336-
// or EBADF; but that is fine for our purposes.
353+
// Repeated invocations result in EINVAL or EBADF,
354+
// but that is fine for our purposes.
337355
if srv != nil {
338356
_ = srv.Close()
339357
}
@@ -351,15 +369,15 @@ func tryPluginRun(ctx context.Context, dockerCli command.Cli, cmd *cobra.Command
351369

352370
go func() {
353371
retries := 0
354-
force := false
355372
// catch the first signal through context cancellation
356373
<-ctx.Done()
357-
tryTerminatePlugin(force)
374+
tryTerminatePlugin(false)
358375

359376
// register subsequent signals
360377
signals := make(chan os.Signal, exitLimit)
361378
signal.Notify(signals, platformsignals.TerminationSignals...)
362379

380+
force := false
363381
for range signals {
364382
retries++
365383
// If we're still running after 3 interruptions
@@ -440,7 +458,7 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
440458
}
441459
}()
442460
} else {
443-
fmt.Fprint(dockerCli.Err(), "Warning: Unexpected OTEL error, metrics may not be flushed")
461+
_, _ = fmt.Fprint(dockerCli.Err(), "Warning: Unexpected OTEL error, metrics may not be flushed")
444462
}
445463

446464
dockerCli.InstrumentCobraCommands(ctx, cmd)
@@ -451,12 +469,11 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
451469
return err
452470
}
453471

454-
if cli.HasCompletionArg(args) {
472+
if hasCompletionArg(args) {
455473
// We add plugin command stubs early only for completion. We don't
456474
// want to add them for normal command execution as it would cause
457475
// a significant performance hit.
458-
err = pluginmanager.AddPluginCommandStubs(dockerCli, cmd)
459-
if err != nil {
476+
if err := pluginmanager.AddPluginCommandStubs(dockerCli, cmd); err != nil {
460477
return err
461478
}
462479
}
@@ -504,6 +521,16 @@ func runDocker(ctx context.Context, dockerCli *command.DockerCli) error {
504521
return err
505522
}
506523

524+
// hasCompletionArg returns true if a cobra completion arg request is found.
525+
func hasCompletionArg(args []string) bool {
526+
for _, arg := range args {
527+
if arg == cobra.ShellCompRequestCmd || arg == cobra.ShellCompNoDescRequestCmd {
528+
return true
529+
}
530+
}
531+
return false
532+
}
533+
507534
type versionDetails interface {
508535
CurrentVersion() string
509536
ServerInfo() command.ServerInfo

‎cmd/docker/docker_test.go

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/docker/cli/cli/debug"
1515
platformsignals "github.com/docker/cli/cmd/docker/internal/signals"
1616
"github.com/sirupsen/logrus"
17+
"github.com/spf13/cobra"
1718
"gotest.tools/v3/assert"
1819
is "gotest.tools/v3/assert/cmp"
1920
)
@@ -108,3 +109,21 @@ func TestUserTerminatedError(t *testing.T) {
108109

109110
assert.Equal(t, getExitCode(context.Cause(notifyCtx)), 143)
110111
}
112+
113+
func TestVisitAll(t *testing.T) {
114+
root := &cobra.Command{Use: "root"}
115+
sub1 := &cobra.Command{Use: "sub1"}
116+
sub1sub1 := &cobra.Command{Use: "sub1sub1"}
117+
sub1sub2 := &cobra.Command{Use: "sub1sub2"}
118+
sub2 := &cobra.Command{Use: "sub2"}
119+
120+
root.AddCommand(sub1, sub2)
121+
sub1.AddCommand(sub1sub1, sub1sub2)
122+
123+
var visited []string
124+
visitAll(root, func(ccmd *cobra.Command) {
125+
visited = append(visited, ccmd.Name())
126+
})
127+
expected := []string{"sub1sub1", "sub1sub2", "sub1", "sub2", "root"}
128+
assert.DeepEqual(t, expected, visited)
129+
}

0 commit comments

Comments
 (0)