Repository: reviewdog/reviewdog
Pull request #504 (2020-03-22 20:33:40)

Allow to return failure if any violations found in input data
Reviewdog allows to filter out checkstyle errors based on git diff and that is cool.
But reviewdog always returns 0 (success) exit code if all jobs are done.

I am using it as a part of CI process, so I want to mark the step as failed if any violations found/reported by using reviewdog and any linters.

By adding new `fail-on-error` flag we can control the resulting exit code.
If flag is positive it will return `1` as exit code if at least one error found/reported by reviewdog

DmitryLanda: @mgrachev @haya14busa it would be nice guys if you provide any feedback on this
ruudk: Thanks for this. Today one of our checkers failed, but Reviewdog dus said everything was fine, causing a breaking change in master... This would have prevented this. Please merge 🙏 

Patch:
diff --git a/README.md b/README.md
index f1877aba..8c1807ac 100644
--- a/README.md
+++ b/README.md
@@ -71,6 +71,7 @@ by diff.
   * [GitLab CI](#gitlab-ci)
   * [Common (Jenkins, local, etc...)](#common-jenkins-local-etc)
     + [Jenkins with Github pull request builder plugin](#jenkins-with-github-pull-request-builder-plugin)
+- [Exit codes](#exit-codes)
 - [Articles](#articles)
 
 [![github-pr-check sample](https://user-images.githubusercontent.com/3797062/40884858-6efd82a0-6756-11e8-9f1a-c6af4f920fb0.png)](https://github.com/reviewdog/reviewdog/pull/131/checks)
@@ -642,6 +643,12 @@ $ REVIEWDOG_TOKEN="<token>" reviewdog -reporter=github-pr-check
 $ REVIEWDOG_GITHUB_API_TOKEN="<token>" reviewdog -reporter=github-pr-review
 ```
 
+## Exit codes
+By default Reviewdog will return `0` as exit code once all actions done (linter report analyzed, comments posted and so on).
+
+Once `fail-on-error` flag passed - Reviewdog will return `1` as exit code if at least one violation was found/reported.
+This can be helpful when you are using it as a step in your CI pipeline and want to mark the step failed if any error found by linter.
+
 ## Articles
 - [reviewdog — A code review dog who keeps your codebase healthy ](https://medium.com/@haya14busa/reviewdog-a-code-review-dog-who-keeps-your-codebase-healthy-d957c471938b)
 - [reviewdog ♡ GitHub Check — improved automated review experience](https://medium.com/@haya14busa/reviewdog-github-check-improved-automated-review-experience-58f89e0c95f3)
diff --git a/cmd/reviewdog/main.go b/cmd/reviewdog/main.go
index c2edb8f5..b2afafe1 100644
--- a/cmd/reviewdog/main.go
+++ b/cmd/reviewdog/main.go
@@ -55,6 +55,7 @@ type option struct {
 	guessPullRequest bool
 	tee              bool
 	filterMode       reviewdog.FilterMode
+	failOnError      bool
 }
 
 // flags doc
@@ -144,6 +145,7 @@ const (
 		$ export CI_REPO_OWNER="haya14busa" # repository owner
 		$ export CI_REPO_NAME="reviewdog" # repository name
 `
+	failOnErrorDoc = `Returns 1 as exit code if any errors/warnings found in input`
 )
 
 var opt = &option{}
@@ -164,6 +166,7 @@ func init() {
 	flag.BoolVar(&opt.guessPullRequest, "guess", false, guessPullRequestDoc)
 	flag.BoolVar(&opt.tee, "tee", false, teeDoc)
 	flag.Var(&opt.filterMode, "filter-mode", filterModeDoc)
+	flag.BoolVar(&opt.failOnError, "fail-on-error", false, failOnErrorDoc)
 }
 
 func usage() {
@@ -308,7 +311,7 @@ github-pr-check reporter as a fallback.
 		if err != nil {
 			return err
 		}
-		return project.Run(ctx, conf, buildRunnersMap(opt.runners), cs, ds, opt.tee, opt.filterMode)
+		return project.Run(ctx, conf, buildRunnersMap(opt.runners), cs, ds, opt.tee, opt.filterMode, opt.failOnError)
 	}
 
 	p, err := newParserFromOpt(opt)
@@ -316,7 +319,7 @@ github-pr-check reporter as a fallback.
 		return err
 	}
 
-	app := reviewdog.NewReviewdog(toolName(opt), p, cs, ds, opt.filterMode)
+	app := reviewdog.NewReviewdog(toolName(opt), p, cs, ds, opt.filterMode, opt.failOnError)
 	return app.Run(ctx, r)
 }
 
diff --git a/project/run.go b/project/run.go
index 7168d4f3..19d384f7 100644
--- a/project/run.go
+++ b/project/run.go
@@ -88,7 +88,7 @@ func RunAndParse(ctx context.Context, conf *Config, runners map[string]bool, def
 }
 
 // Run runs reviewdog tasks based on Config.
-func Run(ctx context.Context, conf *Config, runners map[string]bool, c reviewdog.CommentService, d reviewdog.DiffService, teeMode bool, filterMode reviewdog.FilterMode) error {
+func Run(ctx context.Context, conf *Config, runners map[string]bool, c reviewdog.CommentService, d reviewdog.DiffService, teeMode bool, filterMode reviewdog.FilterMode, failOnError bool) error {
 	results, err := RunAndParse(ctx, conf, runners, "", teeMode) // Level is not used.
 	if err != nil {
 		return err
@@ -112,7 +112,7 @@ func Run(ctx context.Context, conf *Config, runners map[string]bool, c reviewdog
 	results.Range(func(toolname string, result *reviewdog.Result) {
 		rs := result.CheckResults
 		g.Go(func() error {
-			return reviewdog.RunFromResult(ctx, c, rs, filediffs, d.Strip(), toolname, filterMode)
+			return reviewdog.RunFromResult(ctx, c, rs, filediffs, d.Strip(), toolname, filterMode, failOnError)
 		})
 	})
 	return g.Wait()
diff --git a/reviewdog.go b/reviewdog.go
index fbcae0ff..2cb74959 100644
--- a/reviewdog.go
+++ b/reviewdog.go
@@ -14,22 +14,23 @@ import (
 // or linter, get diff and filter the results by diff, and report filtered
 // results.
 type Reviewdog struct {
-	toolname   string
-	p          Parser
-	c          CommentService
-	d          DiffService
-	filterMode FilterMode
+	toolname    string
+	p           Parser
+	c           CommentService
+	d           DiffService
+	filterMode  FilterMode
+	failOnError bool
 }
 
 // NewReviewdog returns a new Reviewdog.
-func NewReviewdog(toolname string, p Parser, c CommentService, d DiffService, filterMode FilterMode) *Reviewdog {
-	return &Reviewdog{p: p, c: c, d: d, toolname: toolname, filterMode: filterMode}
+func NewReviewdog(toolname string, p Parser, c CommentService, d DiffService, filterMode FilterMode, failOnError bool) *Reviewdog {
+	return &Reviewdog{p: p, c: c, d: d, toolname: toolname, filterMode: filterMode, failOnError: failOnError}
 }
 
 // RunFromResult creates a new Reviewdog and runs it with check results.
 func RunFromResult(ctx context.Context, c CommentService, results []*CheckResult,
-	filediffs []*diff.FileDiff, strip int, toolname string, filterMode FilterMode) error {
-	return (&Reviewdog{c: c, toolname: toolname, filterMode: filterMode}).runFromResult(ctx, results, filediffs, strip)
+	filediffs []*diff.FileDiff, strip int, toolname string, filterMode FilterMode, failOnError bool) error {
+	return (&Reviewdog{c: c, toolname: toolname, filterMode: filterMode, failOnError: failOnError}).runFromResult(ctx, results, filediffs, strip, failOnError)
 }
 
 // CheckResult represents a checked result of static analysis tools.
@@ -75,13 +76,14 @@ type DiffService interface {
 }
 
 func (w *Reviewdog) runFromResult(ctx context.Context, results []*CheckResult,
-	filediffs []*diff.FileDiff, strip int) error {
+	filediffs []*diff.FileDiff, strip int, failOnError bool) error {
 	wd, err := os.Getwd()
 	if err != nil {
 		return err
 	}
 
 	checks := FilterCheck(results, filediffs, strip, wd, w.filterMode)
+	hasViolations := false
 
 	for _, check := range checks {
 		if !check.InDiff {
@@ -96,12 +98,17 @@ func (w *Reviewdog) runFromResult(ctx context.Context, results []*CheckResult,
 		if err := w.c.Post(ctx, comment); err != nil {
 			return err
 		}
+		hasViolations = true
 	}
 
 	if bulk, ok := w.c.(BulkCommentService); ok {
 		return bulk.Flush(ctx)
 	}
 
+	if failOnError && hasViolations {
+		return fmt.Errorf("input data has violations")
+	}
+
 	return nil
 }
 
@@ -122,5 +129,5 @@ func (w *Reviewdog) Run(ctx context.Context, r io.Reader) error {
 		return fmt.Errorf("fail to parse diff: %v", err)
 	}
 
-	return w.runFromResult(ctx, results, filediffs, w.d.Strip())
+	return w.runFromResult(ctx, results, filediffs, w.d.Strip(), w.failOnError)
 }


Test patch:
diff --git a/project/run_test.go b/project/run_test.go
index 41ac735b..3dba6b20 100644
--- a/project/run_test.go
+++ b/project/run_test.go
@@ -38,7 +38,7 @@ func TestRun(t *testing.T) {
 
 	t.Run("empty", func(t *testing.T) {
 		conf := &Config{}
-		if err := Run(ctx, conf, nil, nil, nil, false, reviewdog.FilterModeAdded); err != nil {
+		if err := Run(ctx, conf, nil, nil, nil, false, reviewdog.FilterModeAdded, false); err != nil {
 			t.Error(err)
 		}
 	})
@@ -49,7 +49,7 @@ func TestRun(t *testing.T) {
 				"test": {},
 			},
 		}
-		if err := Run(ctx, conf, nil, nil, nil, false, reviewdog.FilterModeAdded); err == nil {
+		if err := Run(ctx, conf, nil, nil, nil, false, reviewdog.FilterModeAdded, false); err == nil {
 			t.Error("want error, got nil")
 		} else {
 			t.Log(err)
@@ -70,7 +70,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, nil, nil, ds, false, reviewdog.FilterModeAdded); err == nil {
+		if err := Run(ctx, conf, nil, nil, ds, false, reviewdog.FilterModeAdded, false); err == nil {
 			t.Error("want error, got nil")
 		} else {
 			t.Log(err)
@@ -98,7 +98,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, nil, cs, ds, false, reviewdog.FilterModeAdded); err != nil {
+		if err := Run(ctx, conf, nil, cs, ds, false, reviewdog.FilterModeAdded, false); err != nil {
 			t.Error(err)
 		}
 		want := ""
@@ -156,7 +156,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, nil, cs, ds, true, reviewdog.FilterModeAdded); err == nil {
+		if err := Run(ctx, conf, nil, cs, ds, true, reviewdog.FilterModeAdded, false); err == nil {
 			t.Error("want error, got nil")
 		} else {
 			t.Log(err)
@@ -186,7 +186,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, nil, cs, ds, false, reviewdog.FilterModeAdded); err != nil {
+		if err := Run(ctx, conf, nil, cs, ds, false, reviewdog.FilterModeAdded, false); err != nil {
 			t.Error(err)
 		}
 	})
@@ -212,7 +212,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, nil, cs, ds, true, reviewdog.FilterModeAdded); err != nil {
+		if err := Run(ctx, conf, nil, cs, ds, true, reviewdog.FilterModeAdded, false); err != nil {
 			t.Error(err)
 		}
 		want := "hi\n"
@@ -248,7 +248,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, map[string]bool{"test2": true}, cs, ds, false, reviewdog.FilterModeAdded); err != nil {
+		if err := Run(ctx, conf, map[string]bool{"test2": true}, cs, ds, false, reviewdog.FilterModeAdded, false); err != nil {
 			t.Error(err)
 		}
 		if called != 1 {
@@ -281,7 +281,7 @@ func TestRun(t *testing.T) {
 				},
 			},
 		}
-		if err := Run(ctx, conf, map[string]bool{"hoge": true}, cs, ds, false, reviewdog.FilterModeAdded); err == nil {
+		if err := Run(ctx, conf, map[string]bool{"hoge": true}, cs, ds, false, reviewdog.FilterModeAdded, false); err == nil {
 			t.Error("got no error but want runner not found error")
 		}
 	})
diff --git a/reviewdog_test.go b/reviewdog_test.go
index 6d7bf81c..a3713314 100644
--- a/reviewdog_test.go
+++ b/reviewdog_test.go
@@ -47,7 +47,7 @@ golint.new.go:11:1: comment on exported function F2 should be of the form "F2 ..
 	p := NewErrorformatParser(efm)
 	c := NewRawCommentWriter(os.Stdout)
 	d := NewDiffString(difftext, 1)
-	app := NewReviewdog("tool name", p, c, d, FilterModeAdded)
+	app := NewReviewdog("tool name", p, c, d, FilterModeAdded, false)
 	app.Run(context.Background(), strings.NewReader(lintresult))
 	// Unordered output:
 	// golint.new.go:5:5: exported var NewError1 should have comment or be unexported
@@ -93,6 +93,79 @@ index 34cacb9..a727dd3 100644
 	efm, _ := errorformat.NewErrorformat([]string{`%f:%l:%c: %m`})
 	p := NewErrorformatParser(efm)
 	d := NewDiffString(difftext, 1)
-	app := NewReviewdog("tool name", p, c, d, FilterModeAdded)
+	app := NewReviewdog("tool name", p, c, d, FilterModeAdded, false)
 	app.Run(context.Background(), strings.NewReader(lintresult))
 }
+
+func TestReviewdog_Run_returns_nil_if_fail_on_error_not_passed_and_some_errors_found(t *testing.T) {
+	difftext := `diff --git a/golint.old.go b/golint.new.go
+index 34cacb9..a727dd3 100644
+--- a/golint.old.go
++++ b/golint.new.go
+@@ -2,6 +2,12 @@ package test
+
+ var V int
+
++var NewError1 int
++
+ // invalid func comment
+ func F() {
+ }
++
++// invalid func comment2
++func F2() {
++}
+`
+	lintresult := `golint.new.go:3:5: exported var V should have comment or be unexported
+golint.new.go:5:5: exported var NewError1 should have comment or be unexported
+golint.new.go:7:1: comment on exported function F should be of the form "F ..."
+golint.new.go:11:1: comment on exported function F2 should be of the form "F2 ..."
+`
+
+	c := NewRawCommentWriter(os.Stdout)
+	efm, _ := errorformat.NewErrorformat([]string{`%f:%l:%c: %m`})
+	p := NewErrorformatParser(efm)
+	d := NewDiffString(difftext, 1)
+	app := NewReviewdog("tool name", p, c, d, FilterModeAdded, false)
+	err := app.Run(context.Background(), strings.NewReader(lintresult))
+
+	if err != nil {
+		t.Errorf("No errors expected, but got %v", err)
+	}
+}
+
+func TestReviewdog_Run_returns_error_if_fail_on_error_passed_and_some_errors_found(t *testing.T) {
+	difftext := `diff --git a/golint.old.go b/golint.new.go
+index 34cacb9..a727dd3 100644
+--- a/golint.old.go
++++ b/golint.new.go
+@@ -2,6 +2,12 @@ package test
+
+ var V int
+
++var NewError1 int
++
+ // invalid func comment
+ func F() {
+ }
++
++// invalid func comment2
++func F2() {
++}
+`
+	lintresult := `golint.new.go:3:5: exported var V should have comment or be unexported
+golint.new.go:5:5: exported var NewError1 should have comment or be unexported
+golint.new.go:7:1: comment on exported function F should be of the form "F ..."
+golint.new.go:11:1: comment on exported function F2 should be of the form "F2 ..."
+`
+	c := NewRawCommentWriter(os.Stdout)
+	efm, _ := errorformat.NewErrorformat([]string{`%f:%l:%c: %m`})
+	p := NewErrorformatParser(efm)
+	d := NewDiffString(difftext, 1)
+	app := NewReviewdog("tool name", p, c, d, FilterModeAdded, true)
+	err := app.Run(context.Background(), strings.NewReader(lintresult))
+
+	if err != nil && err.Error() != "input data has violations" {
+		t.Errorf("'input data has violations' expected, but got %v", err)
+	}
+}