Switch to golangci-lint v2, cleanup config, fix new linter warnings #2389

kolyshkin · 2025-03-26T00:40:05Z

This can be logically split into 3 parts:

Configuration cleanup (first 2 commits)

.golangci.yml: switch to list of enabled linters

Instead of enabling all linters, and when disabling some of them because
we don't like them, switch to list of explicitly enabled linters. The
upside of this is we can easily upgrade golangci-lint (i.e. any new
linters won't break our CI). The downside is, we need to explicitly
enable extra linters.

To me, this is better from the maintainability perspective.

NOTE this commit does not change the configuration in any way, in other
words, the list of linters being run is the same as before. The next
commit will address this.

PS this is my second attempt of doing this. The first one was in
commit 3bb7e63 ("Bump golangci-lint to 1.45.2", PR golangci-lint spring cleaning and bump #997),
which was reverted in b6cd136 ("golangci.yml: enable all,
disable failing linters"). I still think that enable-all is not sane.
.golangci.yml: remove some linters

This carefully remove linters that makes no sense but were previuously
enabled because of enable-all logic.

Fix or silence some of the new warnings (all the intermediary commits)

pkg/retry: rm unused break
pkg: do not capitalize error strings
libimage: silence a staticcheck warning
pkg/cgroups: apply De Morgan's law
libimage: apply De Morgan's law
pkg: apply De Morgan's law
pkg/timetype: fix linter warning, improve comment
libnetwork/netavark: simplify isMacVlan init
libnetwork/types: rename RegexError to ErrInvalidName
pkg/timezone: simplify switch
libnetwork: simplify write
pkg/cgroups: simplify write
pkg/secrets: use time.Equal

Switch to golangci-lint v2.0 (the last commit)

Questions / discussion

Whether "apply De Morgan's law" check should be enabled?

Personally, I am not convinced it always makes the code more readable. If others agree, I will remove the corresponding commits and instead just disable staticcheck's QF1001;
Can pkg: do not capitalize error strings result in any regressions or test failures?

Probably. If that's the case, I think we should introduce more typed errors.

The alternative, as in the previous case, is to disable staticcheck's ST1005.
Should the .z version of golangci-lint be specified?

Pros of having .z: more stability (no random CI breaks caused by unintended upgrades).
Cons: more upgrade "noisy" PRs.

Luap99 · 2025-03-26T12:50:39Z

.github/workflows/validate.yml


 env:
-  LINT_VERSION: v1.64.8
+  LINT_VERSION: v2.0


shouldn't we pin x.y.z here. Otherwise the linter might can get a ptach update that randomly breaks on a PR, no?

My thinking was by omitting the patch version we have less noise PRs to upgrade, and from my experience with other repos I don't remember that patch bump results in any new warnings.

Of course YMMV, so let me know which way you like it.

personally I hate breakages on random PRs, in case of community contributors they might get confused and you then get to tell them to rebase, etc...

That said I agree if you have experience that it is rather stable I am fine to give that a go, my (bad) experiences about linter updates are from the fact that we had enable-all which of course causes new warnings very often but you already fixed that problem here.

I agree that enable-all is a PITA, and this is actually my second attempt at killing it (🤞🏻 it will stay this time).

Instead of enabling all linters, and when disabling some of them because we don't like them, switch to list of explicitly enabled linters. The upside of this is we can easily upgrade golangci-lint (i.e. any new linters won't break our CI). The downside is, we need to explicitly enable extra linters. To me, this is better from the maintainability perspective. NOTE this commit does not change the configuration in any way, in other words, the list of linters being run is the same as before. The next commit will address this. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Let's take a look at each enabled linter, and find out if it's needed; remove those that make no sense. * gci: formats imports, probably superseded by gofumpt; * gofmt: is a subset of gofumpt; * goheader: does nothing with empty configuration; * goimports: functionality should be covered by gofumpt; * gomodguard: does nothing with empty configuration; * grouper: formats imports, probably superseded by gofumpt; * importas: does nothing with empty configuration; * loggercheck: this repo does not use any loggers it checks (kitlog,klog,logr,zap); * promlinter: this repo does not use Prometheus; * protogetter: this repo does not use protobuf; * rowserrcheck: this repo does not use sql; * sloginit: this repo does not use slog; * spancheck: this repo does not use opentelemetry/opencensus; * sqlclosecheck: this repo does not use sql; * tagalign: this repo does not use multiple struct tags; * testableexamples: this repo does not have any examples; * zerologlint: this repo does not use zerolog. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Fix the following warning, as reported by staticcheck from golangci-lint v2.0.1: > pkg/secrets/secrets_test.go:67:5: QF1009: probably want to use time.Time.Equal instead (staticcheck) > if s.CreatedAt == s.UpdatedAt { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Fix the following staticcheck warning: > pkg/cgroups/utils_linux.go:264:18: QF1012: Use fmt.Fprintf(...) instead of WriteString(fmt.Sprintf(...)) (staticcheck) > if _, err := f.WriteString(fmt.Sprintf("%d\n", pid)); err != nil { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Fix the following staticcheck warning: > libnetwork/slirp4netns/slirp4netns.go:685:15: QF1012: Use fmt.Fprintf(...) instead of Write([]byte(fmt.Sprintf(...))) (staticcheck) > if _, err := conn.Write([]byte(fmt.Sprintf("%s\n", data))); err != nil { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following staticcheck warning: > pkg/timezone/timezone.go:23:2: QF1002: could use tagged switch on timezone (staticcheck) > switch { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

... and add a deprecated alias so backward compatibility is still preserved (and users can gradually switch to the new name). Done because this is now also reported by staticcheck (in addition to revive) linter. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following staticcheck warning: > libnetwork/netavark/config.go:297:2: QF1007: could merge conditional assignment into variable declaration (staticcheck) > isMacVlan := true > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Fix the following staticcheck warning: > pkg/timetype/timestamp.go:35:21: QF1001: could apply De Morgan's law (staticcheck) > parseInLocation := !(strings.ContainsAny(value, "zZ+") || strings.Count(value, "-") == 3) > ^ While at it, improve the comment. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following staticcheck warnings: > pkg/configmaps/configmaps.go:135:5: QF1001: could apply De Morgan's law (staticcheck) > if !(len(data) > 0 && len(data) < maxConfigMapSize) { > ^ > pkg/secrets/secrets.go:169:5: QF1001: could apply De Morgan's law (staticcheck) > if !(len(data) > 0 && len(data) < maxSecretSize) { > ^ as well as the subsequent gocritic warnings: > pkg/configmaps/configmaps.go:135:5: sloppyLen: len(data) <= 0 can be len(data) == 0 (gocritic) > if len(data) <= 0 || len(data) >= maxConfigMapSize { > ^ > pkg/secrets/secrets.go:169:5: sloppyLen: len(data) <= 0 can be len(data) == 0 (gocritic) > if len(data) <= 0 || len(data) >= maxSecretSize { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following staticcheck warnings: > libimage/image.go:463:5: QF1001: could apply De Morgan's law (staticcheck) > if !(referencedBy == "" || numNames == 1) { > ^ > libimage/normalize.go:33:5: QF1001: could apply De Morgan's law (staticcheck) > if !(strings.ContainsAny(registry, ".:") || registry == "localhost") { > ^ > libimage/search.go:220:6: QF1001: could apply De Morgan's law (staticcheck) > if !(filterMatchesAutomatedFilter(&options.Filter, results[i]) && filterMatchesOfficialFilter(&options.Filter, results[i]) && filterMatchesStarFilter(&options.Filter, results[i])) { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following staticcheck warning: > pkg/cgroups/utils_linux.go:224:25: QF1001: could apply De Morgan's law (staticcheck) > if parts[2] == "/" && !(unifiedMode && parts[1] == "") { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following linter warnings: > pkg/auth/auth.go:176:11: ST1005: error strings should not be capitalized (staticcheck) > return errors.New("Can't specify both --password-stdin and --password") > ^ > pkg/auth/auth.go:179:11: ST1005: error strings should not be capitalized (staticcheck) > return errors.New("Must provide --username with --password-stdin") > ^ > pkg/subscriptions/subscriptions.go:325:17: ST1005: error strings should not be capitalized (staticcheck) > return false, fmt.Errorf("Container /etc resolution error: %w", err) > ^ > pkg/subscriptions/subscriptions.go:325:17: ST1005: error strings should not be capitalized (staticcheck) > return false, fmt.Errorf("Container /etc resolution error: %w", err) > ^ > pkg/subscriptions/subscriptions.go:334:17: ST1005: error strings should not be capitalized (staticcheck) > return false, fmt.Errorf("Container /etc/system-fips resolution error: %w", err) > ^ > pkg/subscriptions/subscriptions.go:451:10: ST1005: error strings should not be capitalized (staticcheck) > return fmt.Errorf("Could not expand %q in container: %w", srcPolicyConfig, err) > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This fixes the following linter warning: > pkg/retry/retry.go:50:4: SA4011: ineffective break statement. Did you mean to break out of the outer loop? (staticcheck) > break > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

The new configuration files were initially generated by `golangci-lint migrate`, when tweaked to minimize and simplify. golangci-lint v2 switches to a new version of staticcheck which shows much more warnings. Some of them were fixed by a few previous commits, and the rest of them are disabled. Also, golangci-extra was modified to include ALL staticcheck linters. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Luap99

LGTM

openshift-ci · 2025-03-26T16:43:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kolyshkin, Luap99

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Luap99,kolyshkin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mheon · 2025-03-26T18:04:42Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress approved labels Mar 26, 2025

kolyshkin changed the title ~~Switch to golangci-lint v2~~ Switch to golangci-lint v2, cleanup config, fix some new linter warnings Mar 26, 2025

kolyshkin changed the title ~~Switch to golangci-lint v2, cleanup config, fix some new linter warnings~~ Switch to golangci-lint v2, cleanup config, fix new linter warnings Mar 26, 2025

kolyshkin marked this pull request as ready for review March 26, 2025 00:53

openshift-ci bot removed the do-not-merge/work-in-progress label Mar 26, 2025

Luap99 reviewed Mar 26, 2025

View reviewed changes

kolyshkin added 16 commits March 26, 2025 09:36

pkg/timezone: simplify switch

a51b249

This fixes the following staticcheck warning: > pkg/timezone/timezone.go:23:2: QF1002: could use tagged switch on timezone (staticcheck) > switch { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

pkg/cgroups: apply De Morgan's law

a92abc9

This fixes the following staticcheck warning: > pkg/cgroups/utils_linux.go:224:25: QF1001: could apply De Morgan's law (staticcheck) > if parts[2] == "/" && !(unifiedMode && parts[1] == "") { > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

libimage: silence a staticcheck warning

c57c570

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

pkg/retry: rm unused break

9cbb5cd

This fixes the following linter warning: > pkg/retry/retry.go:50:4: SA4011: ineffective break statement. Did you mean to break out of the outer loop? (staticcheck) > break > ^ Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the golangci-v2 branch from 8d9766c to ebbd38f Compare March 26, 2025 16:37

Luap99 approved these changes Mar 26, 2025

View reviewed changes

openshift-ci bot assigned mheon Mar 26, 2025

openshift-ci bot added the lgtm label Mar 26, 2025

openshift-merge-bot bot merged commit 954a8ce into containers:main Mar 26, 2025
15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switch to golangci-lint v2, cleanup config, fix new linter warnings #2389

Switch to golangci-lint v2, cleanup config, fix new linter warnings #2389

Uh oh!

kolyshkin commented Mar 26, 2025 •

edited

Loading

Uh oh!

Luap99 Mar 26, 2025

Uh oh!

kolyshkin Mar 26, 2025

Uh oh!

Luap99 Mar 26, 2025

Uh oh!

kolyshkin Mar 26, 2025

Uh oh!

Luap99 left a comment

Uh oh!

openshift-ci bot commented Mar 26, 2025

Uh oh!

mheon commented Mar 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Switch to golangci-lint v2, cleanup config, fix new linter warnings #2389

Switch to golangci-lint v2, cleanup config, fix new linter warnings #2389

Uh oh!

Conversation

kolyshkin commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Questions / discussion

Uh oh!

Luap99 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

kolyshkin Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

kolyshkin Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Luap99 left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Mar 26, 2025

Uh oh!

mheon commented Mar 26, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kolyshkin commented Mar 26, 2025 •

edited

Loading