feat(completion): use cobras default completion command (#204)

This requires removing whitespace from completions as cobras bash completions do not support values with whitespace (See spf13/cobra#1740). This is done by replacing whitespace with non-breaking spaces during completion generation. The argument resolvers with values that can include whitespace are also updated to handle both modified and original arguments, i.e. user can still input whitespaces normally when argument is quoted or escaped.
UpCloudLtd · Feb 6, 2023 · 0f8c91d · 0f8c91d
1 parent 96c3173
commit 0f8c91d
Show file tree

Hide file tree

Showing 15 changed files with 38 additions and 184 deletions.
diff --git 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`. To do this while supporting args with whitespace, whitespace in completions is replaced with non-breaking spaces.
 
 ## [2.4.0] - 2022-12-19
 ### Added

diff --git 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
@@ -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=

diff --git 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
diff --git a/internal/commands/root/completion.go b/internal/commands/root/completion.go
diff --git a/internal/completion/helpers.go b/internal/completion/helpers.go
@@ -1,30 +1,29 @@
 package completion
 
 import (
-	"fmt"
+	"regexp"
 	"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 {
+	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
 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, RemoveWordBreaks(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
@@ -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\u00A0a\u00A0", "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
@@ -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.",

diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
 				}