Add golangci-lint #2602
Add golangci-lint #2602
Conversation
|
I would prefer if there were a make target that would allow running all the lint checks locally, not only in GH actions. |
If we have a config in the repo like the That said, I'd rather have this PR use the official action instead of what we have here. Reference: https://github.com/observatorium/opentelemetry-collector-builder/blob/c725aab7de2c3f782f8273ae4cc96faf21747290/.github/workflows/go.yaml#L27-L39 |
Codecov Report
@@ Coverage Diff @@
## master #2602 +/- ##
==========================================
+ Coverage 95.55% 95.57% +0.01%
==========================================
Files 214 215 +1
Lines 9546 9579 +33
==========================================
+ Hits 9122 9155 +33
- Misses 345 346 +1
+ Partials 79 78 -1
Continue to review full report at Codecov.
|
|
@yurishkuro thanks for the review! Do you think this should be a separate |
|
for now let's keep it in ci-unit-tests.yml Can you actually enable any linters? The job is not checking anything right now. |
How did you assess that? The configuration looks sane to me and I would guess that the following linters were used, but I wonder if you are looking at some place with a positive confirmation about the linters that were used for the specific run. |
|
configuration says |
|
It does say
https://github.com/jaegertracing/jaeger/pull/2602/checks?check_run_id=1478020447 |
|
$ make lint-gosec
time gosec -quiet -exclude=G104,G107 -exclude-dir=cmd/opentelemetry ./...
120.03 real 236.28 user 195.93 sys |
|
With the config file, you can use OutputI am specifying
This is a fairly common style for
This seems like a great reason to use golangci-lint, the go sec errors I am seeing look the same, and golangci-lint runs much faster for me ( |
I think this is the key :-) |
|
I am not sure what only-new-issues means: you still have to scan everything to discover IF there are new issues. Why can't we run with -v in CI as well? That would at least provide some proof that the linters are running. |
.github/workflows/ci-unit-tests.yml
Outdated
| uses: golangci/golangci-lint-action@v2 | ||
| with: | ||
| version: v1.33 | ||
| only-new-issues: true |
There was a problem hiding this comment.
Could you please explain what only new issues means? Does it mean it only scans changed files (if so how does it determine that, against master?), or that it reports errors previously unseen (if so where does it store the state?) ?
There was a problem hiding this comment.
This has been dropped since all linters are currently passing.
Per https://golangci-lint.run/usage/configuration/
# Show only new issues: if there are unstaged changes or untracked files,
# only those changes are analyzed, else only changes in HEAD~ are analyzed.
# It's a super-useful option for integration of golangci-lint into existing
# large codebase. It's not practical to fix all existing issues at the moment
# of integration: much better don't allow issues in new code.
# Default is false.
This is meant to allow easier integration into a large codebase by only reporting errors if the added code contains them. It does not report previously seen linter errors (those that are already in master).
This has been dropped though because all outstanding linter issues have been fixed.
|
@jpkrohling my main concern with this is that by adding a whole set of new linters with I think I would prefer instead to add linters one by one or in batches along with the corresponding fixes of ALL issues that a given linter flags. That way each PR could be reasonably sized and not push the burden of the actual fixes on the next random person who happens to touch the files. For example, with the config in this PR: |
Good point and I agree with your proposal. |
|
I'm happy to either reduce the number of enabled linters or fix the found issues. |
|
I'd recommend enabling only those linters that currently pass on the whole repository. |
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
crossdock/main.go:112:23: response body must be closed (bodyclose) Close the response body when the err == nil to avoid leaking resources Signed-off-by: Dax McDonald <[email protected]>
plugin/storage/badger/spanstore/read_write_test.go:142:4: var `lowId` should be `lowID` (golint)
lowId := rand.Uint64()
Signed-off-by: Dax McDonald <[email protected]>
File is not query/app/ui/placeholder.go:20: File is not `goimports`-ed Signed-off-by: Dax McDonald <[email protected]>
Elements yieled from a range statement have the same memory address See https://github.com/golang/go/wiki/CommonMistakes#using-reference-to-loop-iterator-variable Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
Signed-off-by: Dax McDonald <[email protected]>
|
After removing a few linters, there were still some small issues remaining. These all looked like legitimate issues, so I proceeded to fix them Linter errorsINFO [runner] linters took 142.248311ms with stages: goanalysis_metalinter: 112.589465ms, unused: 7.165119ms Happy to revert any commits as needed |
|
The run in the CI succeeds, but when I run it locally with the current config file I get a few errors. Where is the discrepancy coming from? $ golangci-lint -v --max-issues-per-linter 500 --max-same-issues 500 run |
|
Hmm, are you running on this branch? That exact command passes for me with
golangci-lint version `1.33.0`
…On Tue, Dec 8, 2020 at 1:09 PM Yuri Shkuro ***@***.***> wrote:
The run in the CI succeeds, but when I run it locally with the current
config file I get a few errors. Where is the discrepancy coming from?
$ golangci-lint -v --max-issues-per-linter 500 --max-same-issues 500 run
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2602 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AHS5HJRALG47GHWFAKNDR3LST2BXRANCNFSM4TDHO26Q>
.
|
| for _, span := range spans { | ||
| span := span | ||
| writer.WriteSpan(&span) |
There was a problem hiding this comment.
this is weird. I think I know what the linter is complaining about, and a better solution is
| for _, span := range spans { | |
| span := span | |
| writer.WriteSpan(&span) | |
| for i := range spans { | |
| writer.WriteSpan(&spans[i]) |
There was a problem hiding this comment.
I am not familiar with the intention of the code but
for _, span := range spans {
// something with &span
looks like the developer wants to use copies of the original item, not explicitly manipulating the items in the array.
If the intention is to use the items directly then I believe this change makes sense.
There was a problem hiding this comment.
writer treats the spans as immutable, so the copying is not necessary, only extra memory allocation
| @@ -111,6 +111,7 @@ func httpHealthCheck(logger *zap.Logger, service, healthURL string) { | |||
| for i := 0; i < 240; i++ { | |||
| res, err := http.Get(healthURL) | |||
| if err == nil && is2xxStatusCode(res.StatusCode) { | |||
| res.Body.Close() | |||
There was a problem hiding this comment.
fine here because it's a test code, but strictly speaking we must close when err==nil regardless of status code
There was a problem hiding this comment.
Yeah, I wanted to minimize the diff here
| - govet | ||
| - gosec | ||
| - gofmt | ||
| - golint |
There was a problem hiding this comment.
I find it very suspicious that you're not getting errors from golint given the current exclusions below. Usually thrift-gen produces a lot of errors for golint, which is why we excluded it in the makefile
|
Also, what makes it exclude examples? I think examples code is a fair game for linter (unlike generated code). |
Co-authored-by: Yuri Shkuro <[email protected]>
|
Closing due to inactivity. |
Which problem is this PR solving?
Short description of the changes
Signed-off-by: Dax McDonald [email protected]
Happy Hacktoberfest 🎃