feat: make the linter coalesce the passed-in values before running values tests (#7984)

* fix: make the linter coalesce the passed-in values before running values tests

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

* fixed typo

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>

internal/test/ensure/ensure.go

@@ -19,6 +19,7 @@ package ensure
 import (
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"testing"
 
 	"helm.sh/helm/v3/pkg/helmpath"
@@ -49,3 +50,21 @@ func TempDir(t *testing.T) string {
 	}
 	return d
 }
+
+// TempFile ensures a temp file for unit testing purposes.
+//
+// It returns the path to the directory (to which you will still need to join the filename)
+//
+// You must clean up the directory that is returned.
+//
+// 	tempdir := TempFile(t, "foo", []byte("bar"))
+// 	defer os.RemoveAll(tempdir)
+// 	filename := filepath.Join(tempdir, "foo")
+func TempFile(t *testing.T, name string, data []byte) string {
+	path := TempDir(t)
+	filename := filepath.Join(path, name)
+	if err := ioutil.WriteFile(filename, data, 0755); err != nil {
+		t.Fatal(err)
+	}
+	return path
+}

pkg/lint/lint.go

@@ -30,7 +30,7 @@ func All(basedir string, values map[string]interface{}, namespace string, strict
 
 	linter := support.Linter{ChartDir: chartDir}
 	rules.Chartfile(&linter)
-	rules.Values(&linter)
+	rules.ValuesWithOverrides(&linter, values)
 	rules.Templates(&linter, values, namespace, strict)
 	return linter
 }

pkg/lint/rules/values.go

@@ -28,7 +28,19 @@ import (
 )
 
 // Values lints a chart's values.yaml file.
+//
+// This function is deprecated and will be removed in Helm 4.
 func Values(linter *support.Linter) {
+	ValuesWithOverrides(linter, map[string]interface{}{})
+}
+
+// ValuesWithOverrides tests the values.yaml file.
+//
+// If a schema is present in the chart, values are tested against that. Otherwise,
+// they are only tested for well-formedness.
+//
+// If additional values are supplied, they are coalesced into the values in values.yaml.
+func ValuesWithOverrides(linter *support.Linter, values map[string]interface{}) {
 	file := "values.yaml"
 	vf := filepath.Join(linter.ChartDir, file)
 	fileExists := linter.RunLinterRule(support.InfoSev, file, validateValuesFileExistence(vf))
@@ -37,7 +49,7 @@ func Values(linter *support.Linter) {
 		return
 	}
 
-	linter.RunLinterRule(support.ErrorSev, file, validateValuesFile(vf))
+	linter.RunLinterRule(support.ErrorSev, file, validateValuesFile(vf, values))
 }
 
 func validateValuesFileExistence(valuesPath string) error {
@@ -48,12 +60,19 @@ func validateValuesFileExistence(valuesPath string) error {
 	return nil
 }
 
-func validateValuesFile(valuesPath string) error {
+func validateValuesFile(valuesPath string, overrides map[string]interface{}) error {
 	values, err := chartutil.ReadValuesFile(valuesPath)
 	if err != nil {
 		return errors.Wrap(err, "unable to parse YAML")
 	}
 
+	// Helm 3.0.0 carried over the values linting from Helm 2.x, which only tests the top
+	// level values against the top-level expectations. Subchart values are not linted.
+	// We could change that. For now, though, we retain that strategy, and thus can
+	// coalesce tables (like reuse-values does) instead of doing the full chart
+	// CoalesceValues.
+	values = chartutil.CoalesceTables(values, overrides)
+
 	ext := filepath.Ext(valuesPath)
 	schemaPath := valuesPath[:len(valuesPath)-len(ext)] + ".schema.json"
 	schema, err := ioutil.ReadFile(schemaPath)

pkg/lint/rules/values_test.go

@@ -17,15 +17,41 @@ limitations under the License.
 package rules
 
 import (
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"testing"
-)
 
-var (
+	"github.com/stretchr/testify/assert"
-	nonExistingValuesFilePath = filepath.Join("/fake/dir", "values.yaml")
+
+	"helm.sh/helm/v3/internal/test/ensure"
 )
 
+var nonExistingValuesFilePath = filepath.Join("/fake/dir", "values.yaml")
+
+const testSchema = `
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "helm values test schema",
+  "type": "object",
+  "additionalProperties": false,
+  "required": [
+    "username",
+    "password"
+  ],
+  "properties": {
+    "username": {
+      "description": "Your username",
+      "type": "string"
+    },
+    "password": {
+      "description": "Your password",
+      "type": "string"
+    }
+  }
+}
+`
+
 func TestValidateValuesYamlNotDirectory(t *testing.T) {
 	_ = os.Mkdir(nonExistingValuesFilePath, os.ModePerm)
 	defer os.Remove(nonExistingValuesFilePath)
@@ -35,3 +61,68 @@ func TestValidateValuesYamlNotDirectory(t *testing.T) {
 		t.Errorf("validateValuesFileExistence to return a linter error, got no error")
 	}
 }
+
+func TestValidateValuesFileWellFormed(t *testing.T) {
+	badYaml := `
+	not:well[]{}formed
+	`
+	tmpdir := ensure.TempFile(t, "values.yaml", []byte(badYaml))
+	defer os.RemoveAll(tmpdir)
+	valfile := filepath.Join(tmpdir, "values.yaml")
+	if err := validateValuesFile(valfile, map[string]interface{}{}); err == nil {
+		t.Fatal("expected values file to fail parsing")
+	}
+}
+
+func TestValidateValuesFileSchema(t *testing.T) {
+	yaml := "username: admin\npassword: swordfish"
+	tmpdir := ensure.TempFile(t, "values.yaml", []byte(yaml))
+	defer os.RemoveAll(tmpdir)
+	createTestingSchema(t, tmpdir)
+
+	valfile := filepath.Join(tmpdir, "values.yaml")
+	if err := validateValuesFile(valfile, map[string]interface{}{}); err != nil {
+		t.Fatalf("Failed validation with %s", err)
+	}
+}
+
+func TestValidateValuesFileSchemaFailure(t *testing.T) {
+	// 1234 is an int, not a string. This should fail.
+	yaml := "username: 1234\npassword: swordfish"
+	tmpdir := ensure.TempFile(t, "values.yaml", []byte(yaml))
+	defer os.RemoveAll(tmpdir)
+	createTestingSchema(t, tmpdir)
+
+	valfile := filepath.Join(tmpdir, "values.yaml")
+
+	err := validateValuesFile(valfile, map[string]interface{}{})
+	if err == nil {
+		t.Fatal("expected values file to fail parsing")
+	}
+
+	assert.Contains(t, err.Error(), "Expected: string, given: integer", "integer should be caught by schema")
+}
+
+func TestValidateValuesFileSchemaOverrides(t *testing.T) {
+	yaml := "username: admin"
+	overrides := map[string]interface{}{
+		"password": "swordfish",
+	}
+	tmpdir := ensure.TempFile(t, "values.yaml", []byte(yaml))
+	defer os.RemoveAll(tmpdir)
+	createTestingSchema(t, tmpdir)
+
+	valfile := filepath.Join(tmpdir, "values.yaml")
+	if err := validateValuesFile(valfile, overrides); err != nil {
+		t.Fatalf("Failed validation with %s", err)
+	}
+}
+
+func createTestingSchema(t *testing.T, dir string) string {
+	t.Helper()
+	schemafile := filepath.Join(dir, "values.schema.json")
+	if err := ioutil.WriteFile(schemafile, []byte(testSchema), 0700); err != nil {
+		t.Fatalf("Failed to write schema to tmpdir: %s", err)
+	}
+	return schemafile
+}