Skip to content

Commit

Permalink
Merge pull request #1854 from reviewdog/exit-code
Browse files Browse the repository at this point in the history
introduce -fail-level flag and deprecate -fail-on-error
  • Loading branch information
haya14busa authored Aug 1, 2024
2 parents 494e65f + 3788f0a commit 6720b05
Show file tree
Hide file tree
Showing 9 changed files with 242 additions and 41 deletions.
26 changes: 24 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
<!-- TODO: update the v0.19.0 release section -->

## [v0.20.1] - 2024-07-10

Expand Down Expand Up @@ -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.
<!-- TODO: replace upcoming release with next released version -->
The previous behavior can be restored with `-fail-level=error` flag with the
upcoming release.

## [v0.18.1] - 2024-06-22

### :bug: Fixes
Expand Down
7 changes: 3 additions & 4 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -904,12 +904,11 @@ $ REVIEWDOG_SKIP_DOGHOUSE=true REVIEWDOG_GITHUB_API_TOKEN="<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.
Expand Down
24 changes: 21 additions & 3 deletions cmd/reviewdog/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type option struct {
tee bool
filterMode filter.Mode
failOnError bool
failLevel reviewdog.FailLevel
logLevel string
}

Expand Down Expand Up @@ -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)`
)

Expand All @@ -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)
}

Expand Down Expand Up @@ -465,15 +468,15 @@ 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)
if err != nil {
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)
}

Expand Down Expand Up @@ -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
}
88 changes: 88 additions & 0 deletions fail_level.go
Original file line number Diff line number Diff line change
@@ -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
}
}
73 changes: 73 additions & 0 deletions fail_level_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
5 changes: 3 additions & 2 deletions project/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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()
Expand Down
20 changes: 10 additions & 10 deletions project/run_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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 := ""
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
})
Expand All @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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")
}
})
Expand Down
Loading

0 comments on commit 6720b05

Please sign in to comment.