Repository: helm/helm
Pull request #6669 (2019-10-15 14:19:02)

ref(pkg/chartutil): Improve error handling code in Save
This PR is intended to make the code more understandable, to prevent future mistakes and confusion. See the commit message for details.

bacongobbler: [Effective Go convention](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) has described using short variable names: `err` instead of `errCreatingFile`. If the variable's original intent is unclear then it should be made more descriptive. However, since there is no difference in handling between `errCreatingFile`, `errInChartYaml` and `errInValuesYaml`, `err` is more than descriptive enough.

My other issue with this PR is that some of these variable names are misleading. `errCreatingFile` doesn't always mean `os.Create` failed to create the file. It can also occur because `os.Create` failed to truncate the file that already existed. By giving a variable a "descriptive" error name can exclude certain cases that the developer may miss because they inferred the intent based on the varaible name.

I'm 👎 on this change because it goes against the current convention of Effective Go, and it doesn't actually change the behaviour of anything other than a few variable names.
SimonAlling: @bacongobbler Aha. I must admit I didn't know about the Effective Go convention. As for `errCreatingFile`, I picked that name based on the name of `os.Create` without further ado. I definitely see your point there.

Variable names aside, please do note that the current code did actually cause a pretty serious bug where most errors were silently ignored (which I semi-accidentally [fixed](https://github.com/helm/helm/pull/6519/files#diff-9ea0c476e7eb107c6b0f198af1a49caaR136) as part of #6519) – a bug that would never have occurred had each error variable either been more locally scoped (not possible, AFAICT) or named differently. If you take a look at [lines 125–128](https://github.com/helm/helm/blob/38d4f33a0fc1829765ee720327ed29c370d68a79/pkg/chartutil/save.go#L125:L128) before #6519, you can probably see how said bug was caused by how the `err` variable is named and used. The purpose of this PR is to prevent similar mistakes in the future.

Even if you don't want to change any names, I absolutely think the last line of `Save` should be changed from `return filename, err` to `return filename, nil`. That would certainly decrease the risk of introducing similar bugs again. Anyway, it doesn't really make sense at that point to return the `err` variable declared and assigned only once back in line 112, and already known to be `nil`.

What do you think?

Patch:
diff --git a/cmd/helm/get.go b/cmd/helm/get.go
index bfb8c2522..b9b38524d 100644
--- a/cmd/helm/get.go
+++ b/cmd/helm/get.go
@@ -23,27 +23,51 @@ import (
 
 	"helm.sh/helm/v3/cmd/helm/require"
 	"helm.sh/helm/v3/pkg/action"
+	"helm.sh/helm/v3/pkg/cli/output"
 )
 
 var getHelp = `
-This command consists of multiple subcommands which can be used to
-get extended information about the release, including:
+This command shows the details of a named release.
+
+It can be used to get extended information about the release, including:
 
   - The values used to generate the release
+  - The chart used to generate the release
   - The generated manifest file
-  - The notes provided by the chart of the release
-  - The hooks associated with the release
+
+By default, this prints a human readable collection of information about the
+chart, the supplied values, and the generated manifest file.
 `
 
 func newGetCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
+	var template string
+	client := action.NewGet(cfg)
+
 	cmd := &cobra.Command{
-		Use:   "get",
-		Short: "download extended information of a named release",
+		Use:   "get RELEASE_NAME",
+		Short: "download a named release",
 		Long:  getHelp,
-		Args:  require.NoArgs,
+		Args:  require.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			res, err := client.Run(args[0])
+			if err != nil {
+				return err
+			}
+			if template != "" {
+				data := map[string]interface{}{
+					"Release": res,
+				}
+				return tpl(template, data, out)
+			}
+
+			return output.Table.Write(out, &statusPrinter{res, true})
+		},
 	}
 
-	cmd.AddCommand(newGetAllCmd(cfg, out))
+	f := cmd.Flags()
+	f.IntVar(&client.Version, "revision", 0, "get the named release with revision")
+	f.StringVar(&template, "template", "", "go template for formatting the output, eg: {{.Release.Name}}")
+
 	cmd.AddCommand(newGetValuesCmd(cfg, out))
 	cmd.AddCommand(newGetManifestCmd(cfg, out))
 	cmd.AddCommand(newGetHooksCmd(cfg, out))
diff --git a/cmd/helm/get_all.go b/cmd/helm/get_all.go
deleted file mode 100644
index 8e9ab4d6b..000000000
--- a/cmd/helm/get_all.go
+++ /dev/null
@@ -1,64 +0,0 @@
-/*
-Copyright The Helm Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package main
-
-import (
-	"io"
-
-	"github.com/spf13/cobra"
-
-	"helm.sh/helm/v3/cmd/helm/require"
-	"helm.sh/helm/v3/pkg/action"
-	"helm.sh/helm/v3/pkg/cli/output"
-)
-
-var getAllHelp = `
-This command prints a human readable collection of information about the
-notes, hooks, supplied values, and generated manifest file of the given release.
-`
-
-func newGetAllCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
-	var template string
-	client := action.NewGet(cfg)
-
-	cmd := &cobra.Command{
-		Use:   "all RELEASE_NAME",
-		Short: "download all information for a named release",
-		Long:  getAllHelp,
-		Args:  require.ExactArgs(1),
-		RunE: func(cmd *cobra.Command, args []string) error {
-			res, err := client.Run(args[0])
-			if err != nil {
-				return err
-			}
-			if template != "" {
-				data := map[string]interface{}{
-					"Release": res,
-				}
-				return tpl(template, data, out)
-			}
-
-			return output.Table.Write(out, &statusPrinter{res, true})
-		},
-	}
-
-	f := cmd.Flags()
-	f.IntVar(&client.Version, "revision", 0, "get the named release with revision")
-	f.StringVar(&template, "template", "", "go template for formatting the output, eg: {{.Release.Name}}")
-
-	return cmd
-}
diff --git a/cmd/helm/get_notes.go b/cmd/helm/get_notes.go
index 1b0128989..feab4e303 100644
--- a/cmd/helm/get_notes.go
+++ b/cmd/helm/get_notes.go
@@ -34,8 +34,8 @@ func newGetNotesCmd(cfg *action.Configuration, out io.Writer) *cobra.Command {
 	client := action.NewGet(cfg)
 
 	cmd := &cobra.Command{
-		Use:   "notes RELEASE_NAME",
-		Short: "download the notes for a named release",
+		Use:   "notes [flags] RELEASE_NAME",
+		Short: "displays the notes of the named release",
 		Long:  getNotesHelp,
 		Args:  require.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
diff --git a/cmd/helm/show.go b/cmd/helm/show.go
index e9e9cca8b..4c2d42815 100644
--- a/cmd/helm/show.go
+++ b/cmd/helm/show.go
@@ -27,12 +27,10 @@ import (
 )
 
 const showDesc = `
-This command consists of multiple subcommands to display information about a chart
-`
+This command inspects a chart and displays information. It takes a chart reference
+('example/drupal'), a full path to a directory or packaged chart, or a URL.
 
-const showAllDesc = `
-This command inspects a chart (directory, file, or URL) and displays all its content
-(values.yaml, Charts.yaml, README)
+Inspect prints the contents of the Chart.yaml file and the values.yaml file.
 `
 
 const showValuesDesc = `
@@ -54,20 +52,12 @@ func newShowCmd(out io.Writer) *cobra.Command {
 	client := action.NewShow(action.ShowAll)
 
 	showCommand := &cobra.Command{
-		Use:     "show",
-		Short:   "show information of a chart",
+		Use:     "show [CHART]",
+		Short:   "inspect a chart",
 		Aliases: []string{"inspect"},
 		Long:    showDesc,
-		Args:    require.NoArgs,
-	}
-
-	all := &cobra.Command{
-		Use:   "all [CHART]",
-		Short: "shows all information of the chart",
-		Long:  showAllDesc,
-		Args:  require.ExactArgs(1),
+		Args:    require.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			client.OutputFormat = action.ShowAll
 			cp, err := client.ChartPathOptions.LocateChart(args[0], settings)
 			if err != nil {
 				return err
@@ -83,7 +73,7 @@ func newShowCmd(out io.Writer) *cobra.Command {
 
 	valuesSubCmd := &cobra.Command{
 		Use:   "values [CHART]",
-		Short: "shows the chart's values",
+		Short: "shows values for this chart",
 		Long:  showValuesDesc,
 		Args:  require.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -103,7 +93,7 @@ func newShowCmd(out io.Writer) *cobra.Command {
 
 	chartSubCmd := &cobra.Command{
 		Use:   "chart [CHART]",
-		Short: "shows the chart's definition",
+		Short: "shows the chart",
 		Long:  showChartDesc,
 		Args:  require.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -141,9 +131,12 @@ func newShowCmd(out io.Writer) *cobra.Command {
 		},
 	}
 
-	cmds := []*cobra.Command{all, readmeSubCmd, valuesSubCmd, chartSubCmd}
+	cmds := []*cobra.Command{showCommand, readmeSubCmd, valuesSubCmd, chartSubCmd}
 	for _, subCmd := range cmds {
 		addChartPathOptionsFlags(subCmd.Flags(), &client.ChartPathOptions)
+	}
+
+	for _, subCmd := range cmds[1:] {
 		showCommand.AddCommand(subCmd)
 	}
 
diff --git a/pkg/chartutil/errors.go b/pkg/chartutil/errors.go
index fcdcc27ea..061610d41 100644
--- a/pkg/chartutil/errors.go
+++ b/pkg/chartutil/errors.go
@@ -21,15 +21,11 @@ import (
 )
 
 // ErrNoTable indicates that a chart does not have a matching table.
-type ErrNoTable struct {
-	Key string
-}
+type ErrNoTable string
 
-func (e ErrNoTable) Error() string { return fmt.Sprintf("%q is not a table", e.Key) }
+func (e ErrNoTable) Error() string { return fmt.Sprintf("%q is not a table", e) }
 
 // ErrNoValue indicates that Values does not contain a key with a value
-type ErrNoValue struct {
-	Key string
-}
+type ErrNoValue string
 
-func (e ErrNoValue) Error() string { return fmt.Sprintf("%q is not a value", e.Key) }
+func (e ErrNoValue) Error() string { return fmt.Sprintf("%q is not a value", e) }
diff --git a/pkg/chartutil/save.go b/pkg/chartutil/save.go
index ca6eeaa19..bc8f5bdd9 100644
--- a/pkg/chartutil/save.go
+++ b/pkg/chartutil/save.go
@@ -135,7 +135,7 @@ func Save(c *chart.Chart, outDir string) (string, error) {
 		rollback = true
 		return filename, err
 	}
-	return filename, err
+	return filename, nil
 }
 
 func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error {
diff --git a/pkg/chartutil/values.go b/pkg/chartutil/values.go
index a6b55701c..fe9152857 100644
--- a/pkg/chartutil/values.go
+++ b/pkg/chartutil/values.go
@@ -88,7 +88,7 @@ func (v Values) Encode(w io.Writer) error {
 func tableLookup(v Values, simple string) (Values, error) {
 	v2, ok := v[simple]
 	if !ok {
-		return v, ErrNoTable{simple}
+		return v, ErrNoTable(simple)
 	}
 	if vv, ok := v2.(map[string]interface{}); ok {
 		return vv, nil
@@ -101,7 +101,7 @@ func tableLookup(v Values, simple string) (Values, error) {
 		return vv, nil
 	}
 
-	return Values{}, ErrNoTable{simple}
+	return Values{}, ErrNoTable(simple)
 }
 
 // ReadValues will parse YAML byte data into a Values.
@@ -193,20 +193,20 @@ func (v Values) pathValue(path []string) (interface{}, error) {
 		if _, ok := v[path[0]]; ok && !istable(v[path[0]]) {
 			return v[path[0]], nil
 		}
-		return nil, ErrNoValue{path[0]}
+		return nil, ErrNoValue(path[0])
 	}
 
 	key, path := path[len(path)-1], path[:len(path)-1]
 	// get our table for table path
 	t, err := v.Table(joinPath(path...))
 	if err != nil {
-		return nil, ErrNoValue{key}
+		return nil, ErrNoValue(key)
 	}
 	// check table for key and ensure value is not a table
 	if k, ok := t[key]; ok && !istable(k) {
 		return k, nil
 	}
-	return nil, ErrNoValue{key}
+	return nil, ErrNoValue(key)
 }
 
 func parsePath(key string) []string { return strings.Split(key, ".") }


Test patch:
diff --git a/cmd/helm/get_all_test.go b/cmd/helm/get_test.go
similarity index 76%
rename from cmd/helm/get_all_test.go
rename to cmd/helm/get_test.go
index 0b026fca4..1279c5f7f 100644
--- a/cmd/helm/get_all_test.go
+++ b/cmd/helm/get_test.go
@@ -24,19 +24,19 @@ import (
 
 func TestGetCmd(t *testing.T) {
 	tests := []cmdTestCase{{
-		name:   "get all with a release",
-		cmd:    "get all thomas-guide",
+		name:   "get with a release",
+		cmd:    "get thomas-guide",
 		golden: "output/get-release.txt",
 		rels:   []*release.Release{release.Mock(&release.MockReleaseOptions{Name: "thomas-guide"})},
 	}, {
-		name:   "get all with a formatted release",
-		cmd:    "get all elevated-turkey --template {{.Release.Chart.Metadata.Version}}",
+		name:   "get with a formatted release",
+		cmd:    "get elevated-turkey --template {{.Release.Chart.Metadata.Version}}",
 		golden: "output/get-release-template.txt",
 		rels:   []*release.Release{release.Mock(&release.MockReleaseOptions{Name: "elevated-turkey"})},
 	}, {
-		name:      "get all requires release name arg",
-		cmd:       "get all",
-		golden:    "output/get-all-no-args.txt",
+		name:      "get requires release name arg",
+		cmd:       "get",
+		golden:    "output/get-no-args.txt",
 		wantError: true,
 	}}
 	runTestCmd(t, tests)
diff --git a/cmd/helm/testdata/output/get-all-no-args.txt b/cmd/helm/testdata/output/get-all-no-args.txt
deleted file mode 100644
index cc3fc2ad1..000000000
--- a/cmd/helm/testdata/output/get-all-no-args.txt
+++ /dev/null
@@ -1,3 +0,0 @@
-Error: "helm get all" requires 1 argument
-
-Usage:  helm get all RELEASE_NAME [flags]
diff --git a/cmd/helm/testdata/output/get-no-args.txt b/cmd/helm/testdata/output/get-no-args.txt
new file mode 100644
index 000000000..b911b38c5
--- /dev/null
+++ b/cmd/helm/testdata/output/get-no-args.txt
@@ -0,0 +1,3 @@
+Error: "helm get" requires 1 argument
+
+Usage:  helm get RELEASE_NAME [flags]
diff --git a/cmd/helm/testdata/output/get-notes-no-args.txt b/cmd/helm/testdata/output/get-notes-no-args.txt
index 1a0c20caa..6523ce8fd 100644
--- a/cmd/helm/testdata/output/get-notes-no-args.txt
+++ b/cmd/helm/testdata/output/get-notes-no-args.txt
@@ -1,3 +1,3 @@
 Error: "helm get notes" requires 1 argument
 
-Usage:  helm get notes RELEASE_NAME [flags]
+Usage:  helm get notes [flags] RELEASE_NAME
diff --git a/pkg/chartutil/errors_test.go b/pkg/chartutil/errors_test.go
deleted file mode 100644
index 3f9d04a71..000000000
--- a/pkg/chartutil/errors_test.go
+++ /dev/null
@@ -1,37 +0,0 @@
-/*
-Copyright The Helm Authors.
-
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-    http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package chartutil
-
-import (
-	"testing"
-)
-
-func TestErrorNoTableDoesntPanic(t *testing.T) {
-	x := "empty"
-
-	y := ErrNoTable{x}
-
-	t.Logf("error is: %s", y)
-}
-
-func TestErrorNoValueDoesntPanic(t *testing.T) {
-	x := "empty"
-
-	y := ErrNoValue{x}
-
-	t.Logf("error is: %s", y)
-}