Modify the golangci-lint pre-commit and Makefile flow#1687
Modify the golangci-lint pre-commit and Makefile flow#1687Neon-White merged 1 commit intonoobaa:masterfrom
golangci-lint pre-commit and Makefile flow#1687Conversation
WalkthroughCentralizes golangci-lint invocation into a new POSIX script and updates Makefile and pre-commit hook to delegate linting to that script; adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Hook as ".githooks/pre-commit"
participant Make as "Makefile"
participant Runner as "scripts/run-golangci-lint.sh"
participant Git as "git"
participant GCL as "golangci-lint"
participant Go as "go"
Dev->>Hook: git commit (pre-commit)
Hook->>Runner: exec ... precommit
Runner->>Git: detect staged .go files / base rev
alt staged Go files found
Runner->>GCL: run (targeted files / --new-from-rev)
alt golangci-lint missing (precommit)
Runner-->>Hook: warn & exit 0
else issues (ISSUES_EXIT_CODE)
Runner-->>Hook: report issues & exit 1
else success
Runner-->>Hook: pass & exit 0
end
else no staged files
Runner-->>Hook: exit 0
end
Dev->>Make: make lint
Make->>Runner: ./scripts/run-golangci-lint.sh makefile
Runner->>GCL: check presence
alt missing
Runner->>Go: go install golangci-lint@latest
Go-->>GCL: installed
end
Runner->>GCL: run (full repo, --issues-exit-code=42)
alt issues (exit 42)
Runner-->>Make: report & exit 1
else other error
Runner-->>Make: print error & exit non-zero
else success
Runner-->>Make: pass & exit 0
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
186-189: Update Makefile to drop deprecated linters for golangci-lint v2Your
.golangci.ymlis already using v2 syntax (version: "2") and contains no v1-only keys, so no config migration is needed. However, the Makefile still enables linters that have been removed or deprecated since v1.49.0—varcheck,structcheck,deadcode, andgosimple—which will cause CI errors under golangci-lint v2 (they now log “unknown linter” errors) (github.com, golangci-lint.run)Please update the
golangci-lint runinvocation (around line 187 ofMakefile) to only enable supported v2 linters. For example:golangci-lint: gen - golangci-lint run --disable-all -E varcheck,structcheck,typecheck,errcheck,gosimple,unused,deadcode,ineffassign,staticcheck --timeout=10m + golangci-lint run --disable-all -E typecheck,errcheck,unused,ineffassign,staticcheck --timeout=10m @echo "✅ golangci-lint" .PHONY: golangci-lint• Makefile: remove
varcheck,structcheck,deadcode, andgosimplefrom the-Elist
• (Optional) If you prefer centralizing linter configuration, consider moving your enabled linters into.golangci.ymlusing the v2 syntax:linters: default: none enable: - typecheck - errcheck - unused - ineffassign - staticcheck run: timeout: 10mThis will keep the Makefile cleaner and ensure consistency across CI runs.
🧹 Nitpick comments (2)
Makefile (2)
193-196: Version check is brittle; match both "2.4.0" and "v2.4.0" to avoid false reinstallsgolangci-lint v2.x --version output sometimes includes a leading “v” and sometimes not; grepping only for
version 2.4.0may fail even when the correct version is installed, causing unnecessary reinstalls. Use a regex that tolerates the optional prefix. Also consider pinning via a single variable to simplify future bumps.Apply within these lines:
- @if [ ! -f "$(GOBIN)/golangci-lint" ] || ! $(GOBIN)/golangci-lint --version 2>/dev/null | grep -q "version 2.4.0"; then \ - echo "Installing golangci-lint v2.4.0..."; \ - go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \ + @if [ ! -f "$(GOBIN)/golangci-lint" ] || ! $(GOBIN)/golangci-lint --version 2>/dev/null | grep -Eq 'version v?2\.4\.0\b'; then \ + echo "Installing golangci-lint v2.4.0..."; \ + go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \Optional follow-up (outside this hunk): introduce a single version knob and reuse it in both places to reduce future churn:
Makefile snippet:
GOLANGCI_LINT_VERSION ?= 2.4.0 GOLANGCI_LINT_TAG := v$(GOLANGCI_LINT_VERSION)Then:
grep -Eq "version v?$(GOLANGCI_LINT_VERSION)\b" go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_TAG)References: examples of v2 output without “v” in the version string; v2 migration notes. (github.com, golangci-lint.run)
195-195: Reconsider forcing rebuild with -a on every install (may slow CI)
go install <pkg>@<version>already builds that exact version in module mode. Adding-aforces recompilation of all dependencies and can significantly slow down repeated CI runs. If the intent is to avoid stale-cache/toolchain mismatches after bumping Go to 1.24, keep it but consider gating it behind an env var (e.g.,FORCE_REBUILD) or only when the detected version is wrong.Example tweak (within this line):
- go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \ + $${FORCE_REBUILD:+GOFLAGS=-a} go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \Note: Build flags like
-aare valid forgo install; they’re just often unnecessary when installing by module version. (pkg.go.dev)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
📚 Learning: 2025-06-25T15:42:04.436Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
Applied to files:
Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-cli-tests
- GitHub Check: run-hac-test
- GitHub Check: run-admission-test
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Makefile (1)
193-196: Doesn’t meet PR objective: still installs “latest” only when missing, and without -a; won’t fix the Go 1.23 vs 1.24 build mismatch.
- The PR description says: pin golangci-lint to v2.4.0 (built with Go 1.24) and use -a to force rebuild. The code installs @latest only if the binary is absent, and omits -a. If an older binary (built with go1.23) already exists, this target will keep using it and will continue to hit “the Go language version (go1.23) … lower than the targeted Go version (1.24)”.
- Past learnings: v2.x prints versions with a leading v. If you reintroduce version checks, account for the “v” prefix in parsing.
Proposed minimal fix: install when missing OR when the existing binary wasn’t built with Go ≥1.24, and force a rebuild of a known-good version v2.4.0.
@@ - @if [ ! -f "$(GOBIN)/golangci-lint" ] ; then \ - echo "Installing the golangci-lint..."; \ - go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest; \ - fi + @{ \ + set -e; \ + need_install=0; \ + if [ ! -x "$(GOBIN)/golangci-lint" ]; then \ + need_install=1; \ + elif ! "$(GOBIN)/golangci-lint" version 2>/dev/null | grep -qE 'built with go1\.2([4-9]|[0-9]{2,})'; then \ + need_install=1; \ + fi; \ + if [ "$$need_install" -eq 1 ]; then \ + echo "Installing golangci-lint@v2.4.0 (force rebuild)"; \ + go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \ + fi; \ + }If the team prefers to always float to the newest toolchain instead of pinning, switch the install line to “@latest” but keep the same detection and “-a”:
- go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \ + go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest; \You can verify the fix locally after applying the diff:
#!/bin/bash set -euo pipefail make lint FILES= 1>/dev/null || true "$(go env GOPATH)/bin/golangci-lint" version # Expect output to mention: # - "has version v2.4.0" (if pinned) or a v2.x.y version (if latest) # - "built with go1.24" (or higher)
🧹 Nitpick comments (1)
Makefile (1)
191-203: Optional: define a version variable for consistency and easier upgrades.Align with how CONTROLLER_GEN_VERSION/DEEPCOPY_GEN_VERSION are managed. This keeps policy explicit and avoids hidden drifts.
Add near other tool versions:
GOLANGCI_LINT_VERSION ?= v2.4.0Then replace the hardcoded version in the lint block:
- go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.4.0; \ + go install -a github.com/golangci/golangci-lint/v2/cmd/golangci-lint@$(GOLANGCI_LINT_VERSION); \Note: If you later reintroduce a semantic version check, remember the v-prefix in v2.x output per the retrieved learnings.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
Makefile(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
📚 Learning: 2025-06-25T15:42:04.436Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1637
File: Makefile:193-203
Timestamp: 2025-06-25T15:42:04.436Z
Learning: golangci-lint v2.x versions output the version with a leading "v" prefix (e.g., "golangci-lint has version v2.1.6..."), while v1.x versions output without the "v" prefix (e.g., "golangci-lint has version 1.64.8..."). Version detection logic should account for the specific version format being targeted.
Applied to files:
Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-kms-kmip-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-cli-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-operator-tests
- GitHub Check: run-hac-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: golangci-lint
golangci-lint to a version that was built with Go 1.24.0golangci-lint to a version that requires Go 1.24.0
liranmauda
left a comment
There was a problem hiding this comment.
Need to make sure that when we already have a golangci-lint binary, we will not decline commits just because the linter was built using an older version of golang.
It applies when switching branches and when updating golang version.
We need to make sure that the latest golinet is always installed, and the golang it was built with will not create that error.
f7baf9c to
8ddea6c
Compare
|
@liranmauda -
Sadly, these cannot coexist. At any given moment, It's possible to override this by setting While it is technically possible to have a script that tries to iterate through different build options, it is inconsistent, difficult to maintain, and confusing. |
golangci-lint to a version that requires Go 1.24.0golangci-lint pre-commit and Makefile flow
30c0e73 to
f7045e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
scripts/run-golangci-lint.sh (3)
47-49: Prefer letting git filter .go files; avoid sed-based space escaping.This is simpler and avoids brittle backslash escaping.
Apply this diff:
- STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' | sed 's| |\\ |g') + STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR -- '*.go')
61-64: Always pass explicit config for parity with Makefile mode.Keeps behavior consistent across modes.
Included in the earlier diff on Lines 59-66: added “--config .golangci.yml”.
19-19: shellcheck SC2155: split assignment and export.Avoids masking return values.
Apply this diff:
-export PATH="$PATH:$(go env GOPATH)/bin" +GOPATH_BIN="$(go env GOPATH)/bin" +export PATH="$PATH:$GOPATH_BIN".githooks/pre-commit (1)
10-11: Ensure execution from repo root and drop trailing space.Running from the project root avoids any relative-path surprises.
Apply this diff:
-# Call the unified golangci-lint script in precommit mode -exec "$PROJECT_ROOT/scripts/run-golangci-lint.sh" precommit +# Call the unified golangci-lint script in precommit mode +cd "$PROJECT_ROOT" +exec "$PROJECT_ROOT/scripts/run-golangci-lint.sh" precommitMakefile (1)
191-201: checkmake warning: target body length.If you want to satisfy maxbodylength, collapse the conditional or defer to the golangci-lint target.
Example minimal variant:
lint: gen @echo "" @./scripts/run-golangci-lint.sh makefile $(FILES) @echo "✅ lint"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.githooks/pre-commit(1 hunks)Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
🪛 checkmake (0.2.2)
Makefile
[warning] 191-191: Target body for "lint" exceeds allowed length of 5 (9).
(maxbodylength)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: run-hac-test
- GitHub Check: run-kms-dev-test
🔇 Additional comments (2)
.githooks/pre-commit (1)
6-11: Centralizing to the runner script is the right call.Simple, readable hook that delegates all logic. ✅
Makefile (1)
195-199: LGTM: lint target delegates to the unified script.Matches the new exit-code policy and central install handling.
f7045e7 to
ecb98cd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
scripts/run-golangci-lint.sh (3)
12-16: Make linter version configurable (default overridable).Add an env-driven version knob to support pinning while preserving an easy “latest” default.
readonly ISSUES_EXIT_CODE=42 + +# Linter version policy (overridable via env/Makefile). Examples: "v2.4.0" or "latest". +: "${GOLANGCI_LINT_VERSION:=latest}"
21-35: Always rebuild with local toolchain; parameterize version.Current logic installs only when missing, uses @latest, and doesn’t enforce local toolchain, which breaks across branch/toolchain switches.
install_golangci_lint() { GOBIN=$(go env GOBIN) if [ -z "$GOBIN" ]; then GOBIN=$(go env GOPATH)/bin fi - if [ ! -f "$GOBIN/golangci-lint" ]; then - echo "Installing latest golangci-lint and building with local toolchain" - if ! go -a install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest; then - echo "⚠️ Failed to install golangci-lint" - exit 0 - fi - fi + if [ ! -x "$GOBIN/golangci-lint" ]; then + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + else + echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + fi + if ! GOTOOLCHAIN=local go -a install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}"; then + echo "⚠️ Failed to install golangci-lint" + exit 0 + fi }Please validate this fixes the branch-switch/toolchain mismatch scenario you described.
59-66: Pre-commit currently lints unstaged changes when HEAD exists. Limit to staged.Append the staged files in both branches and use explicit config for consistency.
- if [ -z "$BASE" ]; then - # Initial commit – lint everything staged - golangci-lint run --issues-exit-code=$ISSUES_EXIT_CODE $STAGED_FILES - else - # Use new-from-rev to only check new/modified code - golangci-lint run --issues-exit-code=$ISSUES_EXIT_CODE --new-from-rev="$BASE" - fi + if [ -z "$BASE" ]; then + # Initial commit – lint only staged files + golangci-lint run --issues-exit-code="$ISSUES_EXIT_CODE" --config .golangci.yml $STAGED_FILES + else + # Lint only staged changes vs HEAD + golangci-lint run --issues-exit-code="$ISSUES_EXIT_CODE" --config .golangci.yml --new-from-rev="$BASE" $STAGED_FILES + fi
🧹 Nitpick comments (3)
Makefile (1)
186-194: Option: pass/pin linter version through Makefile.Expose GOLANGCI_LINT_VERSION so CI and developers can pin/override without editing the script.
Apply:
+GOLANGCI_LINT_VERSION ?= latest @@ - ./scripts/run-golangci-lint.sh makefile $(FILES); \ + GOLANGCI_LINT_VERSION=$(GOLANGCI_LINT_VERSION) ./scripts/run-golangci-lint.sh makefile $(FILES); \ @@ - ./scripts/run-golangci-lint.sh makefile; \ + GOLANGCI_LINT_VERSION=$(GOLANGCI_LINT_VERSION) ./scripts/run-golangci-lint.sh makefile; \If the team prefers pinning, set a concrete tag (e.g., v2.x.y) instead of latest.
scripts/run-golangci-lint.sh (2)
47-55: Safer staged-file collection (handles spaces/newlines robustly).Current backslash escaping is brittle. Use NUL-delimited paths.
- STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' | sed 's| |\\ |g') + # Build an array of staged .go files, NUL-safe + STAGED_FILES=$( + git diff --cached --name-only -z --diff-filter=ACMR \ + | tr '\0' '\n' | grep -E '\.go$' | sed 's/ /\\ /g' + )Alternatively, switch invocation to xargs -0:
git diff --cached --name-only -z --diff-filter=ACMR \ | grep -z -E '\.go$' \ | xargs -0 golangci-lint run --issues-exit-code="$ISSUES_EXIT_CODE" --config .golangci.yml --new-from-rev="$BASE"
94-101: Expose extra args for repo-wide and file-scoped runs.Let callers pass flags (e.g., timeout, build-tags) via GOLANGCI_LINT_ARGS.
- "$GOBIN/golangci-lint" run --issues-exit-code=$ISSUES_EXIT_CODE --config .golangci.yml "$@" + "$GOBIN/golangci-lint" run --issues-exit-code="$ISSUES_EXIT_CODE" --config .golangci.yml ${GOLANGCI_LINT_ARGS:-} "$@" @@ - "$GOBIN/golangci-lint" run --issues-exit-code=$ISSUES_EXIT_CODE --config .golangci.yml + "$GOBIN/golangci-lint" run --issues-exit-code="$ISSUES_EXIT_CODE" --config .golangci.yml ${GOLANGCI_LINT_ARGS:-}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.githooks/pre-commit(1 hunks)Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .githooks/pre-commit
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-admission-test
- GitHub Check: run-hac-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-cli-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-operator-tests
🔇 Additional comments (2)
Makefile (1)
186-194: Centralizing lint flow via the script is correct.Target now consistently routes through the unified runner; aligns with PR goals and removes duplication.
scripts/run-golangci-lint.sh (1)
41-46: Pre-commit: message suggests running ‘make lint’ to install, which builds with local toolchain after changes above.Good UX; just ensure the Makefile target installs via this script (it does).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/run-golangci-lint.sh (2)
19-20: Guard PATH augmentation and fix ShellCheck SC2155.Avoid invoking
go envif Go isn’t installed and split assign/export to satisfy SC2155.- export PATH="$PATH:$(go env GOPATH)/bin" +if command -v go >/dev/null 2>&1; then + GOPATH_BIN="$(go env GOPATH)/bin" + export PATH="$PATH:$GOPATH_BIN" +fi
14-14: Parameterize golangci-lint version, use local toolchain, and rebuild predictably.Current logic installs only when missing, hard-codes
@latest, and doesn’t setGOTOOLCHAIN=local, which can break across branch/toolchain switches. Make version overridable and force a local-toolchain build; rebuild even if the binary exists.readonly ISSUES_EXIT_CODE=42 +: "${GOLANGCI_LINT_VERSION:=latest}" # set to a pinned tag if you decide to lock itinstall_golangci_lint() { - GOBIN=$(go env GOBIN) + GOBIN=$(go env GOBIN) if [ -z "$GOBIN" ]; then GOBIN=$(go env GOPATH)/bin fi - - if [ ! -x "$GOBIN/golangci-lint" ]; then - echo "Installing latest golangci-lint..." - if ! go -a install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest; then - echo "⚠️ Failed to install golangci-lint" - exit 0 - fi - fi + + if [ ! -x "$GOBIN/golangci-lint" ]; then + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + } else { + echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + fi + if ! GOTOOLCHAIN=local go -a install "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}"; then + echo "⚠️ Failed to install golangci-lint" + exit 0 + fi }Follow-up: do you want this pinned (e.g., via
GOLANGCI_LINT_VERSION=v1.xx.x) and rebuilt only when the repo’s Go toolchain is upgraded?Also applies to: 28-35
🧹 Nitpick comments (2)
scripts/run-golangci-lint.sh (2)
47-55: Make staged-file collection robust to odd filenames.Current space-escaping is brittle. Prefer NUL-delimited plumbing with xargs.
- STAGED_FILES=$(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' | sed 's| |\\ |g') + # NUL-delimited staged Go files + STAGED_FILES_NUL=$(git diff --cached --name-only -z --diff-filter=ACMR -- '*.go') + if [ -z "$STAGED_FILES_NUL" ]; then + echo "✅ No staged Go files were found, exiting" + exit 0 + fiAnd invoke golangci-lint with xargs in both branches below:
- golangci-lint run --issues-exit-code=$ISSUES_EXIT_CODE --config .golangci.yml $STAGED_FILES + printf '%s' "$STAGED_FILES_NUL" | xargs -0 golangci-lint run --issues-exit-code=$ISSUES_EXIT_CODE --config .golangci.yml- golangci-lint run --issues-exit-code=$ISSUES_EXIT_CODE --config .golangci.yml --new-from-rev="$BASE" $STAGED_FILES + printf '%s' "$STAGED_FILES_NUL" | xargs -0 golangci-lint run --issues-exit-code=$ISSUES_EXIT_CODE --config .golangci.yml --new-from-rev="$BASE"
86-93: Handle missing Go toolchain in makefile mode.
install_golangci_lintcallsgo envunguarded; if Go isn’t installed, it emits noise. Short-circuit with a warning and non-blocking exit to match policy.run_makefile_lint() { echo "Running golangci-lint in makefile mode..." - - # Install golangci-lint if needed + # Require Go toolchain for Makefile-driven installs + if ! command -v go >/dev/null 2>&1; then + echo "⚠️ Go toolchain not found; skipping lint (make target will not be blocked)" + exit 0 + fi + # Install golangci-lint if needed install_golangci_lint
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/run-golangci-lint.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 19-19: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: run-cli-tests
- GitHub Check: run-hac-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-sa-test
🔇 Additional comments (2)
scripts/run-golangci-lint.sh (2)
41-46: Graceful precommit fallback looks good.Non-blocking exit on missing
golangci-lintaligns with the PR’s exit-code policy.
56-65: Nice: precommit restricts to staged files in both branches.Including
$STAGED_FILESwith--new-from-rev="$BASE"avoids linting unstaged changes. Good fix.
liranmauda
left a comment
There was a problem hiding this comment.
2 general styling comments:
- All variable calls should be wrapped with {}, for example: a=${variable}
- The syntax of
if... ; thennever made sense to me,;means new line, then it is better to do a new line.
liranmauda
left a comment
There was a problem hiding this comment.
When we move the then to a newline, the if does not need ;
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
scripts/run-golangci-lint.sh (5)
21-26: Good guard for PATH augmentation (prevents failures when Go is missing)This addresses the SC2155/“Go not installed” concern raised earlier. LGTM.
65-75: Pre-commit: correctly limits to staged changes vs HEADIncluding both --new-from-rev and the staged file list fixes the “unstaged changes being linted” issue. Well done.
107-109: Makefile mode: support optional file arguments while keeping unified invocationHonor file paths passed after the mode and keep a single run command. This matches the earlier discussion about keeping only the print conditional.
- echo "Running lint on all files" - "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml + if [ "$#" -gt 0 ]; then + echo "Running lint on specified files: $*" + else + echo "Running lint on all files" + fi + "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml "$@"
12-16: Parameterize golangci-lint version (pin-able, overridable)Expose a version knob so teams can pin or override without editing the script.
readonly ISSUES_EXIT_CODE=42 + +# Allow pinning or overriding the golangci-lint version: +# export GOLANGCI_LINT_VERSION='v2.4.0' # example +: "${GOLANGCI_LINT_VERSION:=latest}"
36-41: Install logic: reflect install vs rebuild and use local toolchain + versionAlways rebuild is fine, but make the behavior explicit and wire the version variable.
- echo "Installing the latest golangci-lint with local toolchain" - if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + if [ ! -x "${GOBIN}/golangci-lint" ]; then + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + else + echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + fi + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fi
🧹 Nitpick comments (1)
scripts/run-golangci-lint.sh (1)
16-20: Tighten/clarify the comment next to shiftThe sentence is incomplete; make the intent explicit.
-# Remove the first argument from the list of arguments -# so that we're -shift +# Drop the mode argument; remaining args (if any) are file paths +shift
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-admission-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-hac-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-kmip-test
- GitHub Check: golangci-lint
🔇 Additional comments (2)
scripts/run-golangci-lint.sh (2)
39-41: Confirm non-blocking behavior on installer failure is desired in CIExiting 0 on install failures meets the “don’t block on non-lint errors” goal for local flows, but CI might want a hard fail to avoid silently skipping lint. Verify expected behavior for CI use.
Would you like the script to detect CI (e.g., CI=true) and fail hard there while staying non-blocking locally?
127-140: Overall structure is clean and maintainableClear mode split, consistent exit-code handling, and concise messages. Good consolidation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
scripts/run-golangci-lint.sh (4)
12-16: Parameterize golangci-lint version to support pinning and predictable upgrades.Add an overridable GOLANGCI_LINT_VERSION. This aligns with the team discussion and golangci-lint docs recommending installing a specific version.
readonly ISSUES_EXIT_CODE=42 + +# Overridable; set to a pinned tag (e.g., v1.61.0) when locking the toolchain. +: "${GOLANGCI_LINT_VERSION:=latest}"
25-39: Harden installer: quote vars, ensure GOBIN exists, and use version param.Current flow may fail if GOBIN is unset or the bin dir doesn’t exist; it also hardcodes @latest and message doesn’t reflect rebuilds.
install_golangci_lint() { - GOBIN=$(go env GOBIN) - if [ -z "${GOBIN}" ] - then - GOBIN=$(go env GOPATH)/bin - fi - - echo "Installing the latest golangci-lint with local toolchain" - if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + GOBIN="$(go env GOBIN)" + if [ -z "${GOBIN}" ]; then + GOBIN="$(go env GOPATH)/bin" + fi + mkdir -p "${GOBIN}" + + if [ ! -x "${GOBIN}/golangci-lint" ]; then + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + else + echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + fi + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fi }
91-106: Makefile mode ignores file arguments; support both targeted and full-repo lint.Honor optional filenames passed after the mode, while keeping identical command for both cases; this matches prior discussion and simplifies logic.
- echo "Running lint on all files" - "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml + # Informative message only; the command is the same with or without args. + if [ "$#" -gt 0 ]; then + echo "Running lint on specified files: ${*}" + else + echo "Running lint on all files" + fi + "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml "$@"
65-72: Pre-commit should pass the collected file list as arguments.Propagate "$@" to restrict analysis to staged files in both initial and non-initial commits.
- # Initial commit – lint only staged files - golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml "${STAGED_FILES}" + # Initial commit – lint only staged files + golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml "$@" else # Lint only staged changes vs HEAD - golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml --new-from-rev="${BASE}" "${STAGED_FILES}" + golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml --new-from-rev="${BASE}" "$@" fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/run-golangci-lint.sh(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
- GitHub Check: run-azure-vault-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-kmip-test
🔇 Additional comments (2)
scripts/run-golangci-lint.sh (2)
18-23: PATH augmentation guard looks good.Nicely avoids failing when Go isn’t installed and fixes SC2155.
107-121: Exit-code handling is correct and aligned with PR objectives.42 blocks the flow for lint issues; other non-zero codes degrade to warnings. Good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/run-golangci-lint.sh (2)
14-14: Parameterize golangci-lint version (default to latest) for pin/overrideExpose GOLANGCI_LINT_VERSION to support pinning per branch/toolchain policy and reproducibility, while keeping default behavior.
readonly ISSUES_EXIT_CODE=42 +: "${GOLANGCI_LINT_VERSION:=latest}" # override to pin a specific version
25-39: Use the version variable and improve install loggingUse the overridable version instead of hard-coded latest; message matches action.
install_golangci_lint() { - GOBIN=$(go env GOBIN) + GOBIN="$(go env GOBIN)" if [ -z "${GOBIN}" ] then - GOBIN=$(go env GOPATH)/bin + GOBIN="$(go env GOPATH)/bin" fi - - echo "Installing the latest golangci-lint with local toolchain" - if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fi }
🧹 Nitpick comments (4)
scripts/run-golangci-lint.sh (4)
16-18: Shift off the mode to allow passing file args to makefile/precommit functionsWithout shifting, "$@" still contains the mode ("precommit"/"makefile"), making future file-arg support error-prone. Safe for precommit since it resets "$@" later; needed for makefile to support optional file lists.
MODE="${1}" +shift
102-117: Makefile mode ignores file args; run on provided files when presentAllow callers (e.g., Make targets) to pass specific paths while keeping identical behavior when none are provided. Mirrors prior discussion.
run_makefile_lint() { echo "Running golangci-lint in makefile mode..." @@ - echo "Running lint on all files" - "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml + # If specific files were provided, lint just those; otherwise lint all. + if [ "$#" -gt 0 ]; then + echo "Running lint on specified files: $*" + else + echo "Running lint on all files" + fi + "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml "$@"
52-66: Silence SC2046 intentionally; current newline-only splitting is POSIX-safeYou’ve set IFS to newline and disabled globbing, so unquoted command substitution here is deliberate. Add a shellcheck directive to avoid noise.
- set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + # shellcheck disable=SC2046 # intentional splitting on newlines after setting IFS + set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true)
41-51: Minor: also accept a pre-installed binary without Go presentPrecommit currently bails out if Go isn’t installed, even if golangci-lint is on PATH. Check for golangci-lint first; only warn about Go if needed.
run_precommit_lint() { echo "Running golangci-lint in precommit mode..." - - # Check if golangci-lint is available - exit gracefully if it isn't - if ! command -v golangci-lint >/dev/null 2>&1 - then + # Prefer a pre-installed golangci-lint on PATH; exit gracefully if missing + if ! command -v golangci-lint >/dev/null 2>&1; then echo "⚠️ golangci-lint not found – run 'make lint' to install it automatically" exit 0 fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/run-golangci-lint.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 62-62: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: golangci-lint
- GitHub Check: run-azure-vault-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-hac-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/run-golangci-lint.sh (2)
52-60: Precommit: staged file parsing breaks on paths with spaces; split on newlines and disable globbing.Current set -- $(...) splits on spaces (SC2046) and mangles filenames. Use a POSIX-safe newline split and silence SC2046 with rationale.
Apply:
- set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + # shellcheck disable=SC2046 # we intentionally expand to positional args after setting IFS + OLD_IFS="${IFS}" + IFS=' +' + set -f + set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + set +f + IFS="${OLD_IFS}"
14-14: Parameterize golangci-lint version; keep local toolchain; clearer logs.Expose GOLANGCI_LINT_VERSION (default latest) and use it in install; improves branch/toolchain predictability and aligns with prior discussion.
Apply:
readonly ISSUES_EXIT_CODE=42 +: "${GOLANGCI_LINT_VERSION:=latest}" # override to pin a specific version @@ - echo "Installing the latest golangci-lint with local toolchain" - if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fiAlso applies to: 33-38
🧹 Nitpick comments (1)
scripts/run-golangci-lint.sh (1)
33-35: Minor: log “Installing/Rebuilding” depending on presence.Cosmetic clarity when rerunning make.
Apply:
- echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + if command -v golangci-lint >/dev/null 2>&1; then + echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + else + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + fi
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scripts/run-golangci-lint.sh(1 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 58-58: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-dev-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-operator-tests
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-hac-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-admission-test
- GitHub Check: run-cli-tests
🔇 Additional comments (1)
scripts/run-golangci-lint.sh (1)
80-89: Exit-code policy is solid.Correctly distinguishes lint issues (42) from other failures and blocks only on issues.
Also applies to: 113-121
694bbbf to
0337192
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/run-golangci-lint.sh (2)
29-35: Align message with behavior; set GOTOOLCHAIN=local and parameterize version (repeat).Echo says “local toolchain” but
GOTOOLCHAINisn’t set; also no way to pin/override version. Recommend adding a version var and using it withGOTOOLCHAIN=local. This improves determinism across branch/toolchain switches.-# Exit code used by golangci-lint when linting issues are found -# Allows us to distinguish between linting issues and other errors -readonly ISSUES_EXIT_CODE=42 +# Exit code used by golangci-lint when linting issues are found +# Allows us to distinguish between linting issues and other errors +readonly ISSUES_EXIT_CODE=42 +# Overridable; set to a fixed tag to pin (e.g., v1.60.3) +: "${GOLANGCI_LINT_VERSION:=latest}"install_golangci_lint() { - echo "Installing the latest golangci-lint with local toolchain" - if ! go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fi }
48-59: Fix staged-file collection: avoid word splitting (SC2046) and paths with spaces.
set -- $(...)splits on spaces and breaks filenames; use newline-only splitting with disabled globbing.- set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + OLD_IFS="${IFS}" + IFS=' +' + set -f + # Read staged .go files as positional args, splitting on newlines only + set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + set +f + IFS="${OLD_IFS}"
🧹 Nitpick comments (1)
scripts/run-golangci-lint.sh (1)
97-105: Prefer PATH-resolved binary; simplify invocation.You already exported GOBIN/GOPATH into PATH. Calling the binary via PATH avoids hard-coding install location.
- GOBIN=$(go env GOBIN) - if [ -z "${GOBIN}" ] - then - GOBIN=$(go env GOPATH)/bin - fi - echo "Running lint on all files" - "${GOBIN}/golangci-lint" run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml + golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.githooks/pre-commit(1 hunks)Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- .githooks/pre-commit
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.112Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
📚 Learning: 2025-09-02T10:23:02.112Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.112Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
Applied to files:
scripts/run-golangci-lint.sh
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-core-config-map-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-cli-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-admission-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-hac-test
🔇 Additional comments (1)
scripts/run-golangci-lint.sh (1)
20-25: PATH handling looks good.Guarding on Go presence and appending both GOBIN and GOPATH/bin is correct.
0337192 to
ed1ed56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/run-golangci-lint.sh (2)
28-35: Make toolchain/version policy explicit; align message with behavior.Currently prints “with local toolchain” but doesn’t set GOTOOLCHAIN. Also no way to pin/override version. Parameterize version and force toolchain to avoid branch/toolchain flakiness.
readonly ISSUES_EXIT_CODE=42 +: "${GOLANGCI_LINT_VERSION:=latest}" # set to a tag to pin (e.g., v2.4.0) install_golangci_lint() { - echo "Installing the latest golangci-lint with local toolchain" - if ! go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fi }
54-59: Fix staged-file collection (handles spaces, silences SC2046).Unquoted command substitution splits on spaces and breaks paths; also flagged by ShellCheck SC2046. Split on newlines with globbing disabled.
- set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + OLD_IFS="${IFS}" + IFS=' +' + set -f + set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + set +f + IFS="${OLD_IFS}"
🧹 Nitpick comments (2)
scripts/run-golangci-lint.sh (1)
67-71: Set an explicit timeout to avoid runaway lints.Make it overridable and apply to both modes.
readonly ISSUES_EXIT_CODE=42 +: "${GOLANGCI_LINT_TIMEOUT:=10m}" @@ - golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml "${@}" + golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --timeout="${GOLANGCI_LINT_TIMEOUT}" --config .golangci.yml "${@}" @@ - golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml --new-from-rev="${BASE}" "${@}" + golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --timeout="${GOLANGCI_LINT_TIMEOUT}" --config .golangci.yml --new-from-rev="${BASE}" "${@}" @@ - golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --config .golangci.yml + golangci-lint run --issues-exit-code=${ISSUES_EXIT_CODE} --timeout="${GOLANGCI_LINT_TIMEOUT}" --config .golangci.ymlAlso applies to: 98-98
Makefile (1)
188-188: Avoid duplicate “Running lint on all files” message.The script already prints mode + “Running lint on all files.” Drop the extra echo to reduce noise.
- @echo "Running lint on all files" @./scripts/run-golangci-lint.sh makefile
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.githooks/pre-commit(1 hunks)Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .githooks/pre-commit
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.112Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
📚 Learning: 2025-09-02T10:23:02.112Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.112Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
Applied to files:
Makefilescripts/run-golangci-lint.sh
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-operator-tests
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-admission-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-dev-test
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-kmip-test
🔇 Additional comments (2)
scripts/run-golangci-lint.sh (1)
20-25: PATH augmentation looks good.Adding both GOBIN and GOPATH/bin conditionally is correct and avoids breaking pre-commit when Go is absent.
Makefile (1)
188-189: LGTM: Makefile delegates linting to the unified script.This removes duplication and centralizes install/exit-code semantics as intended.
|
@coderabbitai - Note that the timeout is set in |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 330 @Neon-White Thank you for pointing that out! I can confirm that the timeout is indeed configured in ✏️ Learnings added
🧠 Learnings used |
ed1ed56 to
a846cf6
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scripts/run-golangci-lint.sh (1)
12-16: Parameterize golangci-lint version and build with local toolchain.Avoid branch/toolchain breakages and make versioning explicit; also align log text with behavior.
Apply:
readonly ISSUES_EXIT_CODE=42 + +: "${GOLANGCI_LINT_VERSION:=latest}" # override to pin (e.g., v1.60.3) @@ install_golangci_lint() { - echo "Installing the latest golangci-lint with local toolchain" - if ! go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" - then - echo "⚠️ Failed to install golangci-lint" - exit 0 - fi + if command -v go >/dev/null 2>&1; then + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}"; then + echo "⚠️ Failed to install golangci-lint" >&2 + exit 0 + fi + else + echo "⚠️ Go toolchain not found; skipping golangci-lint install" >&2 + exit 0 + fi }Also applies to: 27-35
🧹 Nitpick comments (3)
scripts/run-golangci-lint.sh (2)
41-46: Send warnings/errors to stderr.Non-blocking warnings should go to stderr to avoid polluting normal output streams.
Apply:
- echo "⚠️ golangci-lint not found – run 'make lint' to install it automatically" + echo "⚠️ golangci-lint not found – run 'make lint' to install it automatically" >&2 @@ - echo "⚠️ golangci-lint encountered an error (exit code: ${LINT_EXIT_CODE})" + echo "⚠️ golangci-lint encountered an error (exit code: ${LINT_EXIT_CODE})" >&2 @@ - echo "⚠️ golangci-lint encountered an error (exit code: ${LINT_EXIT_CODE})" + echo "⚠️ golangci-lint encountered an error (exit code: ${LINT_EXIT_CODE})" >&2Also applies to: 81-85, 109-112
94-101: Optional: skip reinstall when correct version is already present.To avoid network hits on every make, detect version and short-circuit.
Apply:
run_makefile_lint() { @@ - # Install golangci-lint if needed - install_golangci_lint + # Install golangci-lint if needed or version mismatch + if command -v golangci-lint >/dev/null 2>&1; then + if ! golangci-lint version 2>/dev/null | grep -q "version ${GOLANGCI_LINT_VERSION}"; then + install_golangci_lint + fi + else + install_golangci_lint + fi.githooks/pre-commit (1)
10-11: Trim trailing whitespace.Minor cleanup.
Apply:
-exec "$PROJECT_ROOT/scripts/run-golangci-lint.sh" precommit +exec "$PROJECT_ROOT/scripts/run-golangci-lint.sh" precommit
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
.githooks/pre-commit(1 hunks)Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Makefile
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.112Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
📚 Learning: 2025-09-02T10:23:02.112Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.112Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
Applied to files:
.githooks/pre-commitscripts/run-golangci-lint.sh
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: cnpg-deployment-test
- GitHub Check: golangci-lint
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: run-kms-dev-test
- GitHub Check: run-hac-test
- GitHub Check: run-kms-tls-token-test
🔇 Additional comments (2)
scripts/run-golangci-lint.sh (1)
97-99: LGTM: always lint all files in Makefile mode.Matches the stated design choice to remove file-list support and keep the script as the single source of truth.
.githooks/pre-commit (1)
6-11: LGTM: delegate to unified script.Simple, robust hook; path resolution is correct.
| # Collect staged Go files as positional args | ||
| # cached: retrieve only staged files | ||
| # name-only: retrieve only the names of the files | ||
| # diff-filter=ACMR: retrieve only the files that have been added, copied, modified, or renamed | ||
| # grep -E '\.go$': retrieve only the files that end with .go | ||
| # || true: if there are no staged Go files, return a zero-status exit code | ||
| set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) | ||
| # If there are no staged Go files, exit successfully | ||
| if [ "$#" -eq 0 ]; then |
There was a problem hiding this comment.
Fix staged-file parsing (breaks on spaces) and drop grep with Git pathspec.
Current approach splits on spaces and triggers SC2046. Preserve spaces by splitting on newlines and let Git filter “.go” files.
Apply:
- set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true)
+ # Collect staged Go files as positional args, preserving spaces (split on newlines)
+ OLD_IFS="${IFS}"
+ IFS='
+'
+ set -f
+ # shellcheck disable=SC2046
+ set -- $(git diff --cached --name-only --diff-filter=ACMR -- '*.go' || true)
+ set +f
+ IFS="${OLD_IFS}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Collect staged Go files as positional args | |
| # cached: retrieve only staged files | |
| # name-only: retrieve only the names of the files | |
| # diff-filter=ACMR: retrieve only the files that have been added, copied, modified, or renamed | |
| # grep -E '\.go$': retrieve only the files that end with .go | |
| # || true: if there are no staged Go files, return a zero-status exit code | |
| set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) | |
| # If there are no staged Go files, exit successfully | |
| if [ "$#" -eq 0 ]; then | |
| # Collect staged Go files as positional args | |
| # cached: retrieve only staged files | |
| # name-only: retrieve only the names of the files | |
| # diff-filter=ACMR: retrieve only the files that have been added, copied, modified, or renamed | |
| # || true: if there are no staged Go files, return a zero-status exit code | |
| OLD_IFS="${IFS}" | |
| IFS=' | |
| ' | |
| set -f | |
| # shellcheck disable=SC2046 | |
| set -- $(git diff --cached --name-only --diff-filter=ACMR -- '*.go' || true) | |
| set +f | |
| IFS="${OLD_IFS}" | |
| # If there are no staged Go files, exit successfully | |
| if [ "$#" -eq 0 ]; then |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In scripts/run-golangci-lint.sh around lines 48-56, the current use of command
substitution with set -- $(git ... | grep ... || true) splits filenames on
whitespace and triggers SC2046; replace it by letting Git apply the .go pathspec
(git ... -- '*.go') and read results into an array without word-splitting (e.g.,
use readarray/read - or mapfile to capture lines into an array, then set
positional parameters from that array) so filenames with spaces are preserved
and grep is removed; ensure the fallback when there are no files still results
in zero params so the subsequent if [ "$#" -eq 0 ] check behaves the same.
There was a problem hiding this comment.
@coderabbitai There is no value in this, there are no, and there will never be files with spaces in the name
There was a problem hiding this comment.
@liranmauda You're absolutely right - if the repository doesn't have and won't have Go files with spaces in their names, then the additional complexity isn't justified. I understand your position and will note this for future reference.
✏️ Learnings added
Learnt from: liranmauda
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:48-56
Timestamp: 2025-09-07T07:50:45.540Z
Learning: In the noobaa-operator repository, the maintainer liranmauda has confirmed that Go files with spaces in their names are not present and will never be allowed. Shellcheck warnings about word splitting in filename handling can be ignored when they relate to this edge case, as the added complexity for handling spaces is not justified.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.154Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
Signed-off-by: Ben <belimele@redhat.com>
a846cf6 to
7d37750
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
scripts/run-golangci-lint.sh (1)
48-56: Keep the simple staged-file parsing; optionally drop grep and silence SC2046.Given the repo policy “no spaces in Go filenames,” the current
set -- $(git … | grep …)is acceptable. To trim a process and quiet ShellCheck without adding complexity:Apply:
- # grep -E '\.go$': retrieve only the files that end with .go - # || true: if there are no staged Go files, return a zero-status exit code - set -- $(git diff --cached --name-only --diff-filter=ACMR | grep -E '\.go$' || true) + # Use Git pathspec instead of grep; SC2046 acceptable per repo policy + # shellcheck disable=SC2046 + set -- $(git diff --cached --name-only --diff-filter=ACMR -- '*.go' || true)
🧹 Nitpick comments (2)
scripts/run-golangci-lint.sh (2)
29-35: Pin/parametrize the linter version and actually use the local toolchain.The log says “with local toolchain” but
GOTOOLCHAIN=localisn’t set. Also, hard‑coding@latestmakes branch/toolchain switches brittle. Parameterize the version and honor the local toolchain for deterministic builds.Apply:
readonly ISSUES_EXIT_CODE=42 +: "${GOLANGCI_LINT_VERSION:=latest}" # override to pin (e.g., v2.4.0) install_golangci_lint() { - echo "Installing the latest golangci-lint with local toolchain" - if ! go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@latest" + GOBIN="$(go env GOBIN)"; [ -n "${GOBIN}" ] || GOBIN="$(go env GOPATH)/bin" + if [ -x "${GOBIN}/golangci-lint" ]; then + echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + else + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" + fi + if ! GOTOOLCHAIN=local go install -a "github.com/golangci/golangci-lint/v2/cmd/golangci-lint@${GOLANGCI_LINT_VERSION}" then echo "⚠️ Failed to install golangci-lint" exit 0 fi }Would you like this defaulted to a pinned tag to match the repo’s Go toolchain policy?
Also applies to: 12-16
90-99: Optional: avoid redundant rebuilds unless requested.Rebuilding every
make lintcan be slow. Consider skipping reinstall if the binary exists, with an override to force rebuild.Apply on top of the earlier version‑param change:
install_golangci_lint() { - GOBIN="$(go env GOBIN)"; [ -n "${GOBIN}" ] || GOBIN="$(go env GOPATH)/bin" - if [ -x "${GOBIN}/golangci-lint" ]; then - echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" - else - echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" - fi + GOBIN="$(go env GOBIN)"; [ -n "${GOBIN}" ] || GOBIN="$(go env GOPATH)/bin" + if [ -x "${GOBIN}/golangci-lint" ] && [ -z "${GOLANGCI_LINT_FORCE_REBUILD}" ]; then + echo "golangci-lint already present; set GOLANGCI_LINT_FORCE_REBUILD=1 to force rebuild" + return 0 + fi + [ -x "${GOBIN}/golangci-lint" ] && echo "Rebuilding golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain" || \ + echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION} with local toolchain"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.githooks/pre-commit(1 hunks)Makefile(1 hunks)scripts/run-golangci-lint.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- .githooks/pre-commit
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.154Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
📚 Learning: 2025-09-02T10:23:02.154Z
Learnt from: Neon-White
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:16-16
Timestamp: 2025-09-02T10:23:02.154Z
Learning: In the noobaa-operator repository's run-golangci-lint.sh script, file list support was intentionally removed as part of the golangci-lint refactoring. The script now always runs lint on all files rather than supporting file-specific linting, which is the intended behavior.
Applied to files:
scripts/run-golangci-lint.sh
📚 Learning: 2025-09-07T07:50:45.559Z
Learnt from: liranmauda
PR: noobaa/noobaa-operator#1687
File: scripts/run-golangci-lint.sh:48-56
Timestamp: 2025-09-07T07:50:45.559Z
Learning: In the noobaa-operator repository, the maintainer liranmauda has confirmed that Go files with spaces in their names are not present and will never be allowed. Shellcheck warnings about word splitting in filename handling can be ignored when they relate to this edge case, as the added complexity for handling spaces is not justified.
Applied to files:
scripts/run-golangci-lint.sh
🪛 Shellcheck (0.10.0)
scripts/run-golangci-lint.sh
[warning] 54-54: Quote this to prevent word splitting.
(SC2046)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: cnpg-deployment-test
- GitHub Check: run-core-config-map-tests
- GitHub Check: golangci-lint
- GitHub Check: run-kms-tls-token-test
- GitHub Check: run-azure-vault-test
- GitHub Check: run-kms-dev-test
- GitHub Check: run-admission-test
- GitHub Check: run-operator-tests
- GitHub Check: run-kms-tls-sa-test
- GitHub Check: run-kms-kmip-test
- GitHub Check: run-cli-tests
- GitHub Check: run-kms-key-rotate-test
- GitHub Check: run-hac-test
🔇 Additional comments (3)
scripts/run-golangci-lint.sh (3)
76-85: Exit-code semantics align with PR goals.Non‑lint errors are downgraded to warnings (exit 0), and only real lint issues block (exit 1 with ISSUES_EXIT_CODE). Matches the stated behavior.
Confirm CI jobs depending on lint should still fail on ISSUES_EXIT_CODE=42 (they will), and tolerate non‑lint failures as warnings.
Also applies to: 103-112
20-25: PATH handling for GOBIN/GOPATH looks good.Guards on Go presence and includes both GOBIN and GOPATH/bin. This should resolve discovery issues across environments.
64-71: Pre-commit diffs limited to staged changes – good fix.Combining
--new-from-rev="$BASE"with the staged file list ensures unstaged changes don’t block commits.
Explain the changes
golandci-lintwere moved fromMakefileandpre-committo a unified script (run-golangci-lint.sh) to reduce bloat in external files, and also allow a central maintenance spot (and source of truth) for anything related to the lintermakecommands in case of linting problems in reviewed filesSummary by CodeRabbit
Chores
Refactor
Note