golangci-lint v2 #131477

pohly · 2025-04-25T12:53:25Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This updates Kubernetes to use the new and revised golangci-lint v2. On this occasion, the check suppressions got revisited, revealing that in some cases existing code really is broken (fix included in this PR, see individual commits).

Special notes for your reviewer:

This is a continuation of #131113. See last commit for the differences.

Does this PR introduce a user-facing change?

NONE

/cc @mmorel-35 @ldez

k8s-ci-robot · 2025-04-25T12:53:28Z

@pohly: GitHub didn't allow me to request PR reviews from the following users: mmorel-35, ldez.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This updates Kubernetes to use the new and revised golangci-lint v2. On this occasion, the check suppressions got revisited, revealing that in some cases existing code really is broken (fix included in this PR, see individual commits).

Special notes for your reviewer:

This is a continuation of #131113. See last commit for the differences.

Does this PR introduce a user-facing change?
NONE
/cc @mmorel-35 @ldez

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

BenTheElder · 2025-04-29T20:19:32Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go

@@ -65,7 +65,7 @@ func Convert_v1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s
 		}
 		*out = i
 	} else {
-		out = nil
+		*out = nil


this is a really subtle one, we'll need to tag in API reviewers

/assign @liggitt

Without this:

ERROR: staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go:68:3: ineffectual assignment to out (ineffassign) ERROR: out = nil ERROR: ^

This change is correct, the linter was correct to flag the old code.

This change is also safe. out must be a non-nil pointer for conversion to work, and in all call sites, a non-nil out pointer is passed.

This change is also ultimately a no-op for existing callers because in all call sites, a nil in parameter is never passed.

This is either because the source is not a pointer, so is always non-nil:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go

Line 766 in c064ed8

if err := Convert_v1_JSON_To_apiextensions_JSON(&(*in)[i], &(*out)[i], s); err != nil {

Or the source is a pointer but the nil case is handled by directly assigning nil to the destination rather than calling Convert_v1_JSON_To_apiextensions_JSON:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go

Lines 742 to 750 in c064ed8

if in.Default != nil {

in, out := &in.Default, &out.Default

*out = new(apiextensions.JSON)

if err := Convert_v1_JSON_To_apiextensions_JSON(*in, *out, s); err != nil {

return err

}

} else {

out.Default = nil

}

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go

Lines 898 to 906 in c064ed8

if in.Example != nil {

in, out := &in.Example, &out.Example

*out = new(apiextensions.JSON)

if err := Convert_v1_JSON_To_apiextensions_JSON(*in, *out, s); err != nil {

return err

}

} else {

out.Example = nil

}

BenTheElder · 2025-04-29T20:31:47Z

hack/tools/go.mod

@@ -7,69 +7,77 @@ godebug default=go1.24
 require (
 	github.com/aojea/sloppy-netparser v0.0.0-20210819225411-1b3bd8b3b975
 	github.com/cespare/prettybench v0.0.0-20150116022406-03b8cfe5406c
-	github.com/golangci/golangci-lint v1.64.5
+	github.com/golangci/golangci-lint/v2 v2.1.5


starting to think about moving tools into seperate modules, so they behave closer to if you just go installed them and don't conflate dep versions, there are some tradeoffs (sharing deps improves build caching for them marginally), it's hard to tell if any of these impact other tools

if you want to do that, I recommend reading the section go tool of the golangci-lint doc https://golangci-lint.run/welcome/install/#install-from-sources

Right, currently we have this issue from the other tools:

When using the "tools pattern" or tool command/directives, the dependencies of a tool can modify the dependencies of another tool or your project. The resulting binary was not tested and is not guaranteed to work.

Tracking it in a shared tools module has risks, I think we should split it out.
Then the only question is if we use a specific module or avoid a module entirely.
Using a module means we get GOSHASUM and other visibility, at the risk of breaking replace statements.

We prefer not to take opaque binary dependencies generally, and it's also more complex to do portably across OS-es. It is somewhat less opaque to track this way, and I see shasums breaking as a feature that indicates some unknown change was made that hasn't been looked at. The GOSHASUMS enable TOFU in a way that is much simpler and re-usable than binaries.

I suggested that you read the section "go tool usage recommendations" inside the "install from sources" section.
You have to click on the triangle.

Ahh! it wasn't obvious that this expanded, and the lines above at the linked heading talk about go tool

Might be worth tweaking the styling / layout.

It is intentionally not a heading because we strongly don't recommend that.
But I prefer to provide the right way to do it instead of letting users in the dark.
So this is a compromise.

Installing golangci-lint through a separate module seems worthwhile. I've added one more commit doing that:

golangci-lint: move into hack/tools/golangci-lint The advantage is that it separates the dependencies of the different tools better. golangci-lint in particular has many dependencies and is sometimes sensitive to the exact version being used. This way, "go get" bumps up dependencies exactly as defined by the upstream golangci-lint module. It's not quite self-contained because logcheck as a Go plugin for golangci-lint must be built from the same dependencies. But it only adds one and does not change any of the others. While at it, the Go 1.24 "tools" directive gets used instead of the traditional tools.go approach.

BenTheElder

/approve
for hack/
Need more eyes on the other changes to confirm.

liggitt · 2025-04-30T14:05:53Z

pkg/controller/disruption/disruption.go

@@ -644,7 +646,7 @@ func (dc *DisruptionController) processNextWorkItem(ctx context.Context) bool {
 		return true
 	}

-	utilruntime.HandleError(fmt.Errorf("Error syncing PodDisruptionBudget %v, requeuing: %w", dKey, err)) //nolint:stylecheck
+	utilruntime.HandleErrorWithContext(ctx, err, "Error syncing PodDisruptionBudget", "key", dKey)


This is a curious change to pair with the lint v2 bump ... was the existing //nolint:stylecheck comment suppressing an "errors start with lowercase letter" check? If so, this changed to context logging (which is a functional change that looks unrelated to the linting), but is still passing the initial uppercase message the stylecheck complained about?

At the very least, we should avoid dropping the "requeuing" bit from the message, so "Error syncing PodDisruptionBudget, requeuing"

was the existing //nolint:stylecheck comment suppressing an "errors start with lowercase letter" check?

That check is not enforced. Perhaps //nolint was added in https://github.com/kubernetes/kubernetes/blame/03a3c0c89161935bc2338f5754ebb1104f779af1/pkg/controller/disruption/disruption.go#L647 to silence the hint. That was unnecessary and in this case, I would have preferred fixing the code.

We should have a linter which complains about //nolint:something without an additional comment after it (//nolint: // " Not relevant because ...)...

If so, this changed to context logging (which is a functional change that looks unrelated to the linting),

We can:

Leave this line as it is (including the //nolint:stylecheck, which is useless now because the linter is no longer active).

Remove just the //nolint:stylecheck.

Update the line to use current best practices (= use HandleErrorWithContext).

I prefer the last option.

but is still passing the initial uppercase message the stylecheck complained about?

HandleError was a logging function in disguise with additional tooling that normal error logging doesn't have. Using fmt.Errorf was (mis)used to produce a log message.

HandleErrorWithContext explicitly accepts a log message and additional key/value pairs, in addition to the error (if there is one - it's not required, i.e. no need to fabricate a new one if there isn't one already). For log messages, the recommendation is to start with a capital letter. I'm not exactly sure why, that recommendation pre-dates my involvement.

TLDR: `Error syncing" is fine here.

At the very least, we should avoid dropping the "requeuing" bit from the message, so "Error syncing PodDisruptionBudget, requeuing"

Ack, changed.

We should have a linter which complains about //nolint:something without an additional comment after it (//nolint: // " Not relevant because ...)...

You might want to take a look at whyNoLint rule from go-critic

or just nolintlint https://golangci-lint.run/usage/linters/#nolintlint

nolintlint is the official linter to handle nolint directives.

I suggested whyNoLint because go-critic is already enabled in this project and also nolintlint has been deactivated on some projects for generating conflicts with other linters see this open issue golangci/golangci-lint#3228

This problem is not a specific issue related to nolintlint and not related to comments, this is explained inside the issue.

liggitt · 2025-04-30T14:24:02Z

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/conversion.go

@@ -65,7 +65,7 @@ func Convert_v1_JSON_To_apiextensions_JSON(in *JSON, out *apiextensions.JSON, s
 		}
 		*out = i
 	} else {
-		out = nil
+		*out = nil


This change is correct, the linter was correct to flag the old code.

This change is also safe. out must be a non-nil pointer for conversion to work, and in all call sites, a non-nil out pointer is passed.

This change is also ultimately a no-op for existing callers because in all call sites, a nil in parameter is never passed.

This is either because the source is not a pointer, so is always non-nil:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go

Line 766 in c064ed8

if err := Convert_v1_JSON_To_apiextensions_JSON(&(*in)[i], &(*out)[i], s); err != nil {

Or the source is a pointer but the nil case is handled by directly assigning nil to the destination rather than calling Convert_v1_JSON_To_apiextensions_JSON:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go

Lines 742 to 750 in c064ed8

if in.Default != nil {

in, out := &in.Default, &out.Default

*out = new(apiextensions.JSON)

if err := Convert_v1_JSON_To_apiextensions_JSON(*in, *out, s); err != nil {

return err

}

} else {

out.Default = nil

}

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/zz_generated.conversion.go

Lines 898 to 906 in c064ed8

if in.Example != nil {

in, out := &in.Example, &out.Example

*out = new(apiextensions.JSON)

if err := Convert_v1_JSON_To_apiextensions_JSON(*in, *out, s); err != nil {

return err

}

} else {

out.Example = nil

}

staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1/conversion.go

liggitt · 2025-04-30T14:26:11Z

hack/verify-golangci-lint.sh

@@ -153,9 +155,8 @@ if [ "${golangci_config}" ]; then
  # don't to do it when not required.
  if grep -q 'path: ../_output/local/bin/' "${golangci_config}" &&


should the .. be dropped from the grep if we're dropping it from the sed here?

Yes, thanks for catching this.

liggitt · 2025-04-30T14:29:53Z

hack/verify-golangci-lint.sh

 echo "installing golangci-lint and logcheck plugin from hack/tools into ${GOBIN}"
-GOTOOLCHAIN="$(kube::golang::hack_tools_gotoolchain)" go -C "${KUBE_ROOT}/hack/tools" install github.com/golangci/golangci-lint/v2/cmd/golangci-lint
+GOWORK=off GOTOOLCHAIN="$(kube::golang::hack_tools_gotoolchain)" go -C "${KUBE_ROOT}/hack/tools/golangci-lint" install github.com/golangci/golangci-lint/v2/cmd/golangci-lint


we already have a dummy go.work checked in for hack/tools/go.work that meant we didn't need to do GOWORK=off before ... I'm not 100% sure why we needed to do that ... is there any other reason to add a peer hack/tools/golangci-lint/go.work? I think it would let us drop this disable.

That also works. Changed.

hack/tools/golangci-lint/go.mod

pkg/apis/autoscaling/v2beta2/conversion.go

liggitt · 2025-04-30T15:11:05Z

hack/golangci.yaml.in

+      checks:
+        - "all"
+        {{- if .Base }}
+        - "-QF1001"  # Apply De Morgan’s law


are all these exclusions for checks that are new in v2, or did we start running entire linters we weren't running before? I'm surprised we have to exclude so many individual new checks

To my understanding gosimple and stylecheck have been merged into staticcheck and staticcheck is the only one left available in golangci-lint.

liggitt · 2025-04-30T15:12:17Z

hack/golangci.yaml.in

+  # The default is relative to the configuration, which is confusing because
+  # then all paths start with ../ to move out of the "hack" directory.
+  # `gomod` mirrors the current behavior of `golangci-lint.sh` changing into
+  # the root.


... changing into the root

the root of k8s.io/kubernetes or of the module being evaluated (when we evaluate stuff under staging/src/k8s.io/*?

Changed:

# `gomod` mirrors the current behavior of `golangci-lint.sh` changing into # the root of the repository. Because we are operating in a workspace, # the module picked by `gomod` is the main one.

liggitt · 2025-04-30T15:14:10Z

hack/golangci.yaml.in

It's tricky to understand how stuff in the old config translated to the new one. Was the v2 config auto-generated with https://golangci-lint.run/product/migration-guide/ or done by hand?

Is there a way to see which changes were mechanical v1 → v2 translations and which were intentional changes from how we configured v1 to behave?

I think @mmorel-35 started by converting automatically because the initial commit in #131113 also included some re-ordering.

But some things are necessarily manual. I think we tried to stick as closely as possible to the original configuration, except for a few points where some cleanup was possible because something was obsolete (for example, a suppression because of a bug in the Ginkgo linter which had been fixed already long ago).

The important part is that the current code passes the base configuration. Not much had to be changed in the code to achieve that. If we find that the new configuration is too strong (whether its the base configuration or for hints), then we can always refine it in the future.

The re-ordering is not related to automatic conversion, because the order is not the order produced by the automatic conversion.

Not anymore, because at some point I asked @mmorel-35 to restore the original order.

@mmorel-35: did you initially convert automatically?

I started with golangci-lint migrate and then align with existing config to ease review (restore comments, quoting, spacing...)

haircommander · 2025-04-30T17:22:44Z

/cc

mmorel-35 · 2025-04-30T17:58:40Z

hack/golangci.yaml.in

+        - float-compare
+        - formatter
+        - len
+        - useless-assert


Those rules are new or updated to discover more cases in testifylint and needs to be disabled so the base is still passing

Signed-off-by: Matthieu MOREL <[email protected]>

Passing a constant value to gomega.Consistently means that it will not re-check while running. Found by linter after removing the suppression rule for the check. It was disabled earlier because of a bug in the linter.

pohly · 2025-05-02T12:16:37Z

/test pull-kubernetes-integration

TestVolumeBindingRescheduling timeout

This finishes the work started in kubernetes#131113. Changes: - Remove TODOs for things that we don't have plans to fix. - Add issue for older TODO. - Reorganize and remove suppression rules so that the base check has no unused rules. - Document warn-unused, but don't enable it. - Remove disabling of statistics (they are useful) and ensure that they don't get the ERROR prefix. - Avoid ../ prefix in paths via `run.relative-path-mode: gomod`.

The advantage is that it separates the dependencies of the different tools better. golangci-lint in particular has many dependencies and is sometimes sensitive to the exact version being used. This way, "go get" bumps up dependencies exactly as defined by the upstream golangci-lint module. It's not quite self-contained because logcheck as a Go plugin for golangci-lint must be built from the same dependencies. But it only adds one and does not change any of the others. While at it, the Go 1.24 "tools" directive gets used instead of the traditional tools.go approach.

liggitt · 2025-05-03T04:16:00Z

cc @kubernetes/sig-api-machinery-bugs
unit test flake:

k8s.io/apiserver/pkg/admission/plugin/policy/validating
{Failed  === RUN   TestParamRef/ParamRefWithNameWithNamespaceAllowNotFound

/retest
/lgtm
/approve

k8s-ci-robot · 2025-05-03T04:16:06Z

LGTM label has been added.

Git tree hash: 6c2f07f71e15663eb30eb985328b09a0756aec61

k8s-ci-robot · 2025-05-03T04:16:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, liggitt, pohly

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [BenTheElder,liggitt,pohly]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/controller/disruption/OWNERS~~ [liggitt]
~~pkg/kubelet/certificate/OWNERS~~ [liggitt]
~~pkg/proxy/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/pkg/apis/OWNERS~~ [liggitt]
~~staging/src/k8s.io/code-generator/OWNERS~~ [liggitt]
~~staging/src/k8s.io/mount-utils/OWNERS~~ [liggitt]
~~test/e2e_node/OWNERS~~ [BenTheElder,liggitt,pohly]

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

github-project-automation bot added this to SIG Auth and SIG Apps Apr 25, 2025

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Apr 25, 2025

github-project-automation bot added this to SIG Node CI/Test Board Apr 25, 2025

github-project-automation bot moved this to Needs Triage in SIG Auth Apr 25, 2025

github-project-automation bot moved this to Needs Triage in SIG Apps Apr 25, 2025

k8s-ci-robot added the sig/testing Categorizes an issue or PR as relevant to SIG Testing. label Apr 25, 2025

github-project-automation bot moved this to Triage in SIG Node CI/Test Board Apr 25, 2025

pohly mentioned this pull request Apr 25, 2025

chore: bump golangci-lint to v2 #131113

Closed

pohly changed the title ~~Golangci lint@v2~~ golangci-lint v2 Apr 25, 2025

k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Apr 29, 2025

BenTheElder reviewed Apr 29, 2025

View reviewed changes

k8s-ci-robot assigned liggitt Apr 30, 2025

liggitt reviewed Apr 30, 2025

View reviewed changes

k8s-ci-robot requested a review from haircommander April 30, 2025 17:22

haircommander moved this from Triage to PRs - Needs Reviewer in SIG Node CI/Test Board Apr 30, 2025

mmorel-35 reviewed Apr 30, 2025

View reviewed changes

BenTheElder mentioned this pull request May 1, 2025

Add kube-api-linter verify scripts #131561

Open

mmorel-35 and others added 2 commits May 2, 2025 12:51

chore: bump golangci-lint to v2

4adb585

Signed-off-by: Matthieu MOREL <[email protected]>

DRA node test: fix useless gomega.Consistently

9bada79

Passing a constant value to gomega.Consistently means that it will not re-check while running. Found by linter after removing the suppression rule for the check. It was disabled earlier because of a bug in the linter.

pohly force-pushed the golangci-lint@v2 branch from 9b6a16a to 2e3e9f4 Compare May 2, 2025 10:51

pohly added 2 commits May 2, 2025 14:38

pohly force-pushed the golangci-lint@v2 branch from 2e3e9f4 to 0faeb5a Compare May 2, 2025 12:38

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 3, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2025

k8s-ci-robot merged commit 0b81338 into kubernetes:master May 3, 2025
16 checks passed

k8s-ci-robot added this to the v1.34 milestone May 3, 2025

github-project-automation bot moved this from PRs - Needs Reviewer to Done in SIG Node CI/Test Board May 3, 2025

github-project-automation bot moved this from Needs Triage to Done in SIG Apps May 3, 2025

github-project-automation bot moved this from Needs Triage to Closed / Done in SIG Auth May 3, 2025

bart0sh mentioned this pull request May 5, 2025

Migrate Kubelet codebase to contextual logging #130069

Open

49 tasks

	if in.Default != nil {
	in, out := &in.Default, &out.Default
	*out = new(apiextensions.JSON)
	if err := Convert_v1_JSON_To_apiextensions_JSON(in, out, s); err != nil {
	return err
	}
	} else {
	out.Default = nil
	}

	if in.Example != nil {
	in, out := &in.Example, &out.Example
	*out = new(apiextensions.JSON)
	if err := Convert_v1_JSON_To_apiextensions_JSON(in, out, s); err != nil {
	return err
	}
	} else {
	out.Example = nil
	}

		@@ -153,9 +155,8 @@ if [ "${golangci_config}" ]; then
		# don't to do it when not required.
		if grep -q 'path: ../_output/local/bin/' "${golangci_config}" &&

golangci-lint v2 #131477

golangci-lint v2 #131477

Conversation

pohly commented Apr 25, 2025

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Apr 25, 2025

What type of PR is this?

What this PR does / why we need it:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez Apr 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez May 2, 2025 • edited Loading

Choose a reason for hiding this comment

liggitt Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmorel-35 Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haircommander commented Apr 30, 2025

Choose a reason for hiding this comment

pohly commented May 2, 2025

liggitt commented May 3, 2025

k8s-ci-robot commented May 3, 2025

k8s-ci-robot commented May 3, 2025

liggitt Apr 30, 2025 •

edited

Loading

ldez Apr 29, 2025 •

edited

Loading

ldez May 2, 2025 •

edited

Loading

liggitt Apr 30, 2025 •

edited

Loading

mmorel-35 Apr 30, 2025 •

edited

Loading