From 572e3f1c539fe3dbdeeec812c09bb76419fd7e02 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 23 Apr 2025 12:12:25 +0200 Subject: [PATCH] opts: ListOpts: implement cobra.SliceValue to fix shell completion Cobra's shell completion has specific rules to decide whether a flag can be accepted multiple times. If a flag does not meet that rule, it only completes the flag name once; some of those rules depend on the "type" of the option to end with "Array" or "Slice", which most of our options don't. Starting with Cobra 1.9, it also checks whether an option implements the [cobra.SliceValue] interface (see [spf13/cobra 2210]). This patch implements the [cobra.SliceValue] interface on ListOpts, so that these options can be completed multiple times. In a follow-up, we can update our code to replace our uses of `GetAll()`, which is identical with the `GetSlice()` method, and potentially deprecate the old method. Before this patch, ListOpts would only be completed once when completing flag names. For example, the following would show the `--label` flag the first time, but omit it if a `--label` flag was already set; docker run--l --label (Set meta data on a container) --link-local-ip (Container IPv4/IPv6 link-local addresses) --label-file (Read in a line delimited file of labels) --log-driver (Logging driver for the container) --link (Add link to another container) --log-opt (Log driver options) docker run --label hello --l --label-file (Read in a line delimited file of labels) --link-local-ip (Container IPv4/IPv6 link-local addresses) --log-opt (Log driver options) --link (Add link to another container) --log-driver (Logging driver for the container) With this patch, the completion script correctly identifies the `--label` flag to be accepted multiple times, and also completes it when already set; docker run --label hello --l --label (Set meta data on a container) --link-local-ip (Container IPv4/IPv6 link-local addresses) --label-file (Read in a line delimited file of labels) --log-driver (Logging driver for the container) --link (Add link to another container) --log-opt (Log driver options) [cobra.SliceValue]: https://pkg.go.dev/github.com/spf13/cobra@v1.9.1#SliceValue [spf13/cobra 2210]: https://github.com/spf13/cobra/pull/2210 Signed-off-by: Sebastiaan van Stijn --- opts/opts.go | 10 ++++++++++ opts/opts_test.go | 10 ++++++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/opts/opts.go b/opts/opts.go index 696b5e9576d9..0288b2cb59d1 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -84,6 +84,16 @@ func (opts *ListOpts) GetAll() []string { return *opts.values } +// GetSlice returns the values of slice. +// +// It implements [cobra.SliceValue] to allow shell completion to be provided +// multiple times. +// +// [cobra.SliceValue]: https://pkg.go.dev/github.com/spf13/cobra@v1.9.1#SliceValue +func (opts *ListOpts) GetSlice() []string { + return *opts.values +} + // GetAllOrEmpty returns the values of the slice // or an empty slice when there are no values. func (opts *ListOpts) GetAllOrEmpty() []string { diff --git a/opts/opts_test.go b/opts/opts_test.go index 5169d48a6c2e..7dc87cad9e83 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -112,6 +112,7 @@ func TestMapOpts(t *testing.T) { } } +//nolint:gocyclo // ignore "cyclomatic complexity 17 is too high" func TestListOptsWithoutValidator(t *testing.T) { o := NewListOpts(nil) err := o.Set("foo") @@ -145,12 +146,13 @@ func TestListOptsWithoutValidator(t *testing.T) { if o.String() != "[bar bar]" { t.Errorf("%s != [bar bar]", o.String()) } - listOpts := o.GetAll() - if len(listOpts) != 2 || listOpts[0] != "bar" || listOpts[1] != "bar" { + if listOpts := o.GetAll(); len(listOpts) != 2 || listOpts[0] != "bar" || listOpts[1] != "bar" { t.Errorf("Expected [[bar bar]], got [%v]", listOpts) } - mapListOpts := o.GetMap() - if len(mapListOpts) != 1 { + if listOpts := o.GetSlice(); len(listOpts) != 2 || listOpts[0] != "bar" || listOpts[1] != "bar" { + t.Errorf("Expected [[bar bar]], got [%v]", listOpts) + } + if mapListOpts := o.GetMap(); len(mapListOpts) != 1 { t.Errorf("Expected [map[bar:{}]], got [%v]", mapListOpts) } }