🌱 Add golangci-lint v2 configuration and fix all lint issues#200
Conversation
Add golangci-lint v2 configuration with automated version detection and install scripts. Fix all 69 lint issues across the codebase without introducing any breaking changes. Lint fixes applied: - goimports: fix import grouping across non-test files - misspell: fix US English spelling in comments (cancelled -> canceled) - prealloc: preallocate slices with known capacity - staticcheck QF1008: simplify embedded field selectors - unconvert: remove unnecessary type conversions - revive var-naming: rename local variables and parameters to follow Go conventions (metaJson -> metaJSON, overloadId -> overloadID, etc.) - revive context-as-argument: move ctx to first parameter position - unparam: remove always-nil error return from unexported function Lint exclusions for public API stability: - Exported type names that stutter (e.g., grpc.GRPCServer) are excluded via config rules since renaming would break external consumers - Exported functions returning unexported types are excluded - Package names (common, utils, types) are excluded - Individual nolint directives added for JsonLib, MQTT_*_KEY constants, and two unparam cases where parameters are kept for readability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xuezhaojun <zxue@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
WalkthroughAdds a golangci-lint v2 workflow (installer, runner, v2 config, Makefile change) and a set of small code edits: import/whitespace fixes, identifier renames, slice preallocation, a manifest builder signature change, new Addon v1beta1 accessor, and dynamic TLS certificate reloading for the gRPC server. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
|
/assign @qiujian16 Please take a review when you have time, thanks! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/cloudevents/clients/addon/wrapper.go (1)
3-17:⚠️ Potential issue | 🟠 MajorImport ordering violates goimports configuration and must be reorganized.
With
local-prefixes: open-cluster-management.ioconfigured, goimports groups imports as: stdlib → third-party → local. The current imports place the localopen-cluster-management.io/sdk-goimport (line 6) before the third-partyk8s.ioimport (line 8), which is incorrect. Allopen-cluster-management.ioimports should be grouped together after third-party imports. Reorder to: stdlib (context), then third-party (k8s.io), then allopen-cluster-management.ioimports.
🤖 Fix all issues with AI agents
In `@ci/lint/install-golangci-lint.sh`:
- Around line 163-168: The install script currently skips installation only on
exact match of INSTALLED_VERSION == GOLANGCI_LINT_VERSION which can cause
unwanted downgrades; update the check in install-golangci-lint.sh to perform a
proper semver comparison (>=) like run-lint.sh instead of exact equality.
Implement or reuse the same version-compare logic used by run-lint.sh (or add a
small compare_versions/get_semver_compare function) and change the conditional
that inspects INSTALLED_VERSION and GOLANGCI_LINT_VERSION so it exits when
INSTALLED_VERSION is greater-than-or-equal-to the target, preserving the current
behavior in run-lint.sh.
- Around line 226-257: The script's copy_config function writes .golangci.yml
(LOCAL_CONFIG) into the repo root which creates uncommitted changes; change
copy_config to write the config to a temporary path (e.g. use mktemp or
/tmp/golangci-lint-config) and update any callers/logic that check LOCAL_CONFIG
to prefer that temp file when present (keep CONFIG_SOURCE and CONFIG_FILE names
but avoid creating LOCAL_CONFIG in the repo), or alternatively add logic to
append LOCAL_CONFIG to .gitignore before writing; modify the branch that calls
copy_config and the EXISTING CONFIG check to use the temp location to prevent
dirty working trees in CI.
In `@ci/lint/run-lint.sh`:
- Around line 1-2: The script currently uses "set -e" which allows pipeline
failures to be masked; update the shell options to include pipefail by replacing
the "set -e" invocation with a setting that enables pipefail (e.g., "set -e -o
pipefail" or "set -eo pipefail") so pipelines like "curl ... | sh -s ..." will
surface errors; locate the "set -e" line near the top of the script
(ci/lint/run-lint.sh) and change it accordingly.
🧹 Nitpick comments (5)
pkg/cloudevents/generic/options/grpc/options.go (1)
299-301: Redundant intermediate slice —clientOptscan be passed directly.
optsis created only to hold a copy ofclientOptsand is then immediately spread intoNewProtocol. You can eliminate the allocation entirely:♻️ Suggested simplification
- opts := make([]protocol.Option, 0, len(clientOpts)) - opts = append(opts, clientOpts...) - return protocol.NewProtocol(conn, opts...) + return protocol.NewProtocol(conn, clientOpts...)ci/lint/README-golangci-lint.md (1)
9-13: Security note: piping curl to bash in the Quick Start example.The recommended
curl -sSL ... | bashpattern executes remote code without integrity verification. This is a common CI convenience, but consider noting that users should pin to a specific commit SHA rather thanmainfor reproducibility and supply-chain safety (e.g.,https://raw.githubusercontent.com/.../\<commit-sha\>/ci/lint/run-lint.sh).ci/lint/golangci-v2.yml (1)
87-91: Excluding all test files from linting is a broad exclusion.
_test\.gois excluded from all linters viapaths. This means tests won't benefit fromgovet,errcheck,staticcheck, etc. While this reduces initial noise, it also means bugs in test code (e.g., unchecked errors, incorrect vet findings) will go undetected. Consider moving test exclusions to specific noisy linters only, or plan to remove this blanket exclusion once the test files are cleaned up.ci/lint/run-lint.sh (1)
103-123:install_lintduplicates logic already ininstall-golangci-lint.sh.Both this function and
ci/lint/install-golangci-lint.shimplement Go version detection, version selection, and installation via curl. Divergence between the two is likely over time. Consider havingrun-lint.shdelegate toinstall-golangci-lint.shfor installation, or consolidate the logic in one place.ci/lint/install-golangci-lint.sh (1)
115-145: Platform normalization is redundant whengo envis available.Lines 116-117 already call
go env GOOSandgo env GOARCH(which return normalized values), then lines 120-145 re-normalize the same values. Theunamefallback is the only case needing normalization. Consider simplifying:Suggested simplification
-GOOS="${GOOS:-$(go env GOOS 2>/dev/null || uname -s | tr '[:upper:]' '[:lower:]')}" -GOARCH="${GOARCH:-$(go env GOARCH 2>/dev/null || uname -m)}" - -# Normalize architecture names -case "${GOARCH}" in - x86_64|amd64) - GOARCH="amd64" - ;; - aarch64|arm64) - GOARCH="arm64" - ;; - *) - echo "Unsupported architecture: ${GOARCH}" - exit 1 - ;; -esac - -# Normalize OS names -case "${GOOS}" in - Darwin|darwin) - GOOS="darwin" - ;; - Linux|linux) - GOOS="linux" - ;; - *) - echo "Unsupported OS: ${GOOS}" - exit 1 - ;; -esac +GOOS="${GOOS:-$(go env GOOS 2>/dev/null || uname -s | tr '[:upper:]' '[:lower:]')}" +GOARCH="${GOARCH:-$(go env GOARCH 2>/dev/null || uname -m)}" + +# Normalize values that may come from uname fallback or user override +case "${GOARCH}" in + x86_64) GOARCH="amd64" ;; + aarch64) GOARCH="arm64" ;; + amd64|arm64) ;; # already normalized + *) echo "Unsupported architecture: ${GOARCH}"; exit 1 ;; +esac + +case "${GOOS}" in + Darwin) GOOS="darwin" ;; + Linux) GOOS="linux" ;; + darwin|linux) ;; # already normalized + *) echo "Unsupported OS: ${GOOS}"; exit 1 ;; +esac
Add early return for empty input to maintain nil return value, which was broken by the prealloc optimization (make returns non-nil empty slice). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xuezhaojun <zxue@redhat.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: xuezhaojun <zxue@redhat.com>
|
/assign @zhujian7 Please take a review, I'm thinking we can apply the same pattern for envtest setup. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16, xuezhaojun 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 57fc6b2
into
open-cluster-management-io:main
3 participants
Summary
ci/lint/golangci-v2.yml) with automated version detection and install scriptsmake verifyviaverify-gocilinttargetmake verifyLint Configuration
Linters Enabled
open-cluster-management.iolocal prefix)Exclusion Rules for Public API Stability
Since this SDK is consumed by many external repositories, the following lint rules are excluded via config to avoid breaking changes:
Additionally,
//nolintdirectives are used for 3 specific exported symbols:JsonLib()function name (should beJSONLibper convention)MQTT_SOURCE_PUB_TOPIC_KEY/MQTT_AGENT_PUB_TOPIC_KEYconstants (ALL_CAPS style)Lint Fixes Applied (No Breaking Changes)
All fixes are internal implementation details or comment-only changes. No exported types, functions, constants, or variables were renamed or removed. External consumers will not experience any compilation errors or behavior changes.
cancelled→canceled,Cancelling→Canceling(comments only)make([]T, 0, cap)config.CertConfig.EmbedCerts()→config.EmbedCerts())types.Int()conversionmetaJson→metaJSON,overloadId→overloadID,blockTlsCrt→blockTLSCrt,clientId→clientID,sourceId→sourceIDctxto first param in unexportednewWrappedCloudEventsMetricsStreamnewManifestWorks; rename unused closure param to_No Breaking Changes Guarantee
GRPCServer,MQTTOptions,CertConfig,ServingCertController) retain their original namesJsonLib(),NewScoreNormalizer(),NewGRPCMetricsHandler()) retain their original signaturesMQTT_SOURCE_PUB_TOPIC_KEY) retain their original namesclientId→clientID, etc.) do not affect callers since Go uses positional argumentsTest plan
make verifypasses with 0 lint issues (verified with 2 consecutive runs)go build ./...compiles successfully_test.gofiles modified🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Chores