Enable ErrCheck

[ogaca-dd]

What does this PR do?

This PR enables ErrCheck.
errcheck is a program for checking for unchecked errors in go programs.

Additional Notes

As handling errors may lead to behavior changes, I disabled existing warnings with //nolint:errcheck.

New codes should explicitly handle the errors. In the rare cases you want to discard the error use something like _ := MyFunction().

[olivielpeau]

Adding this linter makes sense to me, and the approach of ignoring the existing non-check errors in this initial PR sounds good as well.

How much does this linter add to the linter wall clock time on a dev env ? Does it support linting only a subset of the packages? (wouldn't want to slow down invoke test by too much, especially when invoke test --targets=./pkg/foo is specified for example)

[prognant]

I don't remember if we have subsequent action planned to handle all the places requiring the //nolint:errcheck assertion, but I guess we will see later.

[prognant]

Given the huge number of http.ResponseWriter.write(...) requiring the //nolint:errcheck would it be possible to exclude it ? (possible errors seem relatively safe to ignore https://golang.org/pkg/net/http/#pkg-variables but I may be wrong about that)

[ogaca-dd]

I tried to ignore http.ResponseWriter.Write a couple of days ago but because of golangci/golangci-lint#656, it seems I can only ignore all the Write methods from http package (and not the specific method http.ResponseWriter.Write).
Upgrading golangci-lint requires extra work.

[hush-hush]

I agree with @prognant, I'm not a fan of all the //nolint:errcheck. We might want to ignore all Write() call from the http package at this point.

As a more general review, we ignore so many warning in this PR that I wonder if it's worth it. I feel like it's increasing the code complexity and we're not gaining a lot. Like all call to json or yaml should actually be fixed instead of being ignore. WDYT ?

I'm afraid people are not actually going to check their error but just pollute the code base with //nolint:errcheck. At this point we will fall back to where we are now: relying on review instead of the linter. That's why I didn't enabled it in the first PR for golangci: way to many warning we might not want to fix.

[olivielpeau]

my 2 cents:

• I guess it makes sense for the linter to ignore all Write calls from the http package. Let's add a fixme comment linking to the golanci issue, we might be able to make the ignore rule more specific when we upgrade golangci
• I prefer explicitly calling out all instances of unhandled errors with //nolint:errcheck than having errors "silently" ignored in the code (by "silent" here, I mean that the calling code doesn't even acknowledge that the call can return an error)
• merging this with a bunch of //nolint:errcheck is simpler to review and merge, and already provides value for future code we'll add to the codebase. But yes please create tasks in the backlog about cleaning up all these //nolint:errcheck (one card per team owning related packages)

[ogaca-dd]

@olivielpeau

How much does this linter add to the linter wall clock time on a dev env ?

Measurements based of the median of 3 runs for time invoke test --targets=./pkg/aggregator
before:

real    0m24.182s
user    0m55.739s
sys     0m18.056s

after:

real    0m24.293s
user    0m55.490s
sys     0m18.228s

Does it support linting only a subset of the packages? (wouldn't want to slow down invoke test by too much, especially when invoke test --targets=./pkg/foo is specified for example

I only enabled a new linter and so the behavior is the same as before.

[hush-hush]

I agree with @prognant, I'm not a fan of all the //nolint:errcheck. We might want to ignore all Write() call from the http package at this point.

As a more general review, we ignore so many warning in this PR that I wonder if it's worth it. I feel like it's increasing the code complexity and we're not gaining a lot. Like all call to json or yaml should actually be fixed instead of being ignore. WDYT ?

I'm afraid people are not actually going to check their error but just pollute the code base with //nolint:errcheck. At this point we will fall back to where we are now: relying on review instead of the linter. That's why I didn't enabled it in the first PR for golangci: way to many warning we might not want to fix.

[ogaca-dd]

@hush-hush :

I agree with @prognant, I'm not a fan of all the //nolint:errcheck. We might want to ignore all Write() call from the http package at this point.

Write methods in net/http are now ignored by ErrCheck.

As a more general review, we ignore so many warning in this PR that I wonder if it's worth it. I feel like it's increasing the code complexity and we're not gaining a lot. Like all call to json or yaml should actually be fixed instead of being ignore. WDYT ?

At first, I wanted to fix everything but I realized that I do not well enough the impacted code to do the changes.

I'm afraid people are not actually going to check their error but just pollute the code base with //nolint:errcheck. At this point we will fall back to where we are now: relying on review instead of the linter. That's why I didn't enabled it in the first PR for golangci: way to many warning we might not want to fix.

I think ErrCheck linter is one the most useful linters of golangci-lint as not checking an error can lead to a bug.

At this point we will fall back to where we are now: relying on review instead of the linter.
Without Errcheck it is very hard during a review to catch error not checked so I think it will make a big difference compared to the current situation.

I'm afraid people are not actually going to check their error but just pollute the code base with //nolint:errcheck

There are some rare cases where ignoring error is the right thing. _ can be used and is shorter and easier to remember than //nolint:errcheck.
Indeed, new //nolint:errcheck might be added. If we really want to avoid it, we can add a check that count the number of //nolint:errcheck. What do you think?