From 47c38ec23d564fa98d40f90979736b446dc521b9 Mon Sep 17 00:00:00 2001 From: Karuppiah Natarajan Date: Sun, 15 Dec 2019 20:13:56 +0530 Subject: [PATCH] Show errors when linting for Chart.yaml version and appVersion fields of type non-string Signed-off-by: Karuppiah Natarajan --- .../multiplecharts-lint-chart-1/Chart.yaml | 2 +- .../multiplecharts-lint-chart-2/Chart.yaml | 2 +- pkg/lint/rules/chartfile.go | 42 ++++++++++ pkg/lint/rules/chartfile_test.go | 78 +++++++++++++------ .../testdata/anotherbadchartfile/Chart.yaml | 15 ++++ 5 files changed, 112 insertions(+), 27 deletions(-) create mode 100644 pkg/lint/rules/testdata/anotherbadchartfile/Chart.yaml diff --git a/pkg/action/testdata/charts/multiplecharts-lint-chart-1/Chart.yaml b/pkg/action/testdata/charts/multiplecharts-lint-chart-1/Chart.yaml index 3b342ae4f..e33c97e8c 100644 --- 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: "" \ No newline at end of file diff --git a/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml b/pkg/action/testdata/charts/multiplecharts-lint-chart-2/Chart.yaml index bd101d808..b27de2754 100644 --- 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: "" \ No newline at end of file diff --git a/pkg/lint/rules/chartfile.go b/pkg/lint/rules/chartfile.go index 91a64fe13..b49f2cec0 100644 --- 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 +} diff --git a/pkg/lint/rules/chartfile_test.go b/pkg/lint/rules/chartfile_test.go index d032dd12f..087cda047 100644 --- 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) + } + }) } diff --git a/pkg/lint/rules/testdata/anotherbadchartfile/Chart.yaml b/pkg/lint/rules/testdata/anotherbadchartfile/Chart.yaml new file mode 100644 index 000000000..7f3cab390 --- /dev/null +++ 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