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

[jufemaiz]

• [X] Yes, I'm using a binary release within 2 latest major releases. Only such installations are supported.
• [X] Yes, I've searched similar issues on GitHub and didn't find any.
• [X] Yes, I've included all information below (version, config, etc).
• [x] Yes, I've tried with the standalone linter if available. (https://golangci-lint.run/usage/linters/) (not applicable)

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

---
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

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

#!/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

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]]

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

[ldez]

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]

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:

• Rubocop --fail-level flag to allow non-errors (or other levels) to be reported but not fail the linting process.
• eslint only returns error code for either any errors, or with the -max-warning flag can set the maximum number of warnings acceptable before becoming returning an error code.
• pylint provides a different approach, wherein the exit code is bit encoded, thus providing the user to determine the next step based on the presence, or absence, of linting issues in the various severity levels (supported: https://github.com/PyCQA/pylint/blob/af52033971eccecea47597ebbfaeac15773b3e1b/pylint/constants.py#L25-L32) along with --fail-on and --fail-under cli flags.

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]

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]

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]

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]

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]

@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]

+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]

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

[jufemaiz]

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

[ImprintNav]

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

[ldez]

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

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

[abemedia]

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.