search

golangci / golangci-lint

Fast linters runner for Go
https://golangci-lint.run
GNU General Public License v3.0
15.36k stars 1.37k forks source link

golangci-lint cli returning non-zero value for issues raised at a severity level not of error #1981

Open jufemaiz opened 3 years ago

jufemaiz commented 3 years ago
Running golangci-lint CLI with severity set to `info` for a given linter returns a non-0 value
v1.39.0, v1.40.0
Config file ```console --- linters: # please, do not use `enable-all`: it's deprecated and will be removed soon. # inverted configuration with `enable-all` and `disable` is not scalable during updates of golangci-lint disable-all: true enable: - dupl - godox issues: # List of regexps of issue texts to exclude, empty list by default. # But independently from this option we use default exclude patterns, # it can be disabled by `exclude-use-default: false`. To list all # excluded by default patterns execute `golangci-lint run --help` exclude: - 'declaration of "(err|ctx)" shadows declaration at' # Excluding configuration per-path, per-linter, per-text and per-source exclude-rules: - path: _test\.go linters: - dupl run: timeout: 5m skip-dirs: - test/testdata_etc - internal/cache - internal/renameio - internal/robustio - tmp/ # golangci.com configuration # https://github.com/golangci/golangci/wiki/Configuration service: golangci-lint-version: 1.40.x prepare: - echo "here I can run custom commands, but no preparation needed for this repo" severity: # Default value is empty string. # Set the default severity for issues. If severity rules are defined and the issues # do not match or no severity is provided to the rule this will be the default # severity applied. Severities should match the supported severity names of the # selected out format. # - Code climate: https://docs.codeclimate.com/docs/issues#issue-severity # - Checkstyle: https://checkstyle.sourceforge.io/property_types.html#severity # - Github: https://help.github.com/en/actions/reference/workflow-commands-for-github-actions#setting-an-error-message default-severity: error # The default value is false. # If set to true severity-rules regular expressions become case sensitive. case-sensitive: false # Default value is empty list. # When a list of severity rules are provided, severity information will be added to lint # issues. Severity rules have the same filtering capability as exclude rules except you # are allowed to specify one matcher per severity rule. # Only affects out formats that support setting severity information. rules: - severity: info linters: - dupl - godox ```
Go environment ```console go version go1.16.3 darwin/amd64 GO111MODULE="" GOARCH="amd64" GOBIN="" GOCACHE="/Users/joel/Library/Caches/go-build" GOENV="/Users/joel/Library/Application Support/go/env" GOEXE="" GOFLAGS="" GOHOSTARCH="amd64" GOHOSTOS="darwin" GOINSECURE="" GOMODCACHE="/Users/joel/go/pkg/mod" GONOPROXY="github.com/enosi" GONOSUMDB="github.com/enosi" GOOS="darwin" GOPATH="/Users/joel/go" GOPRIVATE="github.com/enosi" GOPROXY="https://proxy.golang.org,direct" GOROOT="/usr/local/Cellar/go/1.16.3/libexec" GOSUMDB="sum.golang.org" GOTMPDIR="" GOTOOLDIR="/usr/local/Cellar/go/1.16.3/libexec/pkg/tool/darwin_amd64" GOVCS="" GOVERSION="go1.16.3" GCCGO="gccgo" AR="ar" CC="clang" CXX="clang++" CGO_ENABLED="1" GOMOD="/Users/joel/Sites/git/opensource/golangci-lint-check-exit/go.mod" CGO_CFLAGS="-g -O2" CGO_CPPFLAGS="" CGO_CXXFLAGS="-g -O2" CGO_FFLAGS="-g -O2" CGO_LDFLAGS="-g -O2" PKG_CONFIG="pkg-config" GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/9q/v1dfygx14czghp71cfmb68r00000gn/T/go-build3379489850=/tmp/go-build -gno-record-gcc-switches -fno-common" ```
Verbose output of running ```bash #!/bin/bash version="${1:-"v1.40.0"}" docker run --rm -v $(pwd)/:/app -w /app golangci/golangci-lint:$version golangci-lint run -v ./withtodo/... output=$? echo "Checking error output with 'TODO':" if [ "$output" == "1" ]; then echo "output error = '$output'" else echo "output passed with '$output'" fi docker run --rm -v $(pwd)/:/app -w /app golangci/golangci-lint:$version golangci-lint run -v ./withtoddo/... output=$? echo "Checking error output with 'TODDO':" if [ "$output" == "1" ]; then echo "output error = '$output'" else echo "output passed with '$output'" fi exit 0 ``` ```console level=info msg="[config_reader] Config search paths: [./ /app/withtodo /app / /root]" level=info msg="[config_reader] Used config file .golangci.yml" level=info msg="[lintersdb] Active 2 linters: [dupl godox]" level=info msg="[loader] Go packages loading at mode 575 (deps|exports_file|files|imports|compiled_files|name|types_sizes) took 168.090739ms" level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 3.002419ms" level=info msg="[linters context/goanalysis] analyzers took 1.298209ms with top 10 stages: dupl: 1.246897ms, godox: 51.312µs" withtodo/main.go:5: withtodo/main.go:5: Line contains TODO/BUG/FIXME: "TODO: we need to do a thing." (godox) // TODO: we need to do a thing. level=info msg="[runner] Processors filtering stat (out/in): path_prettifier: 1/1, identifier_marker: 1/1, exclude: 1/1, source_code: 1/1, path_shortener: 1/1, path_prefixer: 1/1, exclude-rules: 1/1, diff: 1/1, severity-rules: 1/1, sort_results: 1/1, cgo: 1/1, skip_dirs: 1/1, autogenerated_exclude: 1/1, nolint: 1/1, uniq_by_line: 1/1, max_per_file_from_linter: 1/1, max_from_linter: 1/1, filename_unadjuster: 1/1, skip_files: 1/1, max_same_issues: 1/1" level=info msg="[runner] processing took 3.696987ms with stages: autogenerated_exclude: 1.296595ms, nolint: 1.128277ms, source_code: 985.142µs, exclude-rules: 132.897µs, identifier_marker: 86.003µs, path_prettifier: 27.022µs, skip_dirs: 8.686µs, exclude: 7.224µs, uniq_by_line: 5.925µs, path_shortener: 4.353µs, max_same_issues: 3.49µs, severity-rules: 3.201µs, cgo: 2.263µs, max_from_linter: 2.018µs, filename_unadjuster: 1.135µs, max_per_file_from_linter: 858ns, diff: 739ns, skip_files: 517ns, sort_results: 488ns, path_prefixer: 154ns" level=info msg="[runner] linters took 48.037957ms with stages: goanalysis_metalinter: 43.912198ms" level=info msg="File cache stats: 1 entries of total size 137B" level=info msg="Memory: 4 samples, avg is 71.1MB, max is 71.3MB" level=info msg="Execution took 252.610194ms" Checking error output with 'TODO': output error = '1' level=info msg="[config_reader] Config search paths: [./ /app/withtoddo /app / /root]" level=info msg="[config_reader] Used config file .golangci.yml" level=info msg="[lintersdb] Active 2 linters: [dupl godox]" level=info msg="[loader] Go packages loading at mode 575 (types_sizes|compiled_files|deps|files|imports|exports_file|name) took 165.866778ms" level=info msg="[runner/filename_unadjuster] Pre-built 0 adjustments in 1.35967ms" level=info msg="[linters context/goanalysis] analyzers took 1.755392ms with top 10 stages: dupl: 1.673776ms, godox: 81.616µs" level=info msg="[runner] processing took 3.434µs with stages: max_same_issues: 582ns, nolint: 353ns, skip_dirs: 326ns, filename_unadjuster: 289ns, path_prettifier: 256ns, max_from_linter: 244ns, cgo: 181ns, autogenerated_exclude: 162ns, identifier_marker: 160ns, skip_files: 154ns, diff: 145ns, uniq_by_line: 141ns, max_per_file_from_linter: 70ns, sort_results: 60ns, severity-rules: 59ns, path_shortener: 53ns, exclude: 51ns, source_code: 51ns, exclude-rules: 49ns, path_prefixer: 48ns" level=info msg="[runner] linters took 38.026854ms with stages: goanalysis_metalinter: 37.935128ms" level=info msg="File cache stats: 0 entries of total size 0B" level=info msg="Memory: 4 samples, avg is 71.1MB, max is 71.3MB" level=info msg="Execution took 216.133839ms" Checking error output with 'TODDO': output passed with '0' ```
Code example or link to a public repository https://github.com/jufemaiz/golangci-lint-check-exit
boring-cyborg[bot] commented 3 years ago

