Repository: helm/helm
Pull request #8664 (2020-08-27 17:12:16)

feat(comp): Add support for fish completion
Part of #8109
Replaces #8111 
  
**What this PR does / why we need it**:

Provides support for Fish shell completion through Cobra

**Special notes for your reviewer**:

This PR replaces #8111 because the contributor has not come back to his PR, which needs a rebase, since July 7th, 2020.
I've reached out three times but no luck. 

Also, a new flag `--no-descriptions` was added just for the command `helm completion fish`, to allow users to disable descriptions if they don't like to have them.  Descriptions are a feature of fish completion (and native zsh completion) that looks like this:
```
$ bin/helm s<TAB>
search  (search for a keyword in charts)  
show    (show information of a chart)  
status  (display the status of the named release)
```


Patch:
diff --git a/cmd/helm/completion.go b/cmd/helm/completion.go
index c7e763c3d..275483f45 100644
--- a/cmd/helm/completion.go
+++ b/cmd/helm/completion.go
@@ -52,6 +52,25 @@ To load completions for every new session, execute once:
 $ helm completion zsh > "${fpath[1]}/_helm"
 `
 
+const fishCompDesc = `
+Generate the autocompletion script for Helm for the fish shell.
+
+To load completions in your current shell session:
+$ helm completion fish | source
+
+To load completions for every new session, execute once:
+$ helm completion fish > ~/.config/fish/completions/helm.fish
+
+You will need to start a new shell for this setup to take effect.
+`
+
+const (
+	noDescFlagName = "no-descriptions"
+	noDescFlagText = "disable completion descriptions"
+)
+
+var disableCompDescriptions bool
+
 func newCompletionCmd(out io.Writer) *cobra.Command {
 	cmd := &cobra.Command{
 		Use:               "completion",
@@ -85,7 +104,20 @@ func newCompletionCmd(out io.Writer) *cobra.Command {
 		},
 	}
 
-	cmd.AddCommand(bash, zsh)
+	fish := &cobra.Command{
+		Use:                   "fish",
+		Short:                 "generate autocompletions script for fish",
+		Long:                  fishCompDesc,
+		Args:                  require.NoArgs,
+		DisableFlagsInUseLine: true,
+		ValidArgsFunction:     noCompletions,
+		RunE: func(cmd *cobra.Command, args []string) error {
+			return runCompletionFish(out, cmd)
+		},
+	}
+	fish.Flags().BoolVar(&disableCompDescriptions, noDescFlagName, false, noDescFlagText)
+
+	cmd.AddCommand(bash, zsh, fish)
 
 	return cmd
 }
@@ -151,7 +183,7 @@ __helm_compgen() {
 	fi
 	for w in "${completions[@]}"; do
 		if [[ "${w}" = "$1"* ]]; then
-			# Use printf instead of echo beause it is possible that
+			# Use printf instead of echo because it is possible that
 			# the value to print is -n, which would be interpreted
 			# as a flag to echo
 			printf "%s\n" "${w}"
@@ -257,6 +289,10 @@ __helm_bash_source <(__helm_convert_bash_to_zsh)
 	return nil
 }
 
+func runCompletionFish(out io.Writer, cmd *cobra.Command) error {
+	return cmd.Root().GenFishCompletion(out, !disableCompDescriptions)
+}
+
 // Function to disable file completion
 func noCompletions(cmd *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) {
 	return nil, cobra.ShellCompDirectiveNoFileComp
diff --git a/cmd/helm/get_all.go b/cmd/helm/get_all.go
index a5037e4df..53f8d5905 100644
--- a/cmd/helm/get_all.go
+++ b/cmd/helm/get_all.go
@@ -59,7 +59,7 @@ func newGetAllCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 				return tpl(template, data, out)
 			}
 
-			return output.Table.Write(out, &statusPrinter{res, true})
+			return output.Table.Write(out, &statusPrinter{res, true, false})
 		},
 	}
 
diff --git a/cmd/helm/install.go b/cmd/helm/install.go
index f7d0239fb..7edd98091 100644
--- a/cmd/helm/install.go
+++ b/cmd/helm/install.go
@@ -122,7 +122,7 @@ func newInstallCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 				return err
 			}
 
-			return outfmt.Write(out, &statusPrinter{rel, settings.Debug})
+			return outfmt.Write(out, &statusPrinter{rel, settings.Debug, false})
 		},
 	}
 
diff --git a/cmd/helm/status.go b/cmd/helm/status.go
index abd32a9c2..7a3204cb9 100644
--- a/cmd/helm/status.go
+++ b/cmd/helm/status.go
@@ -39,6 +39,8 @@ The status consists of:
 - last deployment time
 - k8s namespace in which the release lives
 - state of the release (can be: unknown, deployed, uninstalled, superseded, failed, uninstalling, pending-install, pending-upgrade or pending-rollback)
+- revision of the release
+- description of the release (can be completion message or error message, need to enable --show-desc)
 - list of resources that this release consists of, sorted by kind
 - details on last test suite run, if applicable
 - additional notes provided by the chart
@@ -68,7 +70,7 @@ func newStatusCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 			// strip chart metadata from the output
 			rel.Chart = nil
 
-			return outfmt.Write(out, &statusPrinter{rel, false})
+			return outfmt.Write(out, &statusPrinter{rel, false, client.ShowDescription})
 		},
 	}
 
@@ -88,13 +90,15 @@ func newStatusCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	}
 
 	bindOutputFlag(cmd, &outfmt)
+	f.BoolVar(&client.ShowDescription, "show-desc", false, "if set, display the description message of the named release")
 
 	return cmd
 }
 
 type statusPrinter struct {
-	release *release.Release
-	debug   bool
+	release         *release.Release
+	debug           bool
+	showDescription bool
 }
 
 func (s statusPrinter) WriteJSON(out io.Writer) error {
@@ -116,6 +120,9 @@ func (s statusPrinter) WriteTable(out io.Writer) error {
 	fmt.Fprintf(out, "NAMESPACE: %s\n", s.release.Namespace)
 	fmt.Fprintf(out, "STATUS: %s\n", s.release.Info.Status.String())
 	fmt.Fprintf(out, "REVISION: %d\n", s.release.Version)
+	if s.showDescription {
+		fmt.Fprintf(out, "DESCRIPTION: %s\n", s.release.Info.Description)
+	}
 
 	executions := executionsByHookEvent(s.release)
 	if tests, ok := executions[release.HookTest]; !ok || len(tests) == 0 {
diff --git a/cmd/helm/upgrade.go b/cmd/helm/upgrade.go
index dbddaa368..12d797545 100644
--- a/cmd/helm/upgrade.go
+++ b/cmd/helm/upgrade.go
@@ -115,7 +115,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 					if err != nil {
 						return err
 					}
-					return outfmt.Write(out, &statusPrinter{rel, settings.Debug})
+					return outfmt.Write(out, &statusPrinter{rel, settings.Debug, false})
 				} else if err != nil {
 					return err
 				}
@@ -160,7 +160,7 @@ func newUpgradeCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 				fmt.Fprintf(out, "Release %q has been upgraded. Happy Helming!\n", args[0])
 			}
 
-			return outfmt.Write(out, &statusPrinter{rel, settings.Debug})
+			return outfmt.Write(out, &statusPrinter{rel, settings.Debug, false})
 		},
 	}
 
diff --git a/go.mod b/go.mod
index 66447ee3d..4b7c90f9d 100644
--- a/go.mod
+++ b/go.mod
@@ -5,6 +5,7 @@ go 1.13
 require (
 	github.com/BurntSushi/toml v0.3.1
 	github.com/DATA-DOG/go-sqlmock v1.4.1
+	github.com/Masterminds/goutils v1.1.0
 	github.com/Masterminds/semver/v3 v3.1.0
 	github.com/Masterminds/sprig/v3 v3.1.0
 	github.com/Masterminds/squirrel v1.4.0
diff --git a/pkg/action/status.go b/pkg/action/status.go
index a0c7f6e21..1c556e28d 100644
--- a/pkg/action/status.go
+++ b/pkg/action/status.go
@@ -27,6 +27,11 @@ type Status struct {
 	cfg *Configuration
 
 	Version int
+
+	// If true, display description to output format,
+	// only affect print type table.
+	// TODO Helm 4: Remove this flag and output the description by default.
+	ShowDescription bool
 }
 
 // NewStatus creates a new Status object with the given configuration.
diff --git a/pkg/action/upgrade.go b/pkg/action/upgrade.go
index e7c2aec25..b707e7e69 100644
--- a/pkg/action/upgrade.go
+++ b/pkg/action/upgrade.go
@@ -147,7 +147,8 @@ func validateReleaseName(releaseName string) error {
 		return errMissingRelease
 	}
 
-	if !ValidName.MatchString(releaseName) || (len(releaseName) > releaseNameMaxLen) {
+	// Check length first, since that is a less expensive operation.
+	if len(releaseName) > releaseNameMaxLen || !ValidName.MatchString(releaseName) {
 		return errInvalidName
 	}
 
diff --git a/pkg/lint/rules/template.go b/pkg/lint/rules/template.go
index 9a3a2a1ba..73d645264 100644
--- a/pkg/lint/rules/template.go
+++ b/pkg/lint/rules/template.go
@@ -17,6 +17,8 @@ limitations under the License.
 package rules
 
 import (
+	"bufio"
+	"bytes"
 	"fmt"
 	"os"
 	"path"
@@ -122,6 +124,7 @@ func Templates(linter *support.Linter, values map[string]interface{}, namespace
 
 		renderedContent := renderedContentMap[path.Join(chart.Name(), fileName)]
 		if strings.TrimSpace(renderedContent) != "" {
+			linter.RunLinterRule(support.WarningSev, fpath, validateTopIndentLevel(renderedContent))
 			var yamlStruct K8sYamlStruct
 			// Even though K8sYamlStruct only defines a few fields, an error in any other
 			// key will be raised as well
@@ -137,6 +140,32 @@ func Templates(linter *support.Linter, values map[string]interface{}, namespace
 	}
 }
 
+// validateTopIndentLevel checks that the content does not start with an indent level > 0.
+//
+// This error can occur when a template accidentally inserts space. It can cause
+// unpredictable errors dependening on whether the text is normalized before being passed
+// into the YAML parser. So we trap it here.
+//
+// See https://github.com/helm/helm/issues/8467
+func validateTopIndentLevel(content string) error {
+	// Read lines until we get to a non-empty one
+	scanner := bufio.NewScanner(bytes.NewBufferString(content))
+	for scanner.Scan() {
+		line := scanner.Text()
+		// If line is empty, skip
+		if strings.TrimSpace(line) == "" {
+			continue
+		}
+		// If it starts with one or more spaces, this is an error
+		if strings.HasPrefix(line, " ") || strings.HasPrefix(line, "\t") {
+			return fmt.Errorf("document starts with an illegal indent: %q, which may cause parsing problems", line)
+		}
+		// Any other condition passes.
+		return nil
+	}
+	return scanner.Err()
+}
+
 // Validation functions
 func validateTemplatesDir(templatesPath string) error {
 	if fi, err := os.Stat(templatesPath); err != nil {
@@ -165,6 +194,9 @@ func validateYamlContent(err error) error {
 }
 
 func validateMetadataName(obj *K8sYamlStruct) error {
+	if len(obj.Metadata.Name) == 0 || len(obj.Metadata.Name) > 253 {
+		return fmt.Errorf("object name must be between 0 and 253 characters: %q", obj.Metadata.Name)
+	}
 	// This will return an error if the characters do not abide by the standard OR if the
 	// name is left empty.
 	if validName.MatchString(obj.Metadata.Name) {


Test patch:
diff --git a/cmd/helm/completion_test.go b/cmd/helm/completion_test.go
index 44c74f85a..7eee39832 100644
--- a/cmd/helm/completion_test.go
+++ b/cmd/helm/completion_test.go
@@ -55,4 +55,5 @@ func TestCompletionFileCompletion(t *testing.T) {
 	checkFileCompletion(t, "completion", false)
 	checkFileCompletion(t, "completion bash", false)
 	checkFileCompletion(t, "completion zsh", false)
+	checkFileCompletion(t, "completion fish", false)
 }
diff --git a/cmd/helm/release_testing.go b/cmd/helm/release_testing.go
index 37cac6186..e4e09ef3b 100644
--- a/cmd/helm/release_testing.go
+++ b/cmd/helm/release_testing.go
@@ -61,7 +61,7 @@ func newReleaseTestCmd(cfg *action.Configuration, out io.Writer) *cobra.Command
 				return runErr
 			}
 
-			if err := outfmt.Write(out, &statusPrinter{rel, settings.Debug}); err != nil {
+			if err := outfmt.Write(out, &statusPrinter{rel, settings.Debug, false}); err != nil {
 				return err
 			}
 
diff --git a/cmd/helm/status_test.go b/cmd/helm/status_test.go
index 18731e465..280f486a3 100644
--- a/cmd/helm/status_test.go
+++ b/cmd/helm/status_test.go
@@ -44,6 +44,14 @@ func TestStatusCmd(t *testing.T) {
 		rels: releasesMockWithStatus(&release.Info{
 			Status: release.StatusDeployed,
 		}),
+	}, {
+		name:   "get status of a deployed release, with desc",
+		cmd:    "status --show-desc flummoxed-chickadee",
+		golden: "output/status-with-desc.txt",
+		rels: releasesMockWithStatus(&release.Info{
+			Status:      release.StatusDeployed,
+			Description: "Mock description",
+		}),
 	}, {
 		name:   "get status of a deployed release with notes",
 		cmd:    "status flummoxed-chickadee",
diff --git a/cmd/helm/testdata/output/status-with-desc.txt b/cmd/helm/testdata/output/status-with-desc.txt
new file mode 100644
index 000000000..c681fe3ec
--- /dev/null
+++ b/cmd/helm/testdata/output/status-with-desc.txt
@@ -0,0 +1,7 @@
+NAME: flummoxed-chickadee
+LAST DEPLOYED: Sat Jan 16 00:00:00 2016
+NAMESPACE: default
+STATUS: deployed
+REVISION: 0
+DESCRIPTION: Mock description
+TEST SUITE: None
diff --git a/pkg/chart/loader/load_test.go b/pkg/chart/loader/load_test.go
index 40b86dec2..16a94d4eb 100644
--- a/pkg/chart/loader/load_test.go
+++ b/pkg/chart/loader/load_test.go
@@ -310,7 +310,7 @@ func TestLoadInvalidArchive(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	defer os.Remove(tmpdir)
+	defer os.RemoveAll(tmpdir)
 
 	writeTar := func(filename, internalPath string, body []byte) {
 		dest, err := os.Create(filename)
diff --git a/pkg/lint/rules/template_test.go b/pkg/lint/rules/template_test.go
index ae82c8922..b4397851b 100644
--- a/pkg/lint/rules/template_test.go
+++ b/pkg/lint/rules/template_test.go
@@ -22,6 +22,8 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/Masterminds/goutils"
+
 	"helm.sh/helm/v3/internal/test/ensure"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chartutil"
@@ -119,6 +121,14 @@ func TestValidateMetadataName(t *testing.T) {
 		"a..b":                      false,
 		"%^&#$%*@^*@&#^":            false,
 	}
+
+	// The length checker should catch this first. So this is not true fuzzing.
+	tooLong, err := goutils.RandomAlphaNumeric(300)
+	if err != nil {
+		t.Fatalf("Randomizer failed to initialize: %s", err)
+	}
+	names[tooLong] = false
+
 	for input, expectPass := range names {
 		obj := K8sYamlStruct{Metadata: k8sYamlMetadata{Name: input}}
 		if err := validateMetadataName(&obj); (err == nil) != expectPass {
@@ -305,3 +315,20 @@ spec:
 		t.Error("expected Deployment with no selector to fail")
 	}
 }
+
+func TestValidateTopIndentLevel(t *testing.T) {
+	for doc, shouldFail := range map[string]bool{
+		// Should not fail
+		"\n\n\n\t\n   \t\n":          false,
+		"apiVersion:foo\n  bar:baz":  false,
+		"\n\n\napiVersion:foo\n\n\n": false,
+		// Should fail
+		"  apiVersion:foo":         true,
+		"\n\n  apiVersion:foo\n\n": true,
+	} {
+		if err := validateTopIndentLevel(doc); (err == nil) == shouldFail {
+			t.Errorf("Expected %t for %q", shouldFail, doc)
+		}
+	}
+
+}