Show errors when linting for Chart.yaml version and appVersion fields of type non-string

Signed-off-by: Karuppiah Natarajan <karuppiah7890@gmail.com>
5 years ago · 47c38ec23d
parent 15d9a6190f
commit 47c38ec23d
5 changed files with 112 additions and 27 deletions
--- a/pkg/action/testdata/charts/multiplecharts-lint-chart-1/Chart.yaml
+++ b/pkg/action/testdata/charts/multiplecharts-lint-chart-1/Chart.yaml
@ -1,4 +1,4 @@
 apiVersion: v1
 name: multiplecharts-lint-chart-1
-version: 1
+version: "1"
 icon: ""
--- a/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml
+++ b/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml
@ -1,4 +1,4 @@
 apiVersion: v1
 name: multiplecharts-lint-chart-2
-version: 1
+version: "1"
 icon: ""
--- a/pkg/lint/rules/chartfile.go
+++ b/pkg/lint/rules/chartfile.go
@ -18,12 +18,14 @@ package rules // import "helm.sh/helm/v3/pkg/lint/rules"

 import (
 	"fmt"
+	"io/ioutil"
 	"os"
 	"path/filepath"

 	"github.com/Masterminds/semver/v3"
 	"github.com/asaskevich/govalidator"
 	"github.com/pkg/errors"
+	"sigs.k8s.io/yaml"

 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chartutil"
@ -45,11 +47,18 @@ func Chartfile(linter *support.Linter) {
 		return
 	}

+	// type check for Chart.yaml . ignoring error as any parse
+	// errors would already be caught in the above load function
+	chartFileForTypeCheck, _ := loadChartFileForTypeCheck(chartPath)
+
 	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartName(chartFile))

 	// Chart metadata
 	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartAPIVersion(chartFile))
+
+	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartVersionType(chartFileForTypeCheck))
 	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartVersion(chartFile))
+	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartAppVersionType(chartFileForTypeCheck))
 	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartMaintainer(chartFile))
 	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartSources(chartFile))
 	linter.RunLinterRule(support.InfoSev, chartFileName, validateChartIconPresence(chartFile))
@ -58,6 +67,26 @@ func Chartfile(linter *support.Linter) {
 	linter.RunLinterRule(support.ErrorSev, chartFileName, validateChartDependencies(chartFile))
 }

+func validateChartVersionType(data map[string]interface{}) error {
+	return isStringValue(data, "version")
+}
+
+func validateChartAppVersionType(data map[string]interface{}) error {
+	return isStringValue(data, "appVersion")
+}
+
+func isStringValue(data map[string]interface{}, key string) error {
+	value, ok := data[key]
+	if !ok {
+		return nil
+	}
+	valueType := fmt.Sprintf("%T", value)
+	if valueType != "string" {
+		return errors.Errorf("%s should be of type string but it's of type %s", key, valueType)
+	}
+	return nil
+}
+
 func validateChartYamlNotDirectory(chartPath string) error {
 	fi, err := os.Stat(chartPath)

@ -166,3 +195,16 @@ func validateChartType(cf *chart.Metadata) error {
 	}
 	return nil
 }
