Repository: helm/helm
Pull request #9363 (2021-02-15 04:11:20)

feat(comp): Improve completion for plugin commands
**What this PR does / why we need it**:

The `plugin update` and `plugin uninstall` commands can accept more than one plugin name as arguments; this commit teaches the completion logic to respect that.

**Special notes for your reviewer**:

The logic to not suggest a plugin that is already on the command-line is the same as the logic for completion of the `helm repo rm` command which also takes multiple arguments. See https://github.com/helm/helm/blob/64c8df187af892c2a9f872f91e892a717df2e7ff/cmd/helm/repo_list.go#L103

**If applicable**:
- [x] this PR contains unit tests



Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 308154af5..a67d95c9e 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -191,7 +191,9 @@ below.
       issue to a milestone until the questions are answered.
     - We attempt to do this process at least once per work day.
 3. Discussion
-    - issues that are labeled as `feature` or `bug` should be connected to the PR that resolves it.
+    - Issues that are labeled `feature` or `proposal` must write a Helm Improvement Proposal (HIP).
+      See [Proposing an Idea](#proposing-an-idea). Smaller quality-of-life enhancements are exempt.
+    - Issues that are labeled as `feature` or `bug` should be connected to the PR that resolves it.
     - Whoever is working on a `feature` or `bug` issue (whether a maintainer or someone from the
       community), should either assign the issue to themself or make a comment in the issue saying
       that they are taking it.
@@ -200,9 +202,30 @@ below.
       and reduce noise. Should the issue need to stay open, the `keep open` label can be added.
 4. Issue closure
 
+## Proposing an Idea
+
+Before proposing a new idea to the Helm project, please make sure to write up a [Helm Improvement
+Proposal](https://github.com/helm/community/tree/master/hips). A Helm Improvement Proposal is a
+design document that describes a new feature for the Helm project. The proposal should provide a
+concise technical specification and rationale for the feature.
+
+It is also worth considering vetting your idea with the community via the
+[cncf-helm](mailto:cncf-helm@lists.cncf.io) mailing list. Vetting an idea publicly before going as
+far as writing a proposal is meant to save the potential author time. Many ideas have been proposed;
+it's quite likely there are others in the community who may be working on a similar proposal, or a
+similar proposal may have already been written.
+
+HIPs are submitted to the [helm/community repository](https://github.com/helm/community). [HIP
+1](https://github.com/helm/community/blob/master/hips/hip-0001.md) describes the process to write a
+HIP as well as the review process.
+
+After your proposal has been approved, follow the [developer's
+guide](https://helm.sh/docs/community/developers/) to get started.
+
 ## How to Contribute a Patch
 
-1. Identify or create the related issue.
+1. Identify or create the related issue. If you're proposing a larger change to
+   Helm, see [Proposing an Idea](#proposing-an-idea).
 2. Fork the desired repo; develop and test your code changes.
 3. Submit a pull request, making sure to sign your work and link the related issue.
 
diff --git a/cmd/helm/completion.go b/cmd/helm/completion.go
index f4c8a63f5..5aea0592e 100644
--- a/cmd/helm/completion.go
+++ b/cmd/helm/completion.go
@@ -92,12 +92,11 @@ func newCompletionCmd(out io.Writer) *cobra.Command {
 	}
 
 	zsh := &cobra.Command{
-		Use:                   "zsh",
-		Short:                 "generate autocompletion script for zsh",
-		Long:                  zshCompDesc,
-		Args:                  require.NoArgs,
-		DisableFlagsInUseLine: true,
-		ValidArgsFunction:     noCompletions,
+		Use:               "zsh",
+		Short:             "generate autocompletion script for zsh",
+		Long:              zshCompDesc,
+		Args:              require.NoArgs,
+		ValidArgsFunction: noCompletions,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return runCompletionZsh(out, cmd)
 		},
@@ -105,12 +104,11 @@ func newCompletionCmd(out io.Writer) *cobra.Command {
 	zsh.Flags().BoolVar(&disableCompDescriptions, noDescFlagName, false, noDescFlagText)
 
 	fish := &cobra.Command{
-		Use:                   "fish",
-		Short:                 "generate autocompletion script for fish",
-		Long:                  fishCompDesc,
-		Args:                  require.NoArgs,
-		DisableFlagsInUseLine: true,
-		ValidArgsFunction:     noCompletions,
+		Use:               "fish",
+		Short:             "generate autocompletion script for fish",
+		Long:              fishCompDesc,
+		Args:              require.NoArgs,
+		ValidArgsFunction: noCompletions,
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return runCompletionFish(out, cmd)
 		},
diff --git a/cmd/helm/plugin_list.go b/cmd/helm/plugin_list.go
index 6503161e8..eb4e5c815 100644
--- a/cmd/helm/plugin_list.go
+++ b/cmd/helm/plugin_list.go
@@ -51,12 +51,37 @@ func newPluginListCmd(out io.Writer) *cobra.Command {
 	return cmd
 }
 
+// Returns all plugins from plugins, except those with names matching ignoredPluginNames
+func filterPlugins(plugins []*plugin.Plugin, ignoredPluginNames []string) []*plugin.Plugin {
+	// if ignoredPluginNames is nil, just return plugins
+	if ignoredPluginNames == nil {
+		return plugins
+	}
+
+	var filteredPlugins []*plugin.Plugin
+	for _, plugin := range plugins {
+		found := false
+		for _, ignoredName := range ignoredPluginNames {
+			if plugin.Metadata.Name == ignoredName {
+				found = true
+				break
+			}
+		}
+		if !found {
+			filteredPlugins = append(filteredPlugins, plugin)
+		}
+	}
+
+	return filteredPlugins
+}
+
 // Provide dynamic auto-completion for plugin names
-func compListPlugins(toComplete string) []string {
+func compListPlugins(toComplete string, ignoredPluginNames []string) []string {
 	var pNames []string
 	plugins, err := plugin.FindPlugins(settings.PluginsDirectory)
-	if err == nil {
-		for _, p := range plugins {
+	if err == nil && len(plugins) > 0 {
+		filteredPlugins := filterPlugins(plugins, ignoredPluginNames)
+		for _, p := range filteredPlugins {
 			if strings.HasPrefix(p.Metadata.Name, toComplete) {
 				pNames = append(pNames, p.Metadata.Name)
 			}
diff --git a/cmd/helm/plugin_uninstall.go b/cmd/helm/plugin_uninstall.go
index b2290fb9b..ee4a47beb 100644
--- a/cmd/helm/plugin_uninstall.go
+++ b/cmd/helm/plugin_uninstall.go
@@ -39,10 +39,7 @@ func newPluginUninstallCmd(out io.Writer) *cobra.Command {
 		Aliases: []string{"rm", "remove"},
 		Short:   "uninstall one or more Helm plugins",
 		ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
-			if len(args) != 0 {
-				return nil, cobra.ShellCompDirectiveNoFileComp
-			}
-			return compListPlugins(toComplete), cobra.ShellCompDirectiveNoFileComp
+			return compListPlugins(toComplete, args), cobra.ShellCompDirectiveNoFileComp
 		},
 		PreRunE: func(cmd *cobra.Command, args []string) error {
 			return o.complete(args)
diff --git a/cmd/helm/plugin_update.go b/cmd/helm/plugin_update.go
index c46444e0d..4515acdbb 100644
--- a/cmd/helm/plugin_update.go
+++ b/cmd/helm/plugin_update.go
@@ -40,10 +40,7 @@ func newPluginUpdateCmd(out io.Writer) *cobra.Command {
 		Aliases: []string{"up"},
 		Short:   "update one or more Helm plugins",
 		ValidArgsFunction: func(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
-			if len(args) != 0 {
-				return nil, cobra.ShellCompDirectiveNoFileComp
-			}
-			return compListPlugins(toComplete), cobra.ShellCompDirectiveNoFileComp
+			return compListPlugins(toComplete, args), cobra.ShellCompDirectiveNoFileComp
 		},
 		PreRunE: func(cmd *cobra.Command, args []string) error {
 			return o.complete(args)
diff --git a/pkg/storage/storage.go b/pkg/storage/storage.go
index 2dfa3f615..cfc0d0deb 100644
--- a/pkg/storage/storage.go
+++ b/pkg/storage/storage.go
@@ -61,7 +61,10 @@ func (s *Storage) Create(rls *rspb.Release) error {
 	s.Log("creating release %q", makeKey(rls.Name, rls.Version))
 	if s.MaxHistory > 0 {
 		// Want to make space for one more release.
-		s.removeLeastRecent(rls.Name, s.MaxHistory-1)
+		if err := s.removeLeastRecent(rls.Name, s.MaxHistory-1); err != nil &&
+			!errors.Is(err, driver.ErrReleaseNotFound) {
+			return err
+		}
 	}
 	return s.Driver.Create(makeKey(rls.Name, rls.Version), rls)
 }
diff --git a/scripts/get b/scripts/get
index 777a53bbc..a8ec3c6d9 100755
--- a/scripts/get
+++ b/scripts/get
@@ -50,13 +50,11 @@ initOS() {
 
 # runs the given command as root (detects if we are root already)
 runAsRoot() {
-  local CMD="$*"
-
-  if [ $EUID -ne 0 -a $USE_SUDO = "true" ]; then
-    CMD="sudo $CMD"
+  if [ $EUID -ne 0 -a "$USE_SUDO" = "true" ]; then
+    sudo "${@}"
+  else
+    "${@}"
   fi
-
-  $CMD
 }
 
 # verifySupported checks that the os/arch combination is supported for
diff --git a/scripts/get-helm-3 b/scripts/get-helm-3
index 7afa9bae9..97ebe2435 100755
--- a/scripts/get-helm-3
+++ b/scripts/get-helm-3
@@ -57,13 +57,11 @@ initOS() {
 
 # runs the given command as root (detects if we are root already)
 runAsRoot() {
-  local CMD="$*"
-
-  if [ $EUID -ne 0 -a $USE_SUDO = "true" ]; then
-    CMD="sudo $CMD"
+  if [ $EUID -ne 0 -a "$USE_SUDO" = "true" ]; then
+    sudo "${@}"
+  else
+    "${@}"
   fi
-
-  $CMD
 }
 
 # verifySupported checks that the os/arch combination is supported for


Test patch:
diff --git a/cmd/helm/plugin_test.go b/cmd/helm/plugin_test.go
index 0bf867f2a..87fd17681 100644
--- a/cmd/helm/plugin_test.go
+++ b/cmd/helm/plugin_test.go
@@ -305,6 +305,50 @@ func TestLoadPlugins_HelmNoPlugins(t *testing.T) {
 	}
 }
 
+func TestPluginCmdsCompletion(t *testing.T) {
+
+	tests := []cmdTestCase{{
+		name:   "completion for plugin update",
+		cmd:    "__complete plugin update ''",
+		golden: "output/plugin_list_comp.txt",
+		rels:   []*release.Release{},
+	}, {
+		name:   "completion for plugin update repetition",
+		cmd:    "__complete plugin update args ''",
+		golden: "output/plugin_repeat_comp.txt",
+		rels:   []*release.Release{},
+	}, {
+		name:   "completion for plugin uninstall",
+		cmd:    "__complete plugin uninstall ''",
+		golden: "output/plugin_list_comp.txt",
+		rels:   []*release.Release{},
+	}, {
+		name:   "completion for plugin uninstall repetition",
+		cmd:    "__complete plugin uninstall args ''",
+		golden: "output/plugin_repeat_comp.txt",
+		rels:   []*release.Release{},
+	}, {
+		name:   "completion for plugin list",
+		cmd:    "__complete plugin list ''",
+		golden: "output/empty_nofile_comp.txt",
+		rels:   []*release.Release{},
+	}, {
+		name:   "completion for plugin install no args",
+		cmd:    "__complete plugin install ''",
+		golden: "output/empty_default_comp.txt",
+		rels:   []*release.Release{},
+	}, {
+		name:   "completion for plugin install one arg",
+		cmd:    "__complete plugin list /tmp ''",
+		golden: "output/empty_nofile_comp.txt",
+		rels:   []*release.Release{},
+	}, {}}
+	for _, test := range tests {
+		settings.PluginsDirectory = "testdata/helmhome/helm/plugins"
+		runTestCmd(t, []cmdTestCase{test})
+	}
+}
+
 func TestPluginFileCompletion(t *testing.T) {
 	checkFileCompletion(t, "plugin", false)
 }
diff --git a/cmd/helm/testdata/output/empty_default_comp.txt b/cmd/helm/testdata/output/empty_default_comp.txt
new file mode 100644
index 000000000..879d50d0e
--- /dev/null
+++ b/cmd/helm/testdata/output/empty_default_comp.txt
@@ -0,0 +1,2 @@
+:0
+Completion ended with directive: ShellCompDirectiveDefault
diff --git a/cmd/helm/testdata/output/empty_nofile_comp.txt b/cmd/helm/testdata/output/empty_nofile_comp.txt
new file mode 100644
index 000000000..8d9fad576
--- /dev/null
+++ b/cmd/helm/testdata/output/empty_nofile_comp.txt
@@ -0,0 +1,2 @@
+:4
+Completion ended with directive: ShellCompDirectiveNoFileComp
diff --git a/cmd/helm/testdata/output/plugin_list_comp.txt b/cmd/helm/testdata/output/plugin_list_comp.txt
new file mode 100644
index 000000000..e33b6ce52
--- /dev/null
+++ b/cmd/helm/testdata/output/plugin_list_comp.txt
@@ -0,0 +1,7 @@
+args
+echo
+env
+exitwith
+fullenv
+:4
+Completion ended with directive: ShellCompDirectiveNoFileComp
diff --git a/cmd/helm/testdata/output/plugin_repeat_comp.txt b/cmd/helm/testdata/output/plugin_repeat_comp.txt
new file mode 100644
index 000000000..9e2ee56ab
--- /dev/null
+++ b/cmd/helm/testdata/output/plugin_repeat_comp.txt
@@ -0,0 +1,6 @@
+echo
+env
+exitwith
+fullenv
+:4
+Completion ended with directive: ShellCompDirectiveNoFileComp
diff --git a/internal/test/test.go b/internal/test/test.go
index 480b466ee..b0f624521 100644
--- a/internal/test/test.go
+++ b/internal/test/test.go
@@ -77,6 +77,7 @@ func path(filename string) string {
 }
 
 func compare(actual []byte, filename string) error {
+	actual = normalize(actual)
 	if err := update(filename, actual); err != nil {
 		return err
 	}
@@ -85,6 +86,7 @@ func compare(actual []byte, filename string) error {
 	if err != nil {
 		return errors.Wrapf(err, "unable to read testdata %s", filename)
 	}
+	expected = normalize(expected)
 	if !bytes.Equal(expected, actual) {
 		return errors.Errorf("does not match golden file %s\n\nWANT:\n'%s'\n\nGOT:\n'%s'\n", filename, expected, actual)
 	}
diff --git a/pkg/storage/storage_test.go b/pkg/storage/storage_test.go
index 934a3842c..ee14962ed 100644
--- a/pkg/storage/storage_test.go
+++ b/pkg/storage/storage_test.go
@@ -21,6 +21,8 @@ import (
 	"reflect"
 	"testing"
 
+	"github.com/pkg/errors"
+
 	rspb "helm.sh/helm/v3/pkg/release"
 	"helm.sh/helm/v3/pkg/storage/driver"
 )
@@ -276,6 +278,32 @@ func TestStorageHistory(t *testing.T) {
 	}
 }
 
+func TestStorageRemoveLeastRecentWithError(t *testing.T) {
+	storage := Init(driver.NewMemory())
+	storage.Log = t.Logf
+
+	storage.MaxHistory = 1
+
+	const name = "angry-bird"
+
+	// setup storage with test releases
+	setup := func() {
+		// release records
+		rls1 := ReleaseTestData{Name: name, Version: 1, Status: rspb.StatusSuperseded}.ToRelease()
+
+		// create the release records in the storage
+		assertErrNil(t.Fatal, storage.Driver.Create(makeKey(rls1.Name, rls1.Version), rls1), "Storing release 'angry-bird' (v1)")
+	}
+	setup()
+
+	rls2 := ReleaseTestData{Name: name, Version: 2, Status: rspb.StatusSuperseded}.ToRelease()
+	wantErr := driver.ErrNoDeployedReleases
+	gotErr := storage.Create(rls2)
+	if !errors.Is(gotErr, wantErr) {
+		t.Fatalf("Storing release 'angry-bird' (v2) should return the error %#v, but returned %#v", wantErr, gotErr)
+	}
+}
+
 func TestStorageRemoveLeastRecent(t *testing.T) {
 	storage := Init(driver.NewMemory())
 	storage.Log = t.Logf