Run golangci-lint with extra tags, fix found issues #2815
Conversation
There was a problem hiding this comment.
Thanks!
The cleanups are nice.
The extra 4.5 minutes of CI time, on a validate task which is a serial prerequisite to running almost all other tests, are rather unpleasant, though. I’d prefer that to, at least, happen in a separate Cirrus task that runs concurrently with the other slow tasks — or maybe it’s just not worth the extra CI cost to run these all the time.
kolyshkin
force-pushed
the
extra-lint-tags
branch
2 times, most recently
from
9ded524 to
47ba1d9
Compare
@mtrmac It's about 40-50 extra seconds (from 4:00 minutes to 4:50 minutes), but I can split it out to a separate task if you like (maybe also adding Having looked at it more, I think the best thing to do is to modify skopeo test (which is the longest step and takes ~15 minutes) to not wait on validate. This alone will actually make tests finish 4-5 minutes faster; here's a PR: #2830 |
|
One other thing we can do to make linting faster is something like containers/podman#25772. Let me give it a try. |
PR #2832 |
As reported by golangci-lint run --build-tags=containers_image_openpgp:
> signature/mechanism_openpgp.go:150:53: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
> return nil, "", fmt.Errorf("signature error: %v", md.SignatureError)
> ^
Signed-off-by: Kir Kolyshkin <[email protected]>
These two files test functionality disabled when containers_image_storage_stub build tag is set. Fix accordingly Signed-off-by: Kir Kolyshkin <[email protected]>
Move parseLeafCertFromPEM from fulcio_cert.go to pki_cert.go (and move its unit test accordingly). This way, everything compiles when containers_image_fulcio_stub build tag is set. Similarly, move assertPublicKeyMatchesCert from fulcio_cert_test.go to pki_cert_test.go to fix tests compilation. Signed-off-by: Kir Kolyshkin <[email protected]>
Otherwise, the "unused" linter complains about unused leveledLogger and all its methods. Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
Signed-off-by: Kir Kolyshkin <[email protected]>
kolyshkin
force-pushed
the
extra-lint-tags
branch
from
47ba1d9 to
69abdbc
Compare
|
Thanks! I think something like #2832 is clearly desirable. And that might make the extra time so small to not be worth worrying about. (IIRC it was extra 4.5 minutes originally when testing each tag separately in a loop.) Although, in principle, it’s better to have a small, fast, cheap first test to rule out obviously non-viable PRs, and splitting seems conceptually closer to the ideal, at some point it’s just not worth the complexity (and overhead costs for scheduling+starting+separate caching). |
The idea is to run golangci-lint with flags that are not usually set, to catch some potential issue that are not caught otherwise. Surprisingly, it's not much (see the first 4 commits).