🌱 chore: adopt shared golangci-lint configuration from sdk-go#384
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR consolidates multiple code quality improvements across the addon-framework: replaces local golangci-lint configuration with OCM SDK lint script, adds linter directives to suppress revive warnings on undocumented exported functions, removes deprecated Go random seeding and time imports, reorganizes all import blocks, migrates to newer Kubernetes utilities ( ChangesLinting Compliance and Code Quality Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tesshuflower
changed the title
Replace the local golangci-lint v1 setup with the shared sdk-go lint
infrastructure. The shared run-lint.sh script auto-selects the correct
golangci-lint version based on the project Go version.
Changes:
- Makefile: replace verify-gocilint target with lint target using sdk-go run-lint.sh
- Remove .golangci.yaml (v1 format, incompatible with golangci-lint v2)
- Fix all 86 lint issues found by the stricter v2 linter:
- goimports (40): separate open-cluster-management.io imports into local group
- staticcheck (22): remove deprecated rand.Seed, fix duplicate imports,
use RunWithContext, replace k8s.io/utils/pointer with k8s.io/utils/ptr
- revive (14): fix naming conventions, add nolint for exported API names
- prealloc (5): preallocate slices with known capacity
- unparam (3): use _ for unused function parameters
- misspell (2): fix spelling in comments
Part of ongoing standardization of lint tooling across OCM repositories.
Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Tesshu Flower <tflower@redhat.com>
tesshuflower
force-pushed
the
adopt-sdk-go-lint
branch
from
557d8d8 to
e9770f9
Compare
mikeshng
left a comment
mikeshng
left a comment
There was a problem hiding this comment.
Similar to what was done for: #384
This is pointing at itself? Do you mean another PR?
/hold
Let's discuss. I think linting is less of an issue now days with AI. I rather not spend AI tokens on just getting the linting to pass. As long as it's reasonable it should be ok but shouldn't be super strict. As code are mostly generated by AI now days, it should be pretty linted already also.
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR standardizes linting for this repository by adopting the shared golangci-lint v2 configuration and runner from open-cluster-management-io/sdk-go, and applies repo-wide mechanical updates needed to satisfy the new lint rules (import grouping, deprecation cleanups, minor naming tweaks, and small performance nits like slice preallocation).
Changes:
- Replace the local
golangci-lintsetup with the sharedsdk-gorun-lint.shrunner viamake lintand remove the legacy.golangci.yamlconfig. - Apply
goimports-style import grouping updates across packages/examples to match the shared config. - Address linter findings (deprecated API usage, minor naming cleanups, preallocations, and a few small logic/style simplifications).
Reviewed changes
Copilot reviewed 47 out of 47 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
Makefile |
Replaces local golangci-lint install/run with remote shared sdk-go runner via make lint. |
.golangci.yaml |
Removes legacy v1-format golangci-lint configuration. |
pkg/utils/probe_helper.go |
Preallocates probe field slices; goimports grouping. |
pkg/utils/permission.go |
Switches from deprecated k8s.io/utils/pointer to k8s.io/utils/ptr. |
pkg/utils/image.go |
goimports grouping. |
pkg/utils/helpers.go |
goimports grouping. |
pkg/utils/csr_helpers.go |
Renames Tls vars to TLS-style; goimports grouping. |
pkg/utils/config_checker.go |
Receiver rename cleanup in Check; (comment spelling issue remains). |
pkg/utils/addon_config.go |
goimports grouping. |
pkg/index/index.go |
Adds //nolint:revive for exported API names; small simplification; goimports grouping. |
pkg/cmd/factory/factory.go |
Removes rand.Seed, uses RunWithContext/SetupSignalHandler correctly; comment spelling tweak (“canceling”). |
pkg/assets/template.go |
Replaces strings.Replace(..., -1) with strings.ReplaceAll. |
pkg/agent/v1alpha1/interface.go |
goimports grouping. |
pkg/agent/interface.go |
goimports grouping. |
pkg/agent/adapter.go |
goimports grouping. |
pkg/addonmanager/manager.go |
goimports grouping. |
pkg/addonmanager/interface.go |
goimports grouping. |
pkg/addonmanager/controllers/registration/controller.go |
Refactors driver selection to switch; goimports grouping. |
pkg/addonmanager/controllers/cmamanagedby/controller.go |
goimports grouping. |
pkg/addonmanager/controllers/cmaconfig/controller.go |
Slice prealloc; uses embedded Group/Resource fields; goimports grouping. |
pkg/addonmanager/controllers/certificate/csrsign.go |
goimports grouping. |
pkg/addonmanager/controllers/certificate/csrapprove.go |
Refactors condition checks to switch; goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/utils.go |
Naming cleanup (JSON); minor boolean init cleanup; goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/hosted_sync.go |
goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/hosted_hook_sync.go |
goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/healthcheck_sync.go |
Uses embedded Group/Resource fields; goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/default_sync.go |
goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/default_hook_sync.go |
goimports grouping. |
pkg/addonmanager/controllers/agentdeploy/controller.go |
goimports grouping. |
pkg/addonmanager/controllers/addonconfig/controller.go |
Slice prealloc; uses embedded Group/Resource fields; goimports grouping. |
pkg/addonmanager/cloudevents/manager.go |
Slice prealloc; goimports grouping. |
pkg/addonmanager/base_manager.go |
goimports grouping. |
pkg/addonmanager/addontesting/helpers.go |
goimports grouping. |
pkg/addonfactory/test_helper.go |
goimports grouping. |
pkg/addonfactory/template_agentaddon.go |
Marks unused param; goimports grouping. |
pkg/addonfactory/helper.go |
Adds //nolint:revive for exported API naming constraint. |
pkg/addonfactory/helm_agentaddon.go |
Marks unused params; goimports grouping. |
pkg/addonfactory/addonfactory.go |
goimports grouping. |
pkg/addonfactory/addondeploymentconfig.go |
goimports grouping. |
examples/rbac/rbac.go |
goimports grouping. |
examples/helloworld/helloworld.go |
goimports grouping. |
examples/helloworld_helm/helloworld_helm.go |
JSON naming cleanup; uses embedded Group/Resource fields; goimports grouping. |
examples/helloworld_agent/cleanup_agent.go |
goimports grouping. |
examples/helloworld_agent/agent.go |
goimports grouping. |
cmd/example/helloworld/main.go |
Removes rand.Seed usage; goimports grouping. |
cmd/example/helloworld_hosted/main.go |
Removes rand.Seed usage. |
cmd/example/helloworld_helm/main.go |
Removes rand.Seed usage; goimports grouping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .PHONY: lint | ||
| lint: | ||
| @bash -o pipefail -c 'curl -fsSL https://raw.githubusercontent.com/open-cluster-management-io/sdk-go/main/ci/lint/run-lint.sh | bash' |
There was a problem hiding this comment.
This is just following the practice from other ocm repos and in fact does not get diferent golangci-lint versions each time, but specific ones that align with the go version used in go.mod.
The second part is actually a good point, I had used the cluster-proxy change as a reference, and it should have the same issue as it also has vendoring
Update: I think the default mode of readonly is ok even with vendoring: https://go.dev/ref/mod#go-mod-file-go
specifically:
At go 1.14 or higher, automatic vendoring may be enabled. If the file vendor/modules.txt is present and consistent with go.mod, there is no need to explicitly use the -mod=vendor flag.
| // Note that: configChecker performs a instant update after it returns err, so DO NOT use one | ||
| // configChecker for multible containers!!! | ||
| func (cc *configChecker) Check(_ *http.Request) error { | ||
| newChecksum, err := load(cc.configfiles) | ||
| func (c *configChecker) Check(_ *http.Request) error { |
oops, wrong link, it's this one: open-cluster-management-io/cluster-proxy#279 I disagree, think we should be doing linting, this can reduce AI tokens by getting easy linting fixes that AI doesn't always catch on its own, causing more reviews etc - This change is to use the common linting from sdk-go as other repos do - which will also help when moving to golang v1.26, as the common script from sdk-go picks up the correct version of golangci-lint. To be clear, this is also not adding new linting, this is just moving the linter to download using a shared script from sdk-go instead of a hardcoded one in the Makefile (which we would need to keep up-to-date separately). |
mikeshng
left a comment
mikeshng
left a comment
There was a problem hiding this comment.
To be clear, this is also not adding new linting, this is just moving the linter to download using a shared script from sdk-go instead of a hardcoded one in the Makefile (which we would need to keep up-to-date separately).
Sounds good to me!
/unhold
/lgtm
|
Thanks @tesshuflower You will have to fix the linting error in the CI. |
Signed-off-by: Tesshu Flower <tflower@redhat.com>
tesshuflower
force-pushed
the
adopt-sdk-go-lint
branch
from
80b622f to
e433a02
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Around line 50-54: The Makefile is missing a .PHONY declaration for the verify
target; add `.PHONY: verify` to the .PHONY list (or create a new .PHONY line
that includes verify) so that the verify target (which depends on lint) is
always treated as a phony target rather than a file, preventing make from
skipping it when a file named "verify" exists.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7248b7a1-ba9a-40ce-bb1d-0e3a4c4b10f1
📒 Files selected for processing (48)
.golangci.yamlMakefilecmd/example/helloworld/main.gocmd/example/helloworld_helm/main.gocmd/example/helloworld_hosted/main.goexamples/helloworld/helloworld.goexamples/helloworld_agent/agent.goexamples/helloworld_agent/cleanup_agent.goexamples/helloworld_helm/helloworld_helm.goexamples/helloworld_hosted/helloworld.goexamples/rbac/rbac.gopkg/addonfactory/addondeploymentconfig.gopkg/addonfactory/addonfactory.gopkg/addonfactory/helm_agentaddon.gopkg/addonfactory/helper.gopkg/addonfactory/template_agentaddon.gopkg/addonfactory/test_helper.gopkg/addonmanager/addontesting/helpers.gopkg/addonmanager/base_manager.gopkg/addonmanager/cloudevents/manager.gopkg/addonmanager/controllers/addonconfig/controller.gopkg/addonmanager/controllers/agentdeploy/controller.gopkg/addonmanager/controllers/agentdeploy/default_hook_sync.gopkg/addonmanager/controllers/agentdeploy/default_sync.gopkg/addonmanager/controllers/agentdeploy/healthcheck_sync.gopkg/addonmanager/controllers/agentdeploy/hosted_hook_sync.gopkg/addonmanager/controllers/agentdeploy/hosted_sync.gopkg/addonmanager/controllers/agentdeploy/utils.gopkg/addonmanager/controllers/certificate/csrapprove.gopkg/addonmanager/controllers/certificate/csrsign.gopkg/addonmanager/controllers/cmaconfig/controller.gopkg/addonmanager/controllers/cmamanagedby/controller.gopkg/addonmanager/controllers/registration/controller.gopkg/addonmanager/interface.gopkg/addonmanager/manager.gopkg/agent/adapter.gopkg/agent/interface.gopkg/agent/v1alpha1/interface.gopkg/assets/template.gopkg/cmd/factory/factory.gopkg/index/index.gopkg/utils/addon_config.gopkg/utils/config_checker.gopkg/utils/csr_helpers.gopkg/utils/helpers.gopkg/utils/image.gopkg/utils/permission.gopkg/utils/probe_helper.go
💤 Files with no reviewable changes (2)
- .golangci.yaml
- cmd/example/helloworld_hosted/main.go
Addresses CodeRabbit suggestion: without .PHONY: verify, make would treat verify as a file target and silently skip it if a file named 'verify' existed in the directory. Signed-off-by: Tesshu Flower <tflower@redhat.com>
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mikeshng, qiujian16, tesshuflower The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 5017f37
into
open-cluster-management-io:main
Summary
Adopts the shared golangci-lint configuration from sdk-go to standardize lint tooling across OCM repositories.
Similar to what was done for: open-cluster-management-io/cluster-proxy#279
Should hopefully simplify the move to golang 1.26 as we'll automatically get the correct linter version.
Changes
verify-gocilinttarget withlinttarget that remotely fetches and executes the sharedrun-lint.shscript from sdk-go, which auto-selects the correct golangci-lint version based on the project's Go version.golangci.yaml: Old v1 format config, incompatible with golangci-lint v2; the shared sdk-go config is used insteadgoimports) to properly separateopen-cluster-management.iolocal imports from external packagesrand.Seed()calls (not needed since Go 1.20)k8s.io/apiserver/pkg/server; useRunWithContextinstead of deprecatedRunk8s.io/utils/pointerwithk8s.io/utils/ptrblockTlsCrt→blockTLSCrt,existJsonPaths→existJSONPaths, etc.)//nolint:revivefor exported API names that cannot be renamed without breaking changes (JsonStructToValues,Index*functions)_for unused function parametersNo Breaking Changes
All public API signatures, exported types, and function parameters remain unchanged. Naming convention fixes are only applied to unexported/local symbols.
Test plan
make lintpasses with 0 issuesmake test-unitpassesSummary by CodeRabbit
Chores
Refactor