+
+// loadChartFileForTypeCheck loads the Chart.yaml
+// in a generic form of a map[string]interface{}, so that the type
+// of the values can be checked
+func loadChartFileForTypeCheck(filename string) (map[string]interface{}, error) {
+	b, err := ioutil.ReadFile(filename)
+	if err != nil {
+		return nil, err
+	}
+	y := make(map[string]interface{})
+	err = yaml.Unmarshal(b, &y)
+	return y, err
+}
--- a/pkg/lint/rules/chartfile_test.go
+++ b/pkg/lint/rules/chartfile_test.go
@ -30,7 +30,8 @@ import (
 )

 const (
-	badChartDir = "testdata/badchartfile"
+	badChartDir        = "testdata/badchartfile"
+	anotherBadChartDir = "testdata/anotherbadchartfile"
 )

 var (
@ -184,36 +185,63 @@ func TestValidateChartIconURL(t *testing.T) {
 }

 func TestChartfile(t *testing.T) {
-	linter := support.Linter{ChartDir: badChartDir}
-	Chartfile(&linter)
-	msgs := linter.Messages
+	t.Run("Chart.yaml basic validity issues", func(t *testing.T) {
+		linter := support.Linter{ChartDir: badChartDir}
+		Chartfile(&linter)
+		msgs := linter.Messages
+		expectedNumberOfErrorMessages := 6
+
+		if len(msgs) != expectedNumberOfErrorMessages {
+			t.Errorf("Expected %d errors, got %d", expectedNumberOfErrorMessages, len(msgs))
+			return
+		}

-	if len(msgs) != 6 {
-		t.Errorf("Expected 6 errors, got %d", len(msgs))
-	}
+		if !strings.Contains(msgs[0].Err.Error(), "name is required") {
+			t.Errorf("Unexpected message 0: %s", msgs[0].Err)
+		}

-	if !strings.Contains(msgs[0].Err.Error(), "name is required") {
-		t.Errorf("Unexpected message 0: %s", msgs[0].Err)
-	}
+		if !strings.Contains(msgs[1].Err.Error(), "apiVersion is required. The value must be either \"v1\" or \"v2\"") {
+			t.Errorf("Unexpected message 1: %s", msgs[1].Err)
+		}

-	if !strings.Contains(msgs[1].Err.Error(), "apiVersion is required. The value must be either \"v1\" or \"v2\"") {
-		t.Errorf("Unexpected message 1: %s", msgs[1].Err)
-	}
+		if !strings.Contains(msgs[2].Err.Error(), "version '0.0.0.0' is not a valid SemVer") {
+			t.Errorf("Unexpected message 2: %s", msgs[2].Err)
+		}

-	if !strings.Contains(msgs[2].Err.Error(), "version '0.0.0.0' is not a valid SemVer") {
-		t.Errorf("Unexpected message 2: %s", msgs[2].Err)
-	}
+		if !strings.Contains(msgs[3].Err.Error(), "icon is recommended") {
+			t.Errorf("Unexpected message 3: %s", msgs[3].Err)
+		}

-	if !strings.Contains(msgs[3].Err.Error(), "icon is recommended") {
-		t.Errorf("Unexpected message 3: %s", msgs[3].Err)
-	}
+		if !strings.Contains(msgs[4].Err.Error(), "chart type is not valid in apiVersion") {
+			t.Errorf("Unexpected message 4: %s", msgs[4].Err)
+		}

-	if !strings.Contains(msgs[4].Err.Error(), "chart type is not valid in apiVersion") {
-		t.Errorf("Unexpected message 4: %s", msgs[4].Err)
-	}
+		if !strings.Contains(msgs[5].Err.Error(), "dependencies are not valid in the Chart file with apiVersion") {
+			t.Errorf("Unexpected message 5: %s", msgs[5].Err)
+		}
+	})

-	if !strings.Contains(msgs[5].Err.Error(), "dependencies are not valid in the Chart file with apiVersion") {
-		t.Errorf("Unexpected message 5: %s", msgs[5].Err)
-	}
+	t.Run("Chart.yaml validity issues due to type mismatch", func(t *testing.T) {
+		linter := support.Linter{ChartDir: anotherBadChartDir}
+		Chartfile(&linter)
+		msgs := linter.Messages
+		expectedNumberOfErrorMessages := 3
+
+		if len(msgs) != expectedNumberOfErrorMessages {
+			t.Errorf("Expected %d errors, got %d", expectedNumberOfErrorMessages, len(msgs))
+			return
+		}

+		if !strings.Contains(msgs[0].Err.Error(), "version should be of type string") {
+			t.Errorf("Unexpected message 0: %s", msgs[0].Err)
+		}
+
+		if !strings.Contains(msgs[1].Err.Error(), "version '7.2445e+06' is not a valid SemVer") {
+			t.Errorf("Unexpected message 1: %s", msgs[1].Err)
+		}
+
+		if !strings.Contains(msgs[2].Err.Error(), "appVersion should be of type string") {
+			t.Errorf("Unexpected message 2: %s", msgs[2].Err)
+		}
+	})
 }
--- a/pkg/lint/rules/testdata/anotherbadchartfile/Chart.yaml
+++ b/pkg/lint/rules/testdata/anotherbadchartfile/Chart.yaml
@ -0,0 +1,15 @@
+name: "some-chart"
+apiVersion: v2
+description: A Helm chart for Kubernetes
+version: 72445e2
+home: ""
+type: application
+appVersion: 72225e2
+icon: "https://some-url.com/icon.jpeg"
+dependencies:
+  - name: mariadb
+    version: 5.x.x
+    repository: https://kubernetes-charts.storage.googleapis.com/
+    condition: mariadb.enabled
+    tags:
+      - database