From 1d3d3550f5b00bf7bbda3256dc2a5fb35273ce0a Mon Sep 17 00:00:00 2001 From: Toni Kangas <toni.kangas@upcloud.com> Date: Tue, 21 Jun 2022 23:09:31 +0300 Subject: [PATCH 1/5] refactor(completion): use the default completion command provided by cobra This removes custom bash completion logic and custom completion escape logic as that is not supported by completion scripts cobra generates. --- CHANGELOG.md | 4 ++ internal/commands/all/all.go | 9 ---- internal/commands/bash_completion.go | 79 ---------------------------- internal/commands/root/completion.go | 31 ----------- internal/completion/helpers.go | 13 +---- internal/completion/helpers_test.go | 42 +-------------- internal/core/core.go | 2 - 7 files changed, 7 insertions(+), 173 deletions(-) delete mode 100644 internal/commands/bash_completion.go delete mode 100644 internal/commands/root/completion.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a941e9465..3d0b5f45d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Print warning about unknown resource state before exiting when execution is interrupted with SIGINT. - Add `kubernetes nodegroup create`, `kubernetes nodegroup scale`, and `kubernetes nodegroup delete` commands (EXPERIMENTAL) +- Added support for all shell completions provided by `cobra`. + +### Changed +- Remove custom bash completion logic and replace it with `completion` command provided by `cobra`. ## [2.4.0] - 2022-12-19 ### Added diff --git a/internal/commands/all/all.go b/internal/commands/all/all.go index b5df93bac..7a3155b41 100644 --- a/internal/commands/all/all.go +++ b/internal/commands/all/all.go @@ -142,15 +142,6 @@ func BuildCommands(rootCmd *cobra.Command, conf *config.Config) { commands.BuildCommand(nodegroup.DeleteCommand(), nodeGroupCommand.Cobra(), conf) // Misc - commands.BuildCommand( - &root.CompletionCommand{ - BaseCommand: commands.New( - "completion", - "Generates shell completion", - "upctl completion bash", - ), - }, rootCmd, conf, - ) commands.BuildCommand( &root.VersionCommand{ BaseCommand: commands.New( diff --git a/internal/commands/bash_completion.go b/internal/commands/bash_completion.go deleted file mode 100644 index 604e72449..000000000 --- a/internal/commands/bash_completion.go +++ /dev/null @@ -1,79 +0,0 @@ -package commands - -import ( - "fmt" - - "github.com/spf13/cobra" -) - -// Override Cobra's custom completion function -// This fixes quoting of items with spaces -// TODO: get fixes submitted upstream, otherwise sooner or later something will break if we upgrade cobra -const goCustomCompletion = `__%[1]s_handle_go_custom_completion() -{ - __%[1]s_debug "${FUNCNAME[0]}: cur is ${cur}, words[*] is ${words[*]}, #words[@] is ${#words[@]}" - - local out requestComp lastParam lastChar comp directive args - - # Prepare the command to request completions for the program. - # Calling ${words[0]} instead of directly %[1]s allows to handle aliases - args=("${words[@]:1}") - requestComp="${words[0]} __completeNoDesc ${args[@]:0:$((${#args[@]}-1))} $'$cur'" - - lastParam=${words[$((${#words[@]}-1))]} - lastChar=${lastParam:$((${#lastParam}-1)):1} - __%[1]s_debug "${FUNCNAME[0]}: lastParam ${lastParam}, lastChar ${lastChar}" - - if [ -z "${cur}" ] && [ "${lastChar}" != "=" ]; then - # If the last parameter is complete (there is a space following it) - # We add an extra empty parameter so we can indicate this to the go method. - __%[1]s_debug "${FUNCNAME[0]}: Adding extra empty parameter" - requestComp="${requestComp} \"\"" - fi - - __%[1]s_debug "${FUNCNAME[0]}: calling ${requestComp}" - # Use eval to handle any environment variables and such - out=$(eval "${requestComp}" 2>/dev/null) - - # Extract the directive integer at the very end of the output following a colon (:) - directive=${out##*:} - # Remove the directive - out=${out%%:*} - if [ "${directive}" = "${out}" ]; then - # There is not directive specified - directive=0 - fi - __%[1]s_debug "${FUNCNAME[0]}: the completion directive is: ${directive}" - __%[1]s_debug "${FUNCNAME[0]}: the completions are: ${out[*]}" - - if [ $((directive & %[3]d)) -ne 0 ]; then - # Error code. No completion. - __%[1]s_debug "${FUNCNAME[0]}: received error from custom completion go code" - return - else - if [ $((directive & %[4]d)) -ne 0 ]; then - if [[ $(type -t compopt) = "builtin" ]]; then - __%[1]s_debug "${FUNCNAME[0]}: activating no space" - compopt -o nospace - fi - fi - if [ $((directive & %[5]d)) -ne 0 ]; then - if [[ $(type -t compopt) = "builtin" ]]; then - __%[1]s_debug "${FUNCNAME[0]}: activating no file completion" - compopt +o default - fi - fi - - local IFS=$'\n' - COMPREPLY=($out) - fi -}` - -// CustomBashCompletionFunc returns a bash completion function used by cobras bash completion generator -func CustomBashCompletionFunc(name string) string { - return fmt.Sprintf(goCustomCompletion, name, - cobra.ShellCompNoDescRequestCmd, - cobra.ShellCompDirectiveError, - cobra.ShellCompDirectiveNoSpace, - cobra.ShellCompDirectiveNoFileComp) -} diff --git a/internal/commands/root/completion.go b/internal/commands/root/completion.go deleted file mode 100644 index de07834d1..000000000 --- a/internal/commands/root/completion.go +++ /dev/null @@ -1,31 +0,0 @@ -package root - -import ( - "bytes" - "fmt" - - "github.com/UpCloudLtd/upcloud-cli/v2/internal/commands" - "github.com/UpCloudLtd/upcloud-cli/v2/internal/output" - "github.com/UpCloudLtd/upcloud-cli/v2/internal/resolver" -) - -// CompletionCommand creates shell completion scripts -type CompletionCommand struct { - *commands.BaseCommand - resolver.CompletionResolver -} - -// ExecuteSingleArgument implements commands.SingleArgumentCommand -func (s *CompletionCommand) ExecuteSingleArgument(_ commands.Executor, arg string) (output.Output, error) { - if arg == "bash" { - completion := new(bytes.Buffer) - err := s.Cobra().Root().GenBashCompletion(completion) - - return output.Raw(completion.Bytes()), err - } - - return nil, fmt.Errorf("completion for %s is not supported", arg) -} - -// DoesNotUseServices implements commands.OfflineCommand as this command does not use services -func (s *CompletionCommand) DoesNotUseServices() {} diff --git a/internal/completion/helpers.go b/internal/completion/helpers.go index 5ac40f754..805c7c356 100644 --- a/internal/completion/helpers.go +++ b/internal/completion/helpers.go @@ -1,7 +1,6 @@ package completion import ( - "fmt" "strings" ) @@ -9,22 +8,12 @@ import ( func MatchStringPrefix(vals []string, key string, caseSensitive bool) []string { var r []string key = strings.Trim(key, "'\"") - for _, v := range vals { if (caseSensitive && strings.HasPrefix(v, key)) || (!caseSensitive && strings.HasPrefix(strings.ToLower(v), strings.ToLower(key))) || key == "" { - r = append(r, Escape(v)) + r = append(r, v) } } return r } - -// Escape escapes a string according to completion rules (?) -// in effect, this means that the string will be quoted with double quotes if it contains a space or parentheses. -func Escape(s string) string { - if strings.ContainsAny(s, ` ()`) { - return fmt.Sprintf(`"%s"`, s) - } - return s -} diff --git a/internal/completion/helpers_test.go b/internal/completion/helpers_test.go index 8455a7c60..3d0aa3290 100644 --- a/internal/completion/helpers_test.go +++ b/internal/completion/helpers_test.go @@ -73,11 +73,11 @@ func TestMatchStringPrefix(t *testing.T) { expected: []string{"aba", "aBa", "Aba"}, }, { - name: "escaped output", + name: "output with special characters", vals: []string{"a a ", "a(0)", "aab", "a;<!`'", "bbb"}, key: "a", caseSensitive: false, - expected: []string{"\"a a \"", "\"a(0)\"", "aab", "a;<!`'"}, + expected: []string{"a a ", "a(0)", "aab", "a;<!`'"}, }, } { t.Run(test.name, func(t *testing.T) { @@ -90,41 +90,3 @@ func TestMatchStringPrefix(t *testing.T) { }) } } - -func TestEscape(t *testing.T) { - for _, test := range []struct { - name string - in string - expected string - }{ - { - name: "no escape", - in: "asdasdasd", - expected: "asdasdasd", - }, - { - name: "escape spaces", - in: "asdas dasd", - expected: "\"asdas dasd\"", - }, - { - name: "escape open parentheses", - in: "asdas(", - expected: "\"asdas(\"", - }, - { - name: "escape closed parentheses", - in: "asdas()", - expected: "\"asdas()\"", - }, - { - name: "special chars not escaped", - in: "a;<!`'", - expected: "a;<!`'", - }, - } { - t.Run(test.name, func(t *testing.T) { - assert.Equal(t, test.expected, completion.Escape(test.in)) - }) - } -} diff --git a/internal/core/core.go b/internal/core/core.go index 4918652e4..79533fb54 100644 --- a/internal/core/core.go +++ b/internal/core/core.go @@ -86,8 +86,6 @@ func BuildRootCmd(conf *config.Config) cobra.Command { }, } - rootCmd.BashCompletionFunction = commands.CustomBashCompletionFunc(rootCmd.Use) - flags := &pflag.FlagSet{} flags.StringVarP( &conf.GlobalFlags.ConfigFile, "config", "", "", "Configuration file path.", From 9c9ec8867c4775e5d77e58d894c95bcd32814db4 Mon Sep 17 00:00:00 2001 From: Toni Kangas <toni.kangas@upcloud.com> Date: Wed, 1 Feb 2023 16:13:32 +0200 Subject: [PATCH 2/5] fix(completion): remove word breaks from completions with whitespace --- internal/completion/helpers.go | 11 ++++++++++- internal/completion/helpers_test.go | 2 +- internal/resolver/database.go | 2 +- internal/resolver/matchers.go | 10 ++++++++++ internal/resolver/network.go | 2 +- internal/resolver/router.go | 2 +- internal/resolver/server.go | 2 +- internal/resolver/storage.go | 2 +- 8 files changed, 26 insertions(+), 7 deletions(-) create mode 100644 internal/resolver/matchers.go diff --git a/internal/completion/helpers.go b/internal/completion/helpers.go index 805c7c356..1d8dbfeaa 100644 --- a/internal/completion/helpers.go +++ b/internal/completion/helpers.go @@ -1,9 +1,18 @@ package completion import ( + "regexp" "strings" ) +// RemoveWordBreaks replaces all whitespaces in input strings with non-breaking spaces to prevent bash from splitting completion with whitespace into multiple completions. +// +// This hack allows us to use cobras built-in completion logic and can be removed once cobra supports whitespace in bash completions (See https://github.com/spf13/cobra/issues/1740). +func RemoveWordBreaks(input string) string { + re := regexp.MustCompile(`\s+`) + return re.ReplaceAllString(input, "\u00A0") +} + // MatchStringPrefix returns a list of string in vals which have a prefix as specified in key. Quotes are removed from key and output strings are escaped according to completion rules func MatchStringPrefix(vals []string, key string, caseSensitive bool) []string { var r []string @@ -12,7 +21,7 @@ func MatchStringPrefix(vals []string, key string, caseSensitive bool) []string { if (caseSensitive && strings.HasPrefix(v, key)) || (!caseSensitive && strings.HasPrefix(strings.ToLower(v), strings.ToLower(key))) || key == "" { - r = append(r, v) + r = append(r, RemoveWordBreaks(v)) } } return r diff --git a/internal/completion/helpers_test.go b/internal/completion/helpers_test.go index 3d0aa3290..380880fb3 100644 --- a/internal/completion/helpers_test.go +++ b/internal/completion/helpers_test.go @@ -77,7 +77,7 @@ func TestMatchStringPrefix(t *testing.T) { vals: []string{"a a ", "a(0)", "aab", "a;<!`'", "bbb"}, key: "a", caseSensitive: false, - expected: []string{"a a ", "a(0)", "aab", "a;<!`'"}, + expected: []string{"a\u00A0a\u00A0", "a(0)", "aab", "a;<!`'"}, }, } { t.Run(test.name, func(t *testing.T) { diff --git a/internal/resolver/database.go b/internal/resolver/database.go index 5bf4b963e..81dc7bcd9 100644 --- a/internal/resolver/database.go +++ b/internal/resolver/database.go @@ -22,7 +22,7 @@ func (s CachingDatabase) Get(ctx context.Context, svc internal.AllServices) (Res return func(arg string) (uuid string, err error) { rv := "" for _, db := range databases { - if db.Title == arg || db.UUID == arg { + if MatchArgWithWhitespace(arg, db.Title) || db.UUID == arg { if rv != "" { return "", AmbiguousResolutionError(arg) } diff --git a/internal/resolver/matchers.go b/internal/resolver/matchers.go new file mode 100644 index 000000000..b55ea7906 --- /dev/null +++ b/internal/resolver/matchers.go @@ -0,0 +1,10 @@ +package resolver + +import ( + "github.com/UpCloudLtd/upcloud-cli/v2/internal/completion" +) + +// MatchStringWithWhitespace checks if arg that may include whitespace matches given value. This checks both quoted args and auto-completed args handled with completion.RemoveWordBreaks. +func MatchArgWithWhitespace(arg string, value string) bool { + return completion.RemoveWordBreaks(value) == arg || value == arg +} diff --git a/internal/resolver/network.go b/internal/resolver/network.go index dd0622926..e2ef4b12a 100644 --- a/internal/resolver/network.go +++ b/internal/resolver/network.go @@ -21,7 +21,7 @@ func networkMatcher(cached []upcloud.Network) func(arg string) (uuid string, err return func(arg string) (uuid string, err error) { rv := "" for _, network := range cached { - if network.Name == arg || network.UUID == arg { + if MatchArgWithWhitespace(arg, network.Name) || network.UUID == arg { if rv != "" { return "", AmbiguousResolutionError(arg) } diff --git a/internal/resolver/router.go b/internal/resolver/router.go index 9a3a97fe1..9d5e8a8bc 100644 --- a/internal/resolver/router.go +++ b/internal/resolver/router.go @@ -27,7 +27,7 @@ func (s *CachingRouter) Get(ctx context.Context, svc internal.AllServices) (Reso return func(arg string) (uuid string, err error) { rv := "" for _, router := range s.cached { - if router.Name == arg || router.UUID == arg { + if MatchArgWithWhitespace(arg, router.Name) || router.UUID == arg { if rv != "" { return "", AmbiguousResolutionError(arg) } diff --git a/internal/resolver/server.go b/internal/resolver/server.go index 90933f633..c96cf0bc5 100644 --- a/internal/resolver/server.go +++ b/internal/resolver/server.go @@ -21,7 +21,7 @@ func (s CachingServer) Get(ctx context.Context, svc internal.AllServices) (Resol return func(arg string) (uuid string, err error) { rv := "" for _, server := range servers.Servers { - if server.Title == arg || server.Hostname == arg || server.UUID == arg { + if MatchArgWithWhitespace(arg, server.Title) || server.Hostname == arg || server.UUID == arg { if rv != "" { return "", AmbiguousResolutionError(arg) } diff --git a/internal/resolver/storage.go b/internal/resolver/storage.go index 0fc4d17a3..caf3a03b0 100644 --- a/internal/resolver/storage.go +++ b/internal/resolver/storage.go @@ -22,7 +22,7 @@ func storageMatcher(cached []upcloud.Storage) func(arg string) (uuid string, err return func(arg string) (uuid string, err error) { rv := "" for _, storage := range cached { - if storage.Title == arg || storage.UUID == arg { + if MatchArgWithWhitespace(arg, storage.Title) || storage.UUID == arg { if rv != "" { return "", AmbiguousResolutionError(arg) } From 681f537009842c84ddf28f777fba70a98b3e80ca Mon Sep 17 00:00:00 2001 From: Toni Kangas <toni.kangas@upcloud.com> Date: Wed, 1 Feb 2023 16:16:44 +0200 Subject: [PATCH 3/5] chore(changelog): add note about non-breaking spaces in completions --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d0b5f45d..093511318 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added support for all shell completions provided by `cobra`. ### Changed -- Remove custom bash completion logic and replace it with `completion` command provided by `cobra`. +- Remove custom bash completion logic and replace it with `completion` command provided by `cobra`. To do this while supporting args with whitespace, whitespace in completions is replaced with non-breaking spaces. ## [2.4.0] - 2022-12-19 ### Added From 4a06daeed16f12e587237d67fef6fff798f72fa0 Mon Sep 17 00:00:00 2001 From: Toni Kangas <toni.kangas@upcloud.com> Date: Wed, 1 Feb 2023 16:28:07 +0200 Subject: [PATCH 4/5] chore(dependencies): update cobra to `v1.6.1` --- go.mod | 4 ++-- go.sum | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 03774eaa3..bc2c2d371 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/jedib0t/go-pretty/v6 v6.3.3 github.com/m7shapan/cidr v0.0.0-20200427124835-7eba0889a5d2 github.com/mattn/go-isatty v0.0.16 - github.com/spf13/cobra v1.5.0 + github.com/spf13/cobra v1.6.1 github.com/spf13/pflag v1.0.5 github.com/spf13/viper v1.7.1 github.com/stretchr/testify v1.8.0 @@ -32,7 +32,7 @@ require ( github.com/hashicorp/go-cleanhttp v0.5.1 // indirect github.com/hashicorp/hcl v1.0.0 // indirect github.com/imdario/mergo v0.3.6 // indirect - github.com/inconshreveable/mousetrap v1.0.0 // indirect + github.com/inconshreveable/mousetrap v1.0.1 // indirect github.com/json-iterator/go v1.1.12 // indirect github.com/magiconair/properties v1.8.1 // indirect github.com/mattn/go-colorable v0.1.13 // indirect diff --git a/go.sum b/go.sum index 24e5fa1a5..46e7db82d 100644 --- a/go.sum +++ b/go.sum @@ -193,8 +193,8 @@ github.com/hashicorp/serf v0.8.2/go.mod h1:6hOLApaqBFA1NXqRQAsxw9QxuDEvNxSQRwA/J github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc= github.com/imdario/mergo v0.3.6 h1:xTNEAn+kxVO7dTZGu0CegyqKZmoWFI0rF8UxjlB2d28= github.com/imdario/mergo v0.3.6/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= -github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM= -github.com/inconshreveable/mousetrap v1.0.0/go.mod h1:PxqpIevigyE2G7u3NXJIT2ANytuPF1OarO4DADm73n8= +github.com/inconshreveable/mousetrap v1.0.1 h1:U3uMjPSQEBMNp1lFxmllqCPM6P5u/Xq7Pgzkat/bFNc= +github.com/inconshreveable/mousetrap v1.0.1/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/jedib0t/go-pretty/v6 v6.3.3 h1:shEWoyXqldeP54byATY3IczSfMC1b/UziOISaSxcvMQ= github.com/jedib0t/go-pretty/v6 v6.3.3/go.mod h1:MgmISkTWDSFu0xOqiZ0mKNntMQ2mDgOcwOkwBEkMDJI= github.com/jonboulle/clockwork v0.1.0/go.mod h1:Ii8DK3G1RaLaWxj9trq07+26W01tbo22gdxWY5EU2bo= @@ -294,8 +294,8 @@ github.com/spf13/afero v1.2.2 h1:5jhuqJyZCZf2JRofRvN/nIFgIWNzPa3/Vz8mYylgbWc= github.com/spf13/afero v1.2.2/go.mod h1:9ZxEEn6pIJ8Rxe320qSDBk6AsU0r9pR7Q4OcevTdifk= github.com/spf13/cast v1.3.0 h1:oget//CVOEoFewqQxwr0Ej5yjygnqGkvggSE/gB35Q8= github.com/spf13/cast v1.3.0/go.mod h1:Qx5cxh0v+4UWYiBimWS+eyWzqEqokIECu5etghLkUJE= -github.com/spf13/cobra v1.5.0 h1:X+jTBEBqF0bHN+9cSMgmfuvv2VHJ9ezmFNf9Y/XstYU= -github.com/spf13/cobra v1.5.0/go.mod h1:dWXEIy2H428czQCjInthrTRUg7yKbok+2Qi/yBIJoUM= +github.com/spf13/cobra v1.6.1 h1:o94oiPyS4KD1mPy2fmcYYHHfCxLqYjJOhGsCHFZtEzA= +github.com/spf13/cobra v1.6.1/go.mod h1:IOw/AERYS7UzyrGinqmz6HLUo219MORXGxhbaJUqzrY= github.com/spf13/jwalterweatherman v1.0.0 h1:XHEdyB+EcvlqZamSM4ZOMGlc93t6AcsBEu9Gc1vn7yk= github.com/spf13/jwalterweatherman v1.0.0/go.mod h1:cQK4TGJAtQXfYWX+Ddv3mKDzgVb68N+wFjFa4jdeBTo= github.com/spf13/pflag v1.0.3/go.mod h1:DYY7MBk1bdzusC3SYhjObp+wFpr4gzcvqqNjLnInEg4= From 7da143e853fa63005ff61bcc924b077b1f4590ba Mon Sep 17 00:00:00 2001 From: Toni Kangas <toni.kangas@upcloud.com> Date: Thu, 2 Feb 2023 13:23:15 +0200 Subject: [PATCH 5/5] refactor(completion): compile regexp outside of any loops --- internal/completion/helpers.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/completion/helpers.go b/internal/completion/helpers.go index 1d8dbfeaa..71655e5f7 100644 --- a/internal/completion/helpers.go +++ b/internal/completion/helpers.go @@ -5,12 +5,13 @@ import ( "strings" ) +var oneOrMoreWhitespace = regexp.MustCompile(`\s+`) + // RemoveWordBreaks replaces all whitespaces in input strings with non-breaking spaces to prevent bash from splitting completion with whitespace into multiple completions. // // This hack allows us to use cobras built-in completion logic and can be removed once cobra supports whitespace in bash completions (See https://github.com/spf13/cobra/issues/1740). func RemoveWordBreaks(input string) string { - re := regexp.MustCompile(`\s+`) - return re.ReplaceAllString(input, "\u00A0") + return oneOrMoreWhitespace.ReplaceAllString(input, "\u00A0") } // MatchStringPrefix returns a list of string in vals which have a prefix as specified in key. Quotes are removed from key and output strings are escaped according to completion rules