✨ (go/v4): upgrade golangci-lint to v2.12.1#5676
Conversation
Signed-off-by: dongjiang <dongjiang1989@126.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dongjiang1989 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test pull-kubebuilder-e2e-k8s-1-36-0 |
Signed-off-by: dongjiang <dongjiang1989@126.com>
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 |
There was a problem hiding this comment.
If the goal of this config is to reduce noise from test files and we are already ignoring the tests, is min-ocurrences: 5 still necessary?
There was a problem hiding this comment.
min-ocurrences also applies to non-test files
There was a problem hiding this comment.
Hm...so the summary in the description is a bit misleading. In regards to this config, I favor explicitly adding no-lint directives to ocurrences than silently ignoring them. Also, a minimum of 5 seems a bit too much, doesn't it? It may solve problems at hand, but might also hide ocurrences in the long run.
There was a problem hiding this comment.
Thanks! Agreed on both points.
• I’ll lower min-occurrences to 3.
• For specific cases, we’ll use explicit //nolint directives instead of silent ignores
WDYT? Default value
There was a problem hiding this comment.
Final offer: min of 2, take it or leave it!
Just kidding. Let's wait and hear what @camilamacedo86 has to say.
There was a problem hiding this comment.
I do not think that we should ignore goconst
We need to fix the places where the error is raised instead
There was a problem hiding this comment.
Got it. I'll remove min-occurrences
There was a problem hiding this comment.
I am concern with the changes in the default scaffold.
What are the places that fails in the default scaffold?
Are too many? If not I think we should solve it out
|
|
||
| if len(busybox.Status.Conditions) == 0 { | ||
| //nolint:goconst | ||
| meta.SetStatusCondition(&busybox.Status.Conditions, metav1.Condition{Type: typeAvailableBusybox, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: "Starting reconciliation"}) |
There was a problem hiding this comment.
What is the error here?
What is the value that the linter is asking for a const?
There was a problem hiding this comment.
ref: https://github.com/kubernetes-sigs/kubebuilder/actions/runs/25423812234/job/74572117344
goconst default value:
linters:
settings:
goconst:
# Minimal length of string constant.
# Default: 3
min-len: 2
# Minimum occurrences of constant string count to trigger issue.
# Default: 3
min-occurrences: 2
# Look for existing constants matching the values.
There was a problem hiding this comment.
I will need to check it properly.
How are we linting the samples?
Are we using the lint from samples or Kubebuilder config?
We should not change the config in the default scaffold.
There was a problem hiding this comment.
Thanks @camilamacedo86
Using the lint config from samples; the lint rules in samples are broader in scope than those in golangci-linter default value.
|
Hey @camilamacedo86 @vitorfloriano Please re-check it |
Signed-off-by: dongjiang <dongjiang1989@126.com>
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 |
There was a problem hiding this comment.
I do not think we should add it at all and change the golang ci config for the default scaffold, could we revert this change?
There was a problem hiding this comment.
Hmmmm.... The lint rules in samples are broader in scope than those in golangci-linter default value(remove it). 🤔
There was a problem hiding this comment.
It is in the sample because the default scaffold was changed.
We should not change the default scaffold config unless it is really required
| forbid-focus-container: true | ||
| forbid-spec-pollution: true | ||
| goconst: | ||
| ignore-tests: true |
There was a problem hiding this comment.
I do not think we should add it here as well.
What are the failures ?
There was a problem hiding this comment.
so many errors:
......
pkg/plugins/golang/v4/api_test.go:120:14: string `Frigate` has 3 occurrences, make it a constant (goconst)
Kind: "Frigate",
^
pkg/plugins/golang/v4/webhook_test.go:44:14: string `crew` has 7 occurrences, make it a constant (goconst)
Group: "crew",
^
pkg/plugins/golang/v4/webhook_test.go:45:14: string `test.io` has 8 occurrences, make it a constant (goconst)
Domain: "test.io",
^
pkg/plugins/golang/v4/webhook_test.go:47:14: string `Captain` has 4 occurrences, make it a constant (goconst)
Kind: "Captain",
^
pkg/plugins/golang/v4/webhook_test.go:49:14: string `captains` has 3 occurrences, make it a constant (goconst)
Plural: "captains",
^
pkg/plugins/golang/v4/webhook_test.go:122:15: string `ship` has 3 occurrences, make it a constant (goconst)
Group: "ship",
^
pkg/plugins/golang/v4/webhook_test.go:125:15: string `Frigate` has 3 occurrences, make it a constant (goconst)
Kind: "Frigate",
^
pkg/plugins/optional/helm/v1alpha/scaffolds/edit.go:269:48: string `rbac` has 3 occurrences, make it a constant (goconst)
{"config/rbac", "dist/chart/templates/rbac", "rbac"},
^
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/helpers.go:93:4: string `spec` has 4 occurrences, make it a constant (goconst)
{"spec", "template", "spec", "containers"},
^
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/templater_test.go:38:22: string `test-project` has 7 occurrences, make it a constant (goconst)
detectedPrefix: "test-project",
^
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/templater_test.go:3804:36: string `infrastructure` has 4 occurrences, make it a constant (goconst)
"manager-role-infrastructure": "infrastructure",
^
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/templater_test.go:3805:36: string `users` has 3 occurrences, make it a constant (goconst)
"manager-role-users": "users",
^
pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/templater_test.go:3909:5: string `manager-role` has 3 occurrences, make it a constant (goconst)
"manager-role": "app-infrastructure",
^
50 issues:
* goconst: 50
``
Signed-off-by: dongjiang <dongjiang1989@126.com>
Signed-off-by: dongjiang <dongjiang1989@126.com>
Signed-off-by: dongjiang <dongjiang1989@126.com>
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 |
There was a problem hiding this comment.
Could you please revert it?
Then run make install and make generate ro regenerate all samples and etc?
We should not change the rules for what is scaffold by the tool
There was a problem hiding this comment.
Thanks @camilamacedo86
Remove it many errors.
Do you mind if I remove it in the next PR to keep the scope of code changes smaller for this PR?
There was a problem hiding this comment.
Pull request overview
Upgrades the golangci-lint version used by Kubebuilder and its scaffold/testdata projects, and updates lint configuration/templates to accommodate new linter behavior (notably goconst) and reduce repeated string literals.
Changes:
- Bump golangci-lint from
v2.11.4tov2.12.1across Makefiles, GitHub Actions, and scaffold/testdata configs. - Add/extend
goconstconfiguration in scaffold/testdata/template.golangci.ymlfiles and adjust code/templates to reduce repeated string literals. - Refactor a few spots to use shared constants and
slices.Backward()for reverse scanning.
Reviewed changes
Copilot reviewed 41 out of 41 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Updates root tool version for golangci-lint. |
| .golangci.yml | Enables/configures goconst at the repo level. |
| .github/workflows/verify-all.yml | Updates golangci-lint version used in CI workflow. |
| .github/workflows/lint-sample.yml | Updates golangci-lint version for sample linting. |
| pkg/plugins/golang/v4/scaffolds/init.go | Updates scaffolded golangci-lint version constant. |
| pkg/plugins/golang/v4/scaffolds/internal/templates/golangci.go | Updates scaffolded .golangci.yml template (adds goconst settings). |
| pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_test_updater.go | Uses slices.Backward() for reverse scanning in template updater. |
| pkg/plugins/golang/options.go | Deduplicates repeated "k8s.io" strings via a constant. |
| pkg/plugins/golang/deploy-image/v1alpha1/scaffolds/internal/templates/controllers/controller.go | Deduplicates condition Reason strings via constants in controller template. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/rbac.go | Uses slices.Backward() for reverse scanning in YAML processing. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/manager.go | Uses slices.Backward() for reverse scanning in indentation detection. |
| pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/helpers.go | Deduplicates pod-template field paths used for container-name extraction. |
| pkg/plugins/optional/helm/v1alpha/scaffolds/edit.go | Replaces repeated subdir strings with constants. |
| pkg/machinery/marker.go | Centralizes .go extension as goExt constant for reuse. |
| pkg/machinery/scaffold.go | Uses goExt constant when deciding to run imports.Process. |
| pkg/cli/cli.go | Centralizes go plugin key strings as constants for reuse. |
| pkg/cli/root.go | Uses constants for plugin filtering logic in help output. |
| internal/cli/alpha/internal/generate.go | Replaces repeated subcommand/flag/plugin-key strings with constants. |
| internal/cli/alpha/internal/update/helpers/open_gh_issue.go | Replaces repeated critical-filename strings with constants. |
| testdata/project-v4/Makefile | Updates golangci-lint version in v4 testdata Makefile. |
| testdata/project-v4/.golangci.yml | Adds goconst settings to v4 testdata config. |
| testdata/project-v4/.custom-gcl.yml | Updates custom golangci-lint plugin config version. |
| testdata/project-v4-with-plugins/Makefile | Updates golangci-lint version in plugins testdata Makefile. |
| testdata/project-v4-with-plugins/.golangci.yml | Adds goconst settings to plugins testdata config. |
| testdata/project-v4-with-plugins/.custom-gcl.yml | Updates custom golangci-lint plugin config version. |
| testdata/project-v4-with-plugins/internal/controller/memcached_controller.go | Deduplicates condition Reason strings via constants in testdata controller. |
| testdata/project-v4-with-plugins/internal/controller/busybox_controller.go | Deduplicates condition Reason strings via constants in testdata controller. |
| testdata/project-v4-multigroup/Makefile | Updates golangci-lint version in multigroup testdata Makefile. |
| testdata/project-v4-multigroup/.golangci.yml | Adds goconst settings to multigroup testdata config. |
| testdata/project-v4-multigroup/.custom-gcl.yml | Updates custom golangci-lint plugin config version. |
| testdata/project-v4-multigroup/internal/controller/example.com/memcached_controller.go | Deduplicates condition Reason strings via constants in multigroup testdata controller. |
| testdata/project-v4-multigroup/internal/controller/example.com/busybox_controller.go | Deduplicates condition Reason strings via constants in multigroup testdata controller. |
| docs/book/src/getting-started/testdata/project/Makefile | Updates golangci-lint version in docs testdata Makefile. |
| docs/book/src/getting-started/testdata/project/.golangci.yml | Adds goconst settings to docs testdata config. |
| docs/book/src/getting-started/testdata/project/.custom-gcl.yml | Updates custom golangci-lint plugin config version. |
| docs/book/src/cronjob-tutorial/testdata/project/Makefile | Updates golangci-lint version in docs testdata Makefile. |
| docs/book/src/cronjob-tutorial/testdata/project/.golangci.yml | Adds goconst settings to docs testdata config. |
| docs/book/src/cronjob-tutorial/testdata/project/.custom-gcl.yml | Updates custom golangci-lint plugin config version. |
| docs/book/src/multiversion-tutorial/testdata/project/Makefile | Updates golangci-lint version in docs testdata Makefile. |
| docs/book/src/multiversion-tutorial/testdata/project/.golangci.yml | Adds goconst settings to docs testdata config. |
| docs/book/src/multiversion-tutorial/testdata/project/.custom-gcl.yml | Updates custom golangci-lint plugin config version. |
Signed-off-by: dongjiang <dongjiang1989@126.com>
|
@dongjiang1989: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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. I understand the commands that are listed here. |
Summary
goconstlinter configuration (ignore-tests: true,min-occurrences: 5) to reduce noise from test filesslices.Backward()in 3 files to satisfy the new goconst linter//nolint:goconstfor intentional string constants inoptions.go