PoC: Add Severity Levels, use it to return zero exit code

[kavu]

Hi!

This is my Proof-of-Concept of how it could be possible to approach problem with non-zero exit codes on issues with non-error level.

I've tested it on very simple severity config:

severity:
  default-severity: error
  case-sensitive: true
  rules:
    - linters:
      - paralleltest
      - tparallel
      severity: warning

Running with golangci-lint run ./... && echo $? in my project with several missing t.Parallel().

I don't insist it is final or ideal, just my view on how it could be done.

What do you think, guys?

Solves #1981

[boring-cyborg]

Hey, thank you for opening your first Pull Request !

[CLAassistant]

All committers have signed the CLA.

[ldez]

#2595 (comment)

[kavu]

@ldez I've seen these discussions, but issues are still, so I decided to take a chance and purpose my solution.

So, do I understand correctly that

Regarding the exit code, as far as I remember, discussions have already taken place on this subject and overall the decision was made not to correlate severity and exit code.

means that there will be no any work done on this matter any time soon and we should try to find alternative solutions if we want to ignore issues that should not result with non-zero code?

[ldez]

As I said, the decision was made not to correlate severity and exit code, for me nothing changed.

For me, it's useless to create a lint report just to ignore it, the correlation between severity and exit code is a bad historical way to manage linting: it's like you write tests but you want to exit 0 even if the tests are failing.

If you want to ignore/skip reports you can read this page: https://golangci-lint.run/usage/false-positives/

[kavu]

@ldez
Okay, got your point. Thank you!

But maybe you can give and advice, how we can approach out problem?

We want to add some linter and we want to give some time to developers to fix it, having some clear indication that this problem exits. But as we don't want to enforce it right away, we don't want to it to break pipelines on non-zero code. That's the reasoning behind my intentions hack this part.

What will be the best approach from your perspective? Thank you!

[bombsimon]

I'm torn but I think I mostly agree with @ldez. Now that we have severity, I can kinda see why you would want to filter them out. Also judging by #1981 (comment) golangci-lint would not be the first linter to allow reports combined with a successful exit code.

Personally I also think that it's a somewhat odd requirement to ask for the reports and then ignoring them, it's like running golangci-lint run || true. To me it's not clear what that information would serve. However now we're more talking about workflows and philosophical questions. The same discussion happens all the time with log severity, f.ex. "Are you supposed to get paged when a log with severity error is written?".

I don't really remember the historical reasons to support severity and even less memory about discussions where we decided to not add support to match it with exit codes. Given that we actually did a change regarding exit codes pretty recently in #2357 and that this seems to be a request from several people I would be open to reconsider this and add support for this.

[bombsimon]

We want to add some linter and we want to give some time to developers to fix it, having some clear indication that this problem exits. But as we don't want to enforce it right away, we don't want to it to break pipelines on non-zero code. That's the reasoning behind my intentions hack this part.

What will be the best approach from your perspective? Thank you!

I would start by using new-from-rev or new with the linters you set to lower severity to only care about new violations. That way you can run CI or any other pipelines with them enabled.

The people that would then iteratively fix old issues could run a different config file that is more strict and fail on these issues, then you can pick this config by using golangci-lint run --config my-strict-config.yaml.

This probably doesn't solve your issue but maybe it could inspire a workflow that suits you.

[ldez]

We want to add some linter and we want to give some time to developers to fix it, ...
But as we don't want to enforce it right away, we don't want to it to break pipelines on non-zero code ...

Why would you want to add a linter but wait to fix its reports?

I think the approach explained by @bombsimon with new-from-rev or new is the right way.

[kavu]

Why would you want to add a linter but wait to fix its reports?

Dummy simple reason — we want to "flash in face" the soon-to-be-errors in the pipeline logs, so it would be visible in case someone will check it or will try to read other linter problems, but not failing the whole.

Yes, totally agree, @bombsimon approach sounds really good!

We also considering creating separate task in the pipeline with the future version of lint config, that won't be blocking anything and later will just swap it with the main task.

Thank you for your time and help, guys!

[jufemaiz]

Why would you want to add a linter but wait to fix its reports?

Ref: #1981 (comment) for a full outline of the reasoning with citations of examples used elsewhere (thus an expectation of the ability in the software engineering space more broadly).

IMHO a severity level implies the ability to filter on the severity level for linting issues. I'm supportive of the change(s) as proposed here. They don't detract from the existing use cases, but allow for additional support as outlined as a requirement. In the absence I would support the removal of severity levels for linting issues as its a binary as to whether or not a linting run will be considered "passed" under the current implementation.

[ImprintNav]

@ldez What's the reasoning for having severity levels if the tool provides no means of treating them differently in terms of CI success or run-time filtering? If everything is blocked at the CI level, you might as well make everything an error. We don't have "warnings" or "info" for unit tests either precisely because we don't want to filter out unit test failures by severity level.

[wolf29f]

Hi,
If this can be of any help for the subject, here's an example use-case of per-severity exit-code: I'm using godox to detect TODO FIXME and alike. I don't want to fail on that as they're task to be done later (maybe future feature or pending fixes independent from the current code review), but I still want them to be annotate in actions as warning so that they don't get forgotten.

In my opinion it makes sens to be able to keep a zero exit code despite having warning, it doesn't mean dismissing the linter report, only that we can keep working until it's fixed.

[ldez]

You can configure godox to allow TODO but forbid FIXME.

# ...

linters-settings:
  godox:
    keywords:
      - FIXME
# ...

You will have 2 levels:

• one, non-blocking, related to tasks you will do in the future. (TODO)
• one, blocking, related to tasks you should do before your commit/PR/merge. (FIXME)

I will add an element: the goal of golangci-lint has always been to reduce and not report false positives. golangci-lint tries to provide the most effective information, non-blocking reports are the opposite of that.

[abemedia]

I'd love to see something like this go in. The reason for this is that I want godox to leave annotations for TODO comments, however I don't want them to cause a CI failure.
Otherwise I find that TODO comments are easily forgotten but this gives them visibility, without enforcing anything too strict.

A good alternative might however be to default to non-zero exit codes for everything and allow users to explicitly set it i.e.

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