Repository: helm/helm (go)

Issue:
`helm package` is stripping comments from `values.yaml`
In Helm 3.0.0 final, `helm package` appears to be stripping the comments out of the values file. This happens when the values file is parsed and then re-marshaled (since the Go YAML parser does not keep comments).

To reproduce:

```
$ helm create foo
$ helm package foo
$ helm inspect values ./foo
$ helm inspect values foo-0.1.0.tgz
```




Pull request description:
fix(package): remove --set, --values, etc. flags
These flags snuck in through a feature that was reverted and removed in Helm 2, but snuck into Helm 3. See https://github.com/helm/helm/issues/6951#issuecomment-564261923 for more information.

Luckily, they were never hooked into the action package, so they were effectively a no-op. This shouldn't affect anyone.

This also fixes an issue where comments from values.yaml were being stripped during `helm package`, `helm create --starter` and `helm chart export`.

closes #6951 

To test:

```
./bin/helm create foo
./bin/helm package foo
rm -rf foo
tar xzf foo-0.1.0.tgz
cat foo/values.yaml
```

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>

Patch:
diff --git a/cmd/helm/package.go b/cmd/helm/package.go
index 20cde6e4c..00fe0ef11 100644
--- a/cmd/helm/package.go
+++ b/cmd/helm/package.go
@@ -118,7 +118,6 @@ func newPackageCmd(out io.Writer) *cobra.Command {
 	f.StringVar(&client.AppVersion, "app-version", "", "set the appVersion on the chart to this version")
 	f.StringVarP(&client.Destination, "destination", "d", ".", "location to write the chart.")
 	f.BoolVarP(&client.DependencyUpdate, "dependency-update", "u", false, `update dependencies from "Chart.yaml" to dir "charts/" before packaging`)
-	addValueOptionsFlags(f, valueOpts)
 
 	return cmd
 }
diff --git a/pkg/action/package.go b/pkg/action/package.go
index 5c85ebe0d..b48fc65f0 100644
--- a/pkg/action/package.go
+++ b/pkg/action/package.go
@@ -60,12 +60,6 @@ func (p *Package) Run(path string, vals map[string]interface{}) (string, error)
 		return "", err
 	}
 
