build: upgrade golangci-lint to v2.13.1 and fix resulting lint issues - #122
Open
bhatnitish wants to merge 1 commit into
Open
build: upgrade golangci-lint to v2.13.1 and fix resulting lint issues#122bhatnitish wants to merge 1 commit into
bhatnitish wants to merge 1 commit into
Conversation
`make lint` does not currently work in this repo. It fails before reporting
anything:
can't load config: the Go language version (go1.23) used to build
golangci-lint is lower than the targeted Go version (1.25.8)
[email protected] has a toolchain directive forcing it to be built with
go1.23.6, while go.mod targets Go 1.25.8, and golangci-lint refuses that
combination. Staying on v1 is not an option: every v1 release up to and
including the final v1.64.8 is built with Go <= 1.24.
Bump to v2.13.1 (built with go1.26.7). Note the module path gains a /v2 suffix.
There is no .golangci.yml in this repo, so there is no config to migrate - only
the default linter set changes slightly (gosimple folds into staticcheck).
Fix the 34 issues this surfaces. Note that golangci-lint caps duplicate
messages at 3 by default, which reports only 21 of them; the full set was found
with --max-same-issues=0.
errcheck 7 unchecked Close() / os.Setenv / os.Unsetenv returns
staticcheck 13 ST1005 capitalized error strings
staticcheck 5 QF1004 strings.Replace(..., -1) -> strings.ReplaceAll
staticcheck 4 QF1008 redundant embedded-field selectors
staticcheck 2 QF1003 if/else-if chain -> tagged switch
staticcheck 2 SA1019 deprecated NodeSystemInfo.KubeProxyVersion
staticcheck 1 ST1001 dot import
Notes on the two judgement calls:
- SA1019: KubeProxyVersion appears only as test fixture data and is never read
by non-test code, so the field is dropped rather than suppressed.
- ST1001: the dot import is gocheck's documented idiom and there are 66 call
sites across the e2e suite that would need qualifying. Suppressed with a
nolint carrying that rationale rather than churning the suite.
All changes are behaviour preserving. Verified: make manager, make vet, make
unit-test and make lint all pass, and lint reports 0 issues uncapped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
make lintdoes not currently work in this repo. It fails before reporting anything:[email protected]has atoolchaindirective forcing it to build with go1.23.6, whilego.modtargets Go 1.25.8 — golangci-lint refuses that combination outright, so lint has effectively been unenforced.Staying on v1 isn't an option: every v1 release, up to and including the final v1.64.8, is built with Go ≤ 1.24 and rejects this module.
This is a prerequisite for adding a
Lintjob to CI (follow-up PR).What
Toolchain bump.
v1.63.4→v2.13.1(built with go1.26.7); module path gains a/v2suffix. There is no.golangci.ymlhere, so there's no config to migrate — the only behavioural change is that the default linter set foldsgosimpleintostaticcheck.34 issues fixed. Worth flagging: golangci-lint caps duplicate messages at 3 by default, which reports only 21. The true count came from
--max-same-issues=0— fixing just the visible 21 would have made the remaining 13 surface immediately.Close()/os.Setenv/os.Unsetenvreturnsstrings.Replace(…, -1)→strings.ReplaceAllif/else ifchain → taggedswitchNodeSystemInfo.KubeProxyVersionTwo judgement calls
SA1019 —
KubeProxyVersion. I checked whether any non-test code reads it: nothing does, it's purely test fixture data. So the field is dropped rather than suppressed.ST1001 — dot import in
tests/e2e/suite.go.. "gopkg.in/check.v1"is gocheck's documented idiom, and there are 66*Ccall sites across 5 files that would need qualifying. Churning the whole e2e suite for a style rule isn't a good trade, so this one is suppressed with a//nolintcarrying that rationale.The QF1008 fixes rely on field promotion, so I checked for shadowing:
ctrl.Requestembedstypes.NamespacedNameand defines noString()of its own, andrest.ConfigembedsContentConfig. Neither promotion recurses.Verification
make managermake vetmake unit-testmake lint--max-same-issues=0 --max-issues-per-linter=0)All changes are behaviour-preserving.
Note
Installing golangci-lint v2.13.1 requires Go ≥ 1.26, so the
gocommand switches toolchains automatically. That will fail anywhereGOTOOLCHAIN=localis pinned.