From 77820c748282e9f2134b083a5c1afe480fe7c1da Mon Sep 17 00:00:00 2001 From: Adnan Abdulhussein Date: Tue, 28 Jun 2016 16:06:03 -0700 Subject: [PATCH 1/2] fix(lint): Return non-zero exit status when lint errors present --- cmd/helm/lint.go | 13 ++++++++++--- pkg/lint/lint.go | 5 ++--- pkg/lint/lint_test.go | 11 ++++++----- pkg/lint/support/message.go | 8 +++++++- pkg/lint/support/message_test.go | 29 +++++++++++++++++------------ 5 files changed, 42 insertions(+), 24 deletions(-) diff --git a/cmd/helm/lint.go b/cmd/helm/lint.go index 1c652e182..a14480bbc 100644 --- a/cmd/helm/lint.go +++ b/cmd/helm/lint.go @@ -28,6 +28,7 @@ import ( "k8s.io/helm/pkg/chartutil" "k8s.io/helm/pkg/lint" + "k8s.io/helm/pkg/lint/support" ) var longLintHelp = ` @@ -51,6 +52,7 @@ func init() { } var errLintNoChart = errors.New("no chart found for linting (missing Chart.yaml)") +var errLintFailed = errors.New("lint failed") func lintCmd(cmd *cobra.Command, args []string) error { path := "." @@ -92,15 +94,20 @@ func lintChart(path string) error { return errLintNoChart } - issues := lint.All(path) + linter := lint.All(path) - if len(issues) == 0 { + if len(linter.Messages) == 0 { fmt.Println("Lint OK") + return nil } - for _, i := range issues { + for _, i := range linter.Messages { fmt.Printf("%s\n", i) } + if linter.HighestSeverity == support.ErrorSev { + return errLintFailed + } + return nil } diff --git a/pkg/lint/lint.go b/pkg/lint/lint.go index 6c47413e6..ea723b2f0 100644 --- a/pkg/lint/lint.go +++ b/pkg/lint/lint.go @@ -24,8 +24,7 @@ import ( ) // All runs all of the available linters on the given base directory. -func All(basedir string) []support.Message { - +func All(basedir string) support.Linter { // Using abs path to get directory context chartDir, _ := filepath.Abs(basedir) @@ -33,5 +32,5 @@ func All(basedir string) []support.Message { rules.Chartfile(&linter) rules.Values(&linter) rules.Templates(&linter) - return linter.Messages + return linter } diff --git a/pkg/lint/lint_test.go b/pkg/lint/lint_test.go index c37aa17c2..396c23457 100644 --- a/pkg/lint/lint_test.go +++ b/pkg/lint/lint_test.go @@ -17,9 +17,10 @@ limitations under the License. package lint import ( - "k8s.io/helm/pkg/lint/support" "strings" + "k8s.io/helm/pkg/lint/support" + "testing" ) @@ -29,7 +30,7 @@ const badYamlFileDir = "rules/testdata/albatross" const goodChartDir = "rules/testdata/goodone" func TestBadChart(t *testing.T) { - m := All(badChartDir) + m := All(badChartDir).Messages if len(m) != 4 { t.Errorf("Number of errors %v", len(m)) t.Errorf("All didn't fail with expected errors, got %#v", m) @@ -60,7 +61,7 @@ func TestBadChart(t *testing.T) { } func TestInvalidYaml(t *testing.T) { - m := All(badYamlFileDir) + m := All(badYamlFileDir).Messages if len(m) != 1 { t.Errorf("All didn't fail with expected errors, got %#v", m) } @@ -70,7 +71,7 @@ func TestInvalidYaml(t *testing.T) { } func TestBadValues(t *testing.T) { - m := All(badValuesFileDir) + m := All(badValuesFileDir).Messages if len(m) != 1 { t.Errorf("All didn't fail with expected errors, got %#v", m) } @@ -80,7 +81,7 @@ func TestBadValues(t *testing.T) { } func TestGoodChart(t *testing.T) { - m := All(goodChartDir) + m := All(goodChartDir).Messages if len(m) != 0 { t.Errorf("All failed but shouldn't have: %#v", m) } diff --git a/pkg/lint/support/message.go b/pkg/lint/support/message.go index 1c3fdcd58..8615bc15e 100644 --- a/pkg/lint/support/message.go +++ b/pkg/lint/support/message.go @@ -44,7 +44,9 @@ type Message struct { // Linter encapsulates a linting run of a particular chart. type Linter struct { Messages []Message - ChartDir string + // The highest severity of all the failing lint rules + HighestSeverity int + ChartDir string } // LintError describes an error encountered while linting. @@ -68,6 +70,10 @@ func (l *Linter) RunLinterRule(severity int, lintError LintError) bool { if lintError != nil { l.Messages = append(l.Messages, Message{Text: lintError.Error(), Severity: severity}) + + if severity > l.HighestSeverity { + l.HighestSeverity = severity + } } return lintError == nil } diff --git a/pkg/lint/support/message_test.go b/pkg/lint/support/message_test.go index e97b173a7..438216f2f 100644 --- a/pkg/lint/support/message_test.go +++ b/pkg/lint/support/message_test.go @@ -26,26 +26,31 @@ var lintError LintError = fmt.Errorf("Foobar") func TestRunLinterRule(t *testing.T) { var tests = []struct { - Severity int - LintError error - ExpectedMessages int - ExpectedReturn bool + Severity int + LintError error + ExpectedMessages int + ExpectedReturn bool + ExpectedHighestSeverity int }{ - {ErrorSev, lintError, 1, false}, - {WarningSev, lintError, 2, false}, - {InfoSev, lintError, 3, false}, + {InfoSev, lintError, 1, false, InfoSev}, + {WarningSev, lintError, 2, false, WarningSev}, + {ErrorSev, lintError, 3, false, ErrorSev}, // No error so it returns true - {ErrorSev, nil, 3, true}, + {ErrorSev, nil, 3, true, ErrorSev}, // Invalid severity values - {4, lintError, 3, false}, - {22, lintError, 3, false}, - {-1, lintError, 3, false}, + {4, lintError, 3, false, ErrorSev}, + {22, lintError, 3, false, ErrorSev}, + {-1, lintError, 3, false, ErrorSev}, } for _, test := range tests { isValid := linter.RunLinterRule(test.Severity, test.LintError) if len(linter.Messages) != test.ExpectedMessages { - t.Errorf("RunLinterRule(%d, %v), linter.Messages should have now %d message, we got %d", test.Severity, test.LintError, test.ExpectedMessages, len(linter.Messages)) + t.Errorf("RunLinterRule(%d, %v), linter.Messages should now have %d message, we got %d", test.Severity, test.LintError, test.ExpectedMessages, len(linter.Messages)) + } + + if linter.HighestSeverity != test.ExpectedHighestSeverity { + t.Errorf("RunLinterRule(%d, %v), linter.HighestSeverity should be %d, we got %d", test.Severity, test.LintError, test.ExpectedHighestSeverity, linter.HighestSeverity) } if isValid != test.ExpectedReturn { From 09f56459c7fe5dc928f80b7ca0bf92077f313ede Mon Sep 17 00:00:00 2001 From: Adnan Abdulhussein Date: Wed, 29 Jun 2016 13:53:13 -0700 Subject: [PATCH 2/2] Improve tests to ensure highest severity is retained --- pkg/lint/support/message_test.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pkg/lint/support/message_test.go b/pkg/lint/support/message_test.go index 438216f2f..fcaa63c4f 100644 --- a/pkg/lint/support/message_test.go +++ b/pkg/lint/support/message_test.go @@ -37,10 +37,12 @@ func TestRunLinterRule(t *testing.T) { {ErrorSev, lintError, 3, false, ErrorSev}, // No error so it returns true {ErrorSev, nil, 3, true, ErrorSev}, + // Retains highest severity + {InfoSev, lintError, 4, false, ErrorSev}, // Invalid severity values - {4, lintError, 3, false, ErrorSev}, - {22, lintError, 3, false, ErrorSev}, - {-1, lintError, 3, false, ErrorSev}, + {4, lintError, 4, false, ErrorSev}, + {22, lintError, 4, false, ErrorSev}, + {-1, lintError, 4, false, ErrorSev}, } for _, test := range tests {