-	combinedVals, err := chartutil.CoalesceValues(ch, vals)
-	if err != nil {
-		return "", err
-	}
-	ch.Values = combinedVals
-
 	// If version is set, modify the version.
 	if p.Version != "" {
 		if err := setVersion(ch, p.Version); err != nil {
diff --git a/pkg/chartutil/save.go b/pkg/chartutil/save.go
index fb985bb59..be0dfdc24 100644
--- a/pkg/chartutil/save.go
+++ b/pkg/chartutil/save.go
@@ -50,11 +50,12 @@ func SaveDir(c *chart.Chart, dest string) error {
 	}
 
 	// Save values.yaml
-	if c.Values != nil {
-		vf := filepath.Join(outdir, ValuesfileName)
-		b, _ := yaml.Marshal(c.Values)
-		if err := writeFile(vf, b); err != nil {
-			return err
+	for _, f := range c.Raw {
+		if f.Name == ValuesfileName {
+			vf := filepath.Join(outdir, ValuesfileName)
+			if err := writeFile(vf, f.Data); err != nil {
+				return err
+			}
 		}
 	}
 
@@ -161,12 +162,12 @@ func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error {
 	}
 
 	// Save values.yaml
-	ydata, err := yaml.Marshal(c.Values)
-	if err != nil {
-		return err
-	}
-	if err := writeToTar(out, filepath.Join(base, ValuesfileName), ydata); err != nil {
-		return err
+	for _, f := range c.Raw {
+		if f.Name == ValuesfileName {
+			if err := writeToTar(out, filepath.Join(base, ValuesfileName), f.Data); err != nil {
+				return err
+			}
+		}
 	}
 
 	// Save values.schema.json if it exists


Test patch:
diff --git a/cmd/helm/package_test.go b/cmd/helm/package_test.go
index 38938b4af..e0a5fabd6 100644
--- a/cmd/helm/package_test.go
+++ b/cmd/helm/package_test.go
@@ -17,13 +17,9 @@ package main
 
 import (
 	"bytes"
-	"fmt"
-	"io/ioutil"
 	"os"
 	"path/filepath"
 	"regexp"
-	"runtime"
-	"strings"
 	"testing"
 
 	"github.com/spf13/cobra"
@@ -31,15 +27,9 @@ import (
 	"helm.sh/helm/v3/internal/test/ensure"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
-	"helm.sh/helm/v3/pkg/chartutil"
 )
 
 func TestPackage(t *testing.T) {
-	statFileMsg := "no such file or directory"
-	if runtime.GOOS == "windows" {
-		statFileMsg = "The system cannot find the file specified."
-	}
-
 	tests := []struct {
 		name    string
 		flags   map[string]string
@@ -108,13 +98,6 @@ func TestPackage(t *testing.T) {
 			hasfile: "chart-missing-deps-0.1.0.tgz",
 			err:     true,
 		},
-		{
-			name:   "package --values does-not-exist",
-			args:   []string{"testdata/testcharts/alpine"},
-			flags:  map[string]string{"values": "does-not-exist"},
-			expect: fmt.Sprintf("does-not-exist: %s", statFileMsg),
-			err:    true,
-		},
 		{
 			name: "package testdata/testcharts/chart-bad-type",
 			args: []string{"testdata/testcharts/chart-bad-type"},
@@ -213,159 +196,6 @@ func TestSetAppVersion(t *testing.T) {
 	}
 }
 
-func TestPackageValues(t *testing.T) {
-	defer resetEnv()()
-
-	repoFile := "testdata/helmhome/helm/repositories.yaml"
-
-	testCases := []struct {
-		desc               string
-		args               []string
-		valuefilesContents []string
-		flags              map[string]string
-		expected           []string
-	}{
-		{
-			desc:               "helm package, single values file",
-			args:               []string{"testdata/testcharts/alpine"},
-			flags:              map[string]string{"repository-config": repoFile},
-			valuefilesContents: []string{"Name: chart-name-foo"},
-			expected:           []string{"Name: chart-name-foo"},
-		},
-		{
-			desc:               "helm package, multiple values files",
-			args:               []string{"testdata/testcharts/alpine"},
-			flags:              map[string]string{"repository-config": repoFile},
-			valuefilesContents: []string{"Name: chart-name-foo", "foo: bar"},
-			expected:           []string{"Name: chart-name-foo", "foo: bar"},
-		},
-		{
-			desc:     "helm package, with set option",
-			args:     []string{"testdata/testcharts/alpine"},
-			flags:    map[string]string{"set": "Name=chart-name-foo", "repository-config": repoFile},
-			expected: []string{"Name: chart-name-foo"},
-		},
-		{
-			desc:               "helm package, set takes precedence over value file",
-			args:               []string{"testdata/testcharts/alpine"},
-			valuefilesContents: []string{"Name: chart-name-foo"},
-			flags:              map[string]string{"set": "Name=chart-name-bar", "repository-config": repoFile},
-			expected:           []string{"Name: chart-name-bar"},
-		},
-	}
-
-	for _, tc := range testCases {
-		var files []string
-		for _, contents := range tc.valuefilesContents {
-			f := createValuesFile(t, contents)
-			files = append(files, f)
-		}
-		valueFiles := strings.Join(files, ",")
-
-		expected, err := chartutil.ReadValues([]byte(strings.Join(tc.expected, "\n")))
-		if err != nil {
-			t.Errorf("unexpected error parsing values: %q", err)
-		}
-
-		outputDir := ensure.TempDir(t)
-
-		if len(tc.flags) == 0 {
-			tc.flags = make(map[string]string)
-		}
-		tc.flags["destination"] = outputDir
-
-		if len(valueFiles) > 0 {
-			tc.flags["values"] = valueFiles
-		}
-
-		cmd := newPackageCmd(&bytes.Buffer{})
-		setFlags(cmd, tc.flags)
-		if err := cmd.RunE(cmd, tc.args); err != nil {
-			t.Fatalf("unexpected error: %q", err)
-		}
-
-		outputFile := filepath.Join(outputDir, "alpine-0.1.0.tgz")
-		verifyOutputChartExists(t, outputFile)
-
-		actual, err := getChartValues(outputFile)
-		if err != nil {
-			t.Fatalf("unexpected error extracting chart values: %q", err)
-		}
-
-		verifyValues(t, actual, expected)
-	}
-}
-
-func TestNonExistentDirAndBadPermission(t *testing.T) {
-	nonExistentDir := "testdata/testcharts/non-existent-directory"
-
-	tests := []struct {
-		name    string
-		flags   map[string]string
-		args    []string
-		expect  string
-		hasfile string
-		err     bool
-	}{
-		{
-			name:   "package testdata/testcharts/non-existent-directory",
-			args:   []string{"testdata/testcharts/non-existent-directory"},
-			expect: fmt.Sprintf("stat %s: no such file or directory", nonExistentDir),
-			err:    true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			var buf bytes.Buffer
-			c := newPackageCmd(&buf)
-			re := regexp.MustCompile(tt.expect)
-			err := c.RunE(c, tt.args)
-			if err != nil {
-				if tt.err && re.MatchString(err.Error()) {
-					return
-				}
-				t.Fatalf("%q: expected error %q, got %q", tt.name, tt.expect, err)
-			}
-		})
-	}
-}
-
-func createValuesFile(t *testing.T, data string) string {
-	outputDir := ensure.TempDir(t)
-
-	outputFile := filepath.Join(outputDir, "values.yaml")
-	if err := ioutil.WriteFile(outputFile, []byte(data), 0644); err != nil {
-		t.Fatalf("err: %s", err)
-	}
-	return outputFile
-}
-
-func getChartValues(chartPath string) (chartutil.Values, error) {
-	chart, err := loader.Load(chartPath)
-	if err != nil {
-		return nil, err
-	}
-	return chart.Values, nil
-}
-
-func verifyValues(t *testing.T, actual, expected chartutil.Values) {
-	t.Helper()
-	for key, value := range expected.AsMap() {
-		if got := actual[key]; got != value {
-			t.Errorf("Expected %q, got %q (%v)", value, got, actual)
-		}
-	}
-}
-
-func verifyOutputChartExists(t *testing.T, chartPath string) {
-	if chartFile, err := os.Stat(chartPath); err != nil {
-		t.Errorf("expected file %q, got err %q", chartPath, err)
-	} else if chartFile.Size() == 0 {
-		t.Errorf("file %q has zero bytes.", chartPath)
-	}
-}
-
 func setFlags(cmd *cobra.Command, flags map[string]string) {
 	dest := cmd.Flags()
 	for f, v := range flags {