Hey, thank you for opening your first Issue ! 🙂 If you would like to contribute we have a guide for contributors.

ldez commented 3 years ago

Hello,

The severity configuration doesn't change the exit code but adds/changes the field severity into some output format.

There is no correlation between severity and exit code, then there is no bug.

It kind of duplicate of #1856

jufemaiz commented 3 years ago

I would say it's more a feature request than a question if it's not a bug.

If we look at the general conventions around linting, non-errors shouldn't return an error code. This has been implemented in a few ways:

There's a few patterns in use here between these three and I believe it would be very useful to have at least some solution that allows for the reporting, however, and importantly, not the failing, of the outcome based on a desired level of failure. That could be adopting a flag on the severity level to drive the exit code, or alternatively to provide an approach like pylint where the exit code indicates which lints have failed at each of the severity types, then letting the user determine if that indeed represents a failure or not.

If there's no correlation between severity and the exit code, what purpose does severity serve? Merely a presentation formatting of some of the linters? This seems short sighted in terms of the goal of having multiple severity levels. If that is indeed the case, then I believe the documentation should be updated to reflect this and emphasise that this is for decorative purposes and not to drive exit codes of the linter.

jgallucci32 commented 3 years ago

An example of this can be found in the Ruby linter rubocop

--fail-level flag to allow non-errors (or other levels) to be reported but not fail the linting process.

