diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a6c22ea..55c4e4a9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,11 +12,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [#1845](https://github.com/reviewdog/reviewdog/pull/1845) Normalize file path in `related_locations` too. ### :bug: Fixes -- ... +- [#1846](https://github.com/reviewdog/reviewdog/issues/1846) Unexpected error + exit code when using `-reporter=github-[pr-]check` with + `-fail-on-error=true`. See the below breaking changes section for more details. ### :rotating_light: Breaking changes - [#1858](https://github.com/reviewdog/reviewdog/pull/1858) Remove original_output from rdjson/rdjsonl reporters' output -- ... + +### :rotating_light: Deprecation Warnings +- [#1854](https://github.com/reviewdog/reviewdog/pull/1854) (Actually No Breaking changes) `-fail-on-error` + flag is deprecated. Use `-fail-level=[none,any,error,warning,info]` flag + instead. You can reproduce the same behavior with `-fail-level=any` for most + reporters (reviewdog will exit with 1 if it find any severity level). + As for `github-[pr-]check` reporter you can reproduce the same behavior with + `-fail-level=error` ([#1846](https://github.com/reviewdog/reviewdog/issues/1846)). + + `-fail-on-error` is just deprecated and it's not removed yet. Please update + the flag while it's working. + ## [v0.20.1] - 2024-07-10 @@ -48,6 +61,15 @@ We left both versions as-is so that it won't break existing clients who already - [#1809](https://github.com/reviewdog/reviewdog/pull/1809) Support posting comments outside diff context as a file comment in github-pr-review reporter +### :rotating_light: Breaking changes +- [#1846](https://github.com/reviewdog/reviewdog/issues/1846) the behavior of + `-fail-on-error` changed with `-github-[pr-]check`. Previously, reviewdog + didn't exit with 1 if it find issues with warning and info level. Now + reviewdog will exit with 1 with any level. + + The previous behavior can be restored with `-fail-level=error` flag with the + upcoming release. + ## [v0.18.1] - 2024-06-22 ### :bug: Fixes diff --git a/README.md b/README.md index 1ded0e87..c246035c 100644 --- a/README.md +++ b/README.md @@ -904,12 +904,11 @@ $ REVIEWDOG_SKIP_DOGHOUSE=true REVIEWDOG_GITHUB_API_TOKEN="" reviewdog -r ``` ## Exit codes -By default reviewdog will return `0` as exit code even if it finds errors. -If `-fail-on-error` flag is passed, reviewdog exits with `1` when at least one error was found/reported. +By default (`-fail-level=none`) reviewdog will return `0` as exit code even if it finds errors. +reviewdog will exit with code 1 with `-fail-level=[any,info,warning,error]` flag if it finds at least 1 issue with severity greater than or equal to the given level. 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. -See also `-level` flag for [github-pr-check/github-check](#reporter-github-checks--reportergithub-pr-check) reporters. -reviewdog will exit with `1` if reported check status is `failure` as well if `-fail-on-error=true`. +You can also use `-level` flag to configure default report revel. ## Filter mode reviewdog filter results by diff and you can control how reviewdog filter results by `-filter-mode` flag. diff --git a/cmd/reviewdog/main.go b/cmd/reviewdog/main.go index c5c463e6..f905c3bc 100644 --- a/cmd/reviewdog/main.go +++ b/cmd/reviewdog/main.go @@ -62,6 +62,7 @@ type option struct { tee bool filterMode filter.Mode failOnError bool + failLevel reviewdog.FailLevel logLevel string } @@ -202,7 +203,8 @@ 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` + failOnErrorDoc = `[DEPRECATED] use -fail-level instead` + failLevelDoc = `reviewdog will exit with code 1 if it finds at least 1 issue with severity greater than or equal to the given level. [none(default),any,info,warning,error]` logLevelDoc = `log level for reviewdog itself. (debug, info, warning, error)` ) @@ -225,6 +227,7 @@ func init() { flag.BoolVar(&opt.tee, "tee", false, teeDoc) flag.Var(&opt.filterMode, "filter-mode", filterModeDoc) flag.BoolVar(&opt.failOnError, "fail-on-error", false, failOnErrorDoc) + flag.Var(&opt.failLevel, "fail-level", failLevelDoc) flag.StringVar(&opt.logLevel, "log-level", "info", logLevelDoc) } @@ -465,7 +468,7 @@ func run(r io.Reader, w io.Writer, opt *option) error { } if isProject { - return project.Run(ctx, projectConf, buildRunnersMap(opt.runners), cs, ds, opt.tee, opt.filterMode, opt.failOnError) + return project.Run(ctx, projectConf, buildRunnersMap(opt.runners), cs, ds, opt.tee, opt.filterMode, failLevel(opt)) } p, err := newParserFromOpt(opt) @@ -473,7 +476,7 @@ func run(r io.Reader, w io.Writer, opt *option) error { return err } - app := reviewdog.NewReviewdog(toolName(opt), p, cs, ds, opt.filterMode, opt.failOnError) + app := reviewdog.NewReviewdog(toolName(opt), p, cs, ds, opt.filterMode, failLevel(opt)) return app.Run(ctx, r) } @@ -1029,3 +1032,18 @@ func localDiffService(opt *option) (reviewdog.DiffService, error) { } return diffService(opt.diffCmd, opt.diffStrip) } + +func failLevel(opt *option) reviewdog.FailLevel { + if opt.failOnError { + slog.Warn("reviewdog: -fail-on-error is deprecated. Use -fail-level=any, or -fail-level=error for github-[pr-]check reporter instead. See also https://github.com/reviewdog/reviewdog/blob/master/CHANGELOG.md") + if opt.failLevel == reviewdog.FailLevelDefault { + switch opt.reporter { + default: + return reviewdog.FailLevelAny + case "github-check", "github-pr-check": + return reviewdog.FailLevelError + } + } + } + return opt.failLevel +} diff --git a/fail_level.go b/fail_level.go new file mode 100644 index 00000000..eb737f7e --- /dev/null +++ b/fail_level.go @@ -0,0 +1,88 @@ +package reviewdog + +import ( + "fmt" + + "github.com/reviewdog/reviewdog/proto/rdf" +) + +// FailLevel represents enumeration of available filter modes +type FailLevel int + +const ( + // FailLevelDefault represents default mode, which means users doesn't specify + // fail-level. Basically, it's same as FailLevelNone. + FailLevelDefault FailLevel = iota + // FailLevelNone - Do not fail. + FailLevelNone + // FailLevelAny - Fail with any severity. + FailLevelAny + // FailLevelInfo - Fail with info or above severity. + FailLevelInfo + // FailLevelWarning - Fail with warning or above severity. + FailLevelWarning + // FailLevelError - Fail with error or above severity. + FailLevelError +) + +// String implements the flag.Value interface +func (failLevel *FailLevel) String() string { + names := [...]string{ + "default", + "none", + "any", + "info", + "warning", + "error", + } + if *failLevel < FailLevelDefault || *failLevel > FailLevelError { + return "Unknown failLevel" + } + + return names[*failLevel] +} + +// Set implements the flag.Value interface +func (failLevel *FailLevel) Set(value string) error { + switch value { + case "default", "": + *failLevel = FailLevelDefault + case "none": + *failLevel = FailLevelNone + case "any": + *failLevel = FailLevelAny + case "info": + *failLevel = FailLevelInfo + case "warning": + *failLevel = FailLevelWarning + case "error": + *failLevel = FailLevelError + default: + return fmt.Errorf("invalid failLevel name: %s", value) + } + return nil +} + +// ShouldFail returns true if reviewdog should exit with 1 with given rdf.Severity. +func (failLevel FailLevel) ShouldFail(severity rdf.Severity) bool { + if failLevel == FailLevelDefault || failLevel == FailLevelNone { + return false + } + minSeverity := failLevel.minSeverity() + return minSeverity == rdf.Severity_UNKNOWN_SEVERITY || severity <= minSeverity +} + +func (failLevel FailLevel) minSeverity() rdf.Severity { + switch failLevel { + case FailLevelDefault, FailLevelNone, FailLevelAny: + return rdf.Severity_UNKNOWN_SEVERITY + case FailLevelInfo: + return rdf.Severity_INFO + case FailLevelWarning: + return rdf.Severity_WARNING + case FailLevelError: + return rdf.Severity_ERROR + default: + return rdf.Severity_UNKNOWN_SEVERITY + } +} diff --git a/fail_level_test.go b/fail_level_test.go new file mode 100644 index 00000000..2b5ee85d --- /dev/null +++ b/fail_level_test.go @@ -0,0 +1,73 @@ +package reviewdog + +import ( + "testing" + + "github.com/reviewdog/reviewdog/proto/rdf" +) + +func TestShouldFail(t *testing.T) { + tests := []struct { + failLevel FailLevel + severity rdf.Severity + want bool + }{ + { + failLevel: FailLevelDefault, severity: rdf.Severity_ERROR, want: false, + }, + { + failLevel: FailLevelDefault, severity: rdf.Severity_WARNING, want: false, + }, + { + failLevel: FailLevelDefault, severity: rdf.Severity_INFO, want: false, + }, + { + failLevel: FailLevelDefault, severity: rdf.Severity_UNKNOWN_SEVERITY, want: false, + }, + { + failLevel: FailLevelNone, severity: rdf.Severity_ERROR, want: false, + }, + { + failLevel: FailLevelError, severity: rdf.Severity_ERROR, want: true, + }, + { + failLevel: FailLevelError, severity: rdf.Severity_WARNING, want: false, + }, + { + failLevel: FailLevelError, severity: rdf.Severity_INFO, want: false, + }, + { + failLevel: FailLevelError, severity: rdf.Severity_UNKNOWN_SEVERITY, want: true, + }, + { + failLevel: FailLevelWarning, severity: rdf.Severity_ERROR, want: true, + }, + { + failLevel: FailLevelWarning, severity: rdf.Severity_WARNING, want: true, + }, + { + failLevel: FailLevelWarning, severity: rdf.Severity_INFO, want: false, + }, + { + failLevel: FailLevelWarning, severity: rdf.Severity_UNKNOWN_SEVERITY, want: true, + }, + { + failLevel: FailLevelInfo, severity: rdf.Severity_ERROR, want: true, + }, + { + failLevel: FailLevelInfo, severity: rdf.Severity_WARNING, want: true, + }, + { + failLevel: FailLevelInfo, severity: rdf.Severity_INFO, want: true, + }, + { + failLevel: FailLevelInfo, severity: rdf.Severity_UNKNOWN_SEVERITY, want: true, + }, + } + for _, tt := range tests { + if got := tt.failLevel.ShouldFail(tt.severity); got != tt.want { + t.Errorf("FailLevel(%s).ShouldFail(%s) = %v, want %v", tt.failLevel.String(), + tt.severity, got, tt.want) + } + } +} diff --git a/project/run.go b/project/run.go index d52f03cf..7f21e0f8 100644 --- a/project/run.go +++ b/project/run.go @@ -91,7 +91,8 @@ 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 filter.Mode, failOnError bool) error { +func Run(ctx context.Context, conf *Config, runners map[string]bool, c reviewdog.CommentService, d reviewdog.DiffService, + teeMode bool, filterMode filter.Mode, failLevel reviewdog.FailLevel) error { results, err := RunAndParse(ctx, conf, runners, "", teeMode) // Level is not used. if err != nil { return err @@ -115,7 +116,7 @@ func Run(ctx context.Context, conf *Config, runners map[string]bool, c reviewdog if err := result.CheckUnexpectedFailure(); err != nil { return err } - return reviewdog.RunFromResult(ctx, c, ds, filediffs, d.Strip(), toolname, filterMode, failOnError) + return reviewdog.RunFromResult(ctx, c, ds, filediffs, d.Strip(), toolname, filterMode, failLevel) }) }) return g.Wait() diff --git a/project/run_test.go b/project/run_test.go index f498e70f..f7594385 100644 --- a/project/run_test.go +++ b/project/run_test.go @@ -40,7 +40,7 @@ func TestRun(t *testing.T) { t.Run("empty", func(t *testing.T) { conf := &Config{} - if err := Run(ctx, conf, nil, nil, nil, false, filter.ModeAdded, false); err != nil { + if err := Run(ctx, conf, nil, nil, nil, false, filter.ModeAdded, reviewdog.FailLevelNone); err != nil { t.Error(err) } }) @@ -51,7 +51,7 @@ func TestRun(t *testing.T) { "test": {}, }, } - if err := Run(ctx, conf, nil, nil, nil, false, filter.ModeAdded, false); err == nil { + if err := Run(ctx, conf, nil, nil, nil, false, filter.ModeAdded, reviewdog.FailLevelNone); err == nil { t.Error("want error, got nil") } else { t.Log(err) @@ -72,7 +72,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, nil, nil, ds, false, filter.ModeAdded, false); err == nil { + if err := Run(ctx, conf, nil, nil, ds, false, filter.ModeAdded, reviewdog.FailLevelNone); err == nil { t.Error("want error, got nil") } else { t.Log(err) @@ -100,7 +100,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, nil, cs, ds, false, filter.ModeAdded, false); err != nil { + if err := Run(ctx, conf, nil, cs, ds, false, filter.ModeAdded, reviewdog.FailLevelNone); err != nil { t.Error(err) } want := "" @@ -130,7 +130,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, nil, cs, ds, false, filter.ModeAdded, false); err == nil { + if err := Run(ctx, conf, nil, cs, ds, false, filter.ModeAdded, reviewdog.FailLevelNone); err == nil { t.Error("want error, got nil") } else { t.Log(err) @@ -158,7 +158,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, nil, cs, ds, true, filter.ModeAdded, false); err == nil { + if err := Run(ctx, conf, nil, cs, ds, true, filter.ModeAdded, reviewdog.FailLevelNone); err == nil { t.Error("want error, got nil") } else { t.Log(err) @@ -193,7 +193,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, nil, cs, ds, false, filter.ModeAdded, false); err != nil { + if err := Run(ctx, conf, nil, cs, ds, false, filter.ModeAdded, reviewdog.FailLevelNone); err != nil { t.Error(err) } }) @@ -219,7 +219,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, nil, cs, ds, true, filter.ModeAdded, false); err != nil { + if err := Run(ctx, conf, nil, cs, ds, true, filter.ModeAdded, reviewdog.FailLevelNone); err != nil { t.Error(err) } want := "hi\n" @@ -255,7 +255,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, map[string]bool{"test2": true}, cs, ds, false, filter.ModeAdded, false); err != nil { + if err := Run(ctx, conf, map[string]bool{"test2": true}, cs, ds, false, filter.ModeAdded, reviewdog.FailLevelNone); err != nil { t.Error(err) } if called != 1 { @@ -288,7 +288,7 @@ func TestRun(t *testing.T) { }, }, } - if err := Run(ctx, conf, map[string]bool{"hoge": true}, cs, ds, false, filter.ModeAdded, false); err == nil { + if err := Run(ctx, conf, map[string]bool{"hoge": true}, cs, ds, false, filter.ModeAdded, reviewdog.FailLevelNone); err == nil { t.Error("got no error but want runner not found error") } }) diff --git a/reviewdog.go b/reviewdog.go index 08e8b5f8..18c1c85a 100644 --- a/reviewdog.go +++ b/reviewdog.go @@ -18,23 +18,23 @@ import ( // or linter, get diff and filter the results by diff, and report filtered // results. type Reviewdog struct { - toolname string - p parser.Parser - c CommentService - d DiffService - filterMode filter.Mode - failOnError bool + toolname string + p parser.Parser + c CommentService + d DiffService + filterMode filter.Mode + failLevel FailLevel } // NewReviewdog returns a new Reviewdog. -func NewReviewdog(toolname string, p parser.Parser, c CommentService, d DiffService, filterMode filter.Mode, failOnError bool) *Reviewdog { - return &Reviewdog{p: p, c: c, d: d, toolname: toolname, filterMode: filterMode, failOnError: failOnError} +func NewReviewdog(toolname string, p parser.Parser, c CommentService, d DiffService, filterMode filter.Mode, failLevel FailLevel) *Reviewdog { + return &Reviewdog{p: p, c: c, d: d, toolname: toolname, filterMode: filterMode, failLevel: failLevel} } // RunFromResult creates a new Reviewdog and runs it with check results. func RunFromResult(ctx context.Context, c CommentService, results []*rdf.Diagnostic, - filediffs []*diff.FileDiff, strip int, toolname string, filterMode filter.Mode, failOnError bool) error { - return (&Reviewdog{c: c, toolname: toolname, filterMode: filterMode, failOnError: failOnError}).runFromResult(ctx, results, filediffs, strip, failOnError) + filediffs []*diff.FileDiff, strip int, toolname string, filterMode filter.Mode, failLevel FailLevel) error { + return (&Reviewdog{c: c, toolname: toolname, filterMode: filterMode, failLevel: failLevel}).runFromResult(ctx, results, filediffs, strip) } // Comment represents a reported result as a comment. @@ -68,7 +68,7 @@ type DiffService interface { } func (w *Reviewdog) runFromResult(ctx context.Context, results []*rdf.Diagnostic, - filediffs []*diff.FileDiff, strip int, failOnError bool) error { + filediffs []*diff.FileDiff, strip int) error { wd, err := os.Getwd() if err != nil { return err @@ -77,7 +77,7 @@ func (w *Reviewdog) runFromResult(ctx context.Context, results []*rdf.Diagnostic pathutil.NormalizePathInResults(results, wd) checks := filter.FilterCheck(results, filediffs, strip, wd, w.filterMode) - hasViolations := false + shouldFail := false for _, check := range checks { comment := &Comment{ @@ -96,7 +96,7 @@ func (w *Reviewdog) runFromResult(ctx context.Context, results []*rdf.Diagnostic if err := w.c.Post(ctx, comment); err != nil { return err } - hasViolations = true + shouldFail = shouldFail || w.failLevel.ShouldFail(check.Diagnostic.GetSeverity()) } } @@ -106,8 +106,8 @@ func (w *Reviewdog) runFromResult(ctx context.Context, results []*rdf.Diagnostic } } - if failOnError && hasViolations { - return fmt.Errorf("input data has violations") + if shouldFail { + return fmt.Errorf("found at least one issue with severity greater than or equal to the given level: %s", w.failLevel.String()) } return nil @@ -130,5 +130,5 @@ func (w *Reviewdog) Run(ctx context.Context, r io.Reader) error { return fmt.Errorf("fail to parse diff: %w", err) } - return w.runFromResult(ctx, results, filediffs, w.d.Strip(), w.failOnError) + return w.runFromResult(ctx, results, filediffs, w.d.Strip()) } diff --git a/reviewdog_test.go b/reviewdog_test.go index 5067e85a..4b612a19 100644 --- a/reviewdog_test.go +++ b/reviewdog_test.go @@ -50,7 +50,7 @@ golint.new.go:11:1: comment on exported function F2 should be of the form "F2 .. p := parser.NewErrorformatParser(efm) c := NewRawCommentWriter(os.Stdout) d := NewDiffString(difftext, 1) - app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, false) + app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, FailLevelDefault) app.Run(context.Background(), strings.NewReader(lintresult)) // Unordered output: // golint.new.go:5:5: exported var NewError1 should have comment or be unexported @@ -96,7 +96,7 @@ index 34cacb9..a727dd3 100644 efm, _ := errorformat.NewErrorformat([]string{`%f:%l:%c: %m`}) p := parser.NewErrorformatParser(efm) d := NewDiffString(difftext, 1) - app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, false) + app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, FailLevelDefault) app.Run(context.Background(), strings.NewReader(lintresult)) } @@ -129,7 +129,7 @@ golint.new.go:11:1: comment on exported function F2 should be of the form "F2 .. efm, _ := errorformat.NewErrorformat([]string{`%f:%l:%c: %m`}) p := parser.NewErrorformatParser(efm) d := NewDiffString(difftext, 1) - app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, false) + app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, FailLevelDefault) err := app.Run(context.Background(), strings.NewReader(lintresult)) if err != nil { @@ -165,7 +165,7 @@ golint.new.go:11:1: comment on exported function F2 should be of the form "F2 .. efm, _ := errorformat.NewErrorformat([]string{`%f:%l:%c: %m`}) p := parser.NewErrorformatParser(efm) d := NewDiffString(difftext, 1) - app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, true) + app := NewReviewdog("tool name", p, c, d, filter.ModeAdded, FailLevelAny) err := app.Run(context.Background(), strings.NewReader(lintresult)) if err != nil && err.Error() != "input data has violations" {