ci: remove highest firing golanglint-ci

Created by: keegancsmith

This is an attempt to improve the signal of golanglint-ci. It removes the highest firing lints in our codebase. We should probably remove more and stick to just lints that find bugs (like staticcheck). This is a first step.

Ran the following shell command and removed all checks with 50 or more complaints.

golangci-lint run | egrep -o '\([a-z]*\)$' | sort | uniq -c | sort -n

   1 (c)
   1 (ctx)
   1 (govet)
   1 (ineffassign)
   2 (ch)
   2 (compressed)
   2 (contents)
   2 (data)
   2 (errcheck)
   2 (expected)
   2 (file)
   2 (recompressed)
   2 (req)
   2 (staticcheck)
   2 (stored)
   2 (unused)
   3 (bodyclose)
   3 (goimports)
   3 (nakedret)
   3 (rowserrcheck)
   3 (unconvert)
   4 (dogsled)
   4 (nil)
   6 (whitespace)
   7 (deadcode)
   9 (stylecheck)
  12 (interfacer)
  20 (t)
  22 (err)
  29 (gosec)
  49 (misspell)
  50 (dupl)
  50 (gocritic)
  50 (golint)
  50 (unparam)
  56 ()

Created by: codecov[bot]

Codecov Report

Merging #11951 into master will decrease coverage by 2.23%. The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #11951      +/-   ##
==========================================
- Coverage   49.98%   47.75%   -2.24%     
==========================================
  Files        1515     1410     -105     
  Lines       88340    80080    -8260     
  Branches     6808     6807       -1     
==========================================
- Hits        44161    38246    -5915     
+ Misses      40251    38255    -1996     
+ Partials     3928     3579     -349     

Flag	Coverage Δ
#go	51.87% <ø> (-2.46%)
#storybook	10.85% <ø> (ø)
#typescript	36.76% <ø> (ø)
#unit	47.33% <ø> (-2.27%)

Impacted Files	Coverage Δ
cmd/frontend/db/orgs_mock.go	0.00% <0.00%> (-100.00%)
cmd/frontend/db/users_mock.go	0.00% <0.00%> (-100.00%)
internal/extsvc/github/codehost.go	0.00% <0.00%> (-100.00%)
internal/extsvc/gitlab/codehost.go	0.00% <0.00%> (-100.00%)
...rker/internal/correlation/lsif/jsonlines/reader.go	0.00% <0.00%> (-100.00%)
internal/authz/bitbucketserver/authz.go	0.00% <0.00%> (-89.48%)
internal/metrics/operation.go	0.00% <0.00%> (-87.70%)
internal/authz/gitlab/gitlab.go	0.00% <0.00%> (-86.52%)
...e-code-intel-bundle-manager/internal/paths/util.go	0.00% <0.00%> (-83.34%)
internal/vcs/util/fileinfo.go	0.00% <0.00%> (-81.82%)
... and 256 more

Created by: tsenart

While I personally find go-critic checks valuable, I'm deferring to the team on this one.

Created by: keegancsmith

I think the issue is they are so easy to ignore and we have so much noise that we are trained to ignore everything from golanglint-ci. Really we should do what we had before, high signal checks that fail the build.

If we want to integrate these other checks, we should first make the codebase clean so it will only blame on new code.

Created by: tsenart

If we want to integrate these other checks, we should first make the codebase clean so it will only blame on new code.

It only brings up warnings on files you touched. I did that intentionally thinking that granularity would make it relatively easy to leave the ground cleaner than you found it, but it seems that isn't the case. Why is that?

In any case, it can be configured to only display warnings for text ranges you actually touched, rather than the whole set of files. How would you feel about that? I am not sure cleaning the whole codebase at once for each category of lints / checks is always feasible, so an incremental approach that we commit to is worth discussing.

Created by: keegancsmith

I agree, lets do just the ranges you touched then. You will likely have a better outcome. Can you do a PR for that?

Created by: tsenart

Can you do a PR for that?

Let's keep this discussion centralized in this PR for easy contribution / catch up by others.

The functionality I was referring to is implemented by Review Dog which is the tool we use to comment on PRs, with actual input by golangci-lint. It is documented here: https://github.com/reviewdog/reviewdog#filter-mode

• added (default): Filter results by added/modified lines.
• diff_context: Filter results by diff context. i.e. changed lines +-N lines (N=3 for example).
• file: Filter results by added/modified file. i.e. reviewdog will report results as long as they are in added/modified file even if the results are not in actual diff.
• nofilter: Do not filter any results. Useful for posting results as comments as much as possible and check other results in console at the same time.

I think we want either added or diff_context. You can change that in this PR here: https://github.com/sourcegraph/sourcegraph/blob/bf8e187046afdce8324693296b80de9172ca73be/.github/workflows/reviewdog.yml#L14

Created by: tsenart

Stale.