Adding a flag such as this would enable the linter to fail on certain errors but return a zero value for other levels. This is important because if you exclude items from the linter they will not appear in the report. Later on you may decide you want to fail a particular error as the program matures and you want to restrict (or loosen) some of the linting properties.

armsnyder commented 3 years ago

Some kind of exit-code-filtering feature would be very useful.

For example, golangci-lint today can generate code quality reports for GitLab merge requests, which use the Code Climate format that golangci-lint supports. GitLab displays new issues on merge requests and breaks them down by severity level.

I would really like to enable the godox linter with severity=info so that TODOs are shown in the quality report, but at the same time, I don't want these issues to cause errors when a developer runs golangci-lint locally before committing.

Today the workaround would be to enable the godox linter using a commandline flag in the CI/CD script, to override the config file. However this does not scale to more linters, where it would become necessary to manage two different configs.

armsnyder commented 3 years ago

How about adding a new field to the config.SeverityRule struct? It could be IssuesExitCode *int, to align with the existing global --issues-exit-code commandline arg.

I imagine this would be where you want to configure this, since golangci-lint doesn't know how to sort severities.

https://github.com/golangci/golangci-lint/blob/ee30b44e4e770bdc8879c9622a288d099d12a6a6/pkg/config/severity.go#L11-L14

Example:

severity:
  default-severity: major
  rules:
    - severity: minor
      linters:
        - funlen
    - severity: info
      issuesExitCode: 0
      linters:
        - godox
butuzov commented 3 years ago

I was going to work on this (and severity coloring) feature starting next month. Feel free to pick it up and make PR, if you don't want to wait.

kavu commented 2 years ago

@ldez @butuzov Hi guys! Any news on this? Maybe you can give a tip on how it should be implemented properly, so I can contribute? Thanks in advance!

wrouesnel commented 1 year ago

+1 for this - it needs to be command line configurable easily.

The worst offenders are the documentation-type linters, which provide good actionable items, but shouldn't block builds in CI but also should be kept visible.

The essence of the problem here is that the alternative is maintaining and synchronizing multiple, fairly complicated linter configs per project for different scenarios.

i.e. issues with variable naming (revive and the like) really should block before people commit code. But documentation can be fixed at any time and won't effect interfaces...but it's nice to know about it.

Currently though, there's no option: I can't have my linter scripts just say --exit-at-or-above "error" when running the linters at a git commit hook, and then have a separate pre-release check set to a lower --exit-at-or-above "warning" to gate my release builds. It's all or nothing (or duplicating config files).

butuzov commented 1 year ago

@wrouesnel I will return to this feature once war ends.

jufemaiz commented 1 year ago

You have far bigger concerns to focus on right now @butuzov. Stay safe.

ImprintNav commented 1 year ago

Would anyone else be able to work on this ticket while @butuzov is indisposed? I could take a look at it myself.

ldez commented 1 year ago

https://github.com/golangci/golangci-lint/pull/2595#issuecomment-1043739800

https://github.com/golangci/golangci-lint/pull/3184#issuecomment-1235438429

abemedia commented 7 months ago

I'd be happy to have a look at this if there's agreement on how it should work. Personally, I'm in favour of returning a non-zero exit code for anything that isn't error and defaulting to error if severity.default-severity is not set, however I also don't mind making it explicit.

We could add a new property to severity e.g.

severity:
  default-severity: error
  severities:
    error:
      exit-code: 1
    warning:
      exit-code: 0

Alternatively, if we're certain there will never be more settings on the severities, it could also be like so:

severity:
  default-severity: error
  exit-codes:
    error: 1
    warning: 0

Or it could be part of the rule config e.g.

severity:
  default-severity: error
  rules:
    - linters:
        - godox
      severity: warning
      exit-code: 0

I'm not a huge fan of the latter though, as I feel the exit code should be tied to the severity, not the rule.

jeeftor commented 1 month ago

I think it's important to allow certain linters to override the default setting. My use case is in CI I want some linters to be fails and some to be more like "Well you should get to this at some point"