NO-JIRA: add golangci-lint#835
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@simonpasquier: This pull request explicitly references no jira issue. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/cc @jgbernalp |
openshift-ci
bot
requested review from
PeterYurkovich,
etmurasaki,
jan--f and
jgbernalp
WalkthroughIntroduces GolangCI-Lint configuration and integrates it into the build pipeline via Makefile, with supporting configuration for linting rules. Includes minor code refactoring: import reordering across multiple files and removal of an internal error handling helper function. No functional changes to runtime behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Around line 95-98: Remove the destructive "go mod tidy" from the lint-backend
target so lint-backend (and any targets depending on it) remain read-only;
instead create a separate target (e.g., check-modules or verify-modules) that
runs "go mod tidy" followed by a check like "git diff --exit-code" to fail if
tidy would modify go.mod/go.sum, and update any higher-level targets that need
module validation (e.g., deploy) to depend on that new verify-modules target
rather than lint-backend; look for the lint-backend target and the golangci-lint
symbol to apply this change.
- Around line 87-93: The golangci-lint Makefile target currently only checks for
the binary file and uses the moving master installer URL; update the
golangci-lint target (and references to GOLANGCI_LINT and GOLANGCI_LINT_VERSION)
to verify the installed binary's version (e.g., run '$(GOLANGCI_LINT) --version'
and compare to '$(GOLANGCI_LINT_VERSION)') and if missing or mismatched
remove/reinstall, and switch the installer URL to the official pinned endpoint
'https://golangci-lint.run/install.sh' while passing the pinned
'$(GOLANGCI_LINT_VERSION)' to the installer so changing GOLANGCI_LINT_VERSION
triggers a reinstall.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 8a86dab6-7112-44e3-979c-62d662a361dd
📒 Files selected for processing (6)
.gitignore.golangci-lint.yamlMakefilecmd/plugin-backend.gopkg/proxy/proxy.gopkg/server.go
💤 Files with no reviewable changes (1)
- pkg/proxy/proxy.go
| # Download and install golangci-lint if not already installed | ||
| .PHONY: golangci-lint | ||
| golangci-lint: | ||
| @[ -f $(GOLANGCI_LINT) ] || { \ | ||
| set -e ;\ | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) $(GOLANGCI_LINT_VERSION) ;\ | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
golangci-lint install.sh official documentation recommended URL
💡 Result:
The golangci-lint docs recommend using the install script hosted at:
https://golangci-lint.run/install.shThis recommendation appears in the official Local Installation → Binaries section (with example curl ... | sh -s ...). 1
Official install documentation page: 2
Citations:
- 1: https://golangci-lint.run/docs/welcome/install/local/?utm_source=openai
- 2: https://golangci-lint.run/docs/welcome/install/?utm_source=openai
🌐 Web query:
golangci-lint version checking best practices
💡 Result:
Best practices for golangci-lint version checking (and avoiding “works on my machine”)
-
Pin a specific golangci-lint version in CI (avoid
latest)
The golangci-lint docs explicitly recommend installing a specific release for reproducible CI, because new/updated linters can suddenly break builds. [1] -
Use an install method that makes the version explicit
- Install script / binary: pass an explicit tag (e.g.,
v2.11.3) and then you can sanity-check withgolangci-lint --version. [2] - Docker: use a tagged image (e.g.,
golangci/golangci-lint:v2.11.3). [2]
- Install script / binary: pass an explicit tag (e.g.,
-
If you use GitHub Actions, pin both:
- the action major (e.g.,
golangci/golangci-lint-action@v9) - and the linter version via
version:orversion-file:(e.g.,.golangci-lint-version/.tool-versions). [3]
Note: the action acceptsv2.3(minor) and will pick the latest patch for that minor; for maximum reproducibility, prefer pinning the full patch (e.g.,v2.11.3) rather than onlyv2.11. [3]
- the action major (e.g.,
-
Version-control your config format and validate it in CI
- Add
version: "2"at the top of.golangci.ymlto lock the config format. [4] - Keep config validation enabled (the action’s
verifyis on by default, and validation depends on the golangci-lint version you run). [3][4]
- Add
-
Add a lightweight “version gate” for developers
- Put the expected version in a file (e.g.,
.golangci-lint-version) and havemake lint(or a pre-commit hook) comparegolangci-lint --versionto that value; fail fast with a clear message. (The action already supports reading such a file in CI.) [3]
- Put the expected version in a file (e.g.,
Sources
[1] Golangci-lint CI installation docs (reproducible CI; recommend pinning a specific release) (golangci-lint.run)
[2] Golangci-lint local installation docs (install script with explicit tag; docker tags; --version) (golangci-lint.run)
[3] golangci-lint GitHub Action README (inputs version, version-file, behavior, config verification) (github.com)
[4] Golangci-lint configuration file docs (version: "2" is the only valid config version) (golangci-lint.run)
Citations:
- 1: https://golangci-lint.run/docs/welcome/install/ci
- 2: https://golangci-lint.run/docs/welcome/install/local/
- 3: https://github.com/golangci/golangci-lint-action
- 4: https://golangci-lint.run/docs/configuration/file/
🌐 Web query:
site:golangci-lint.run install.sh
💡 Result:
To install golangci-lint via the hosted install.sh script:
# Installs to $(go env GOPATH)/bin
curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b "$(go env GOPATH)/bin" v2.9.0
# Or install into ./bin (default)
curl -sSfL https://golangci-lint.run/install.sh | sh -s v2.9.0
# Alpine (no curl by default)
wget -O- -nv https://golangci-lint.run/install.sh | sh -s v2.9.0Verify:
golangci-lint --version[1]
🏁 Script executed:
# First, find and examine the Makefile to understand GOLANGCI_LINT variable
fd -type f -name "Makefile" | head -5Repository: openshift/monitoring-plugin
Length of output: 241
🏁 Script executed:
# Check what golangci-lint --version outputs (simulate based on known behavior)
# Also verify the syntax: is it "version" or "--version"?
web_search_needed="golangci-lint --version output format"
echo "Need to verify: golangci-lint version output format and flag (--version vs version)"Repository: openshift/monitoring-plugin
Length of output: 154
🏁 Script executed:
# Look at the Makefile to see variable definitions
cat Makefile | head -100Repository: openshift/monitoring-plugin
Length of output: 2470
🏁 Script executed:
# Verify the exact golangci-lint command syntax for version checking
# Check web documentation or common usage
echo "Checking golangci-lint command syntax..."Repository: openshift/monitoring-plugin
Length of output: 111
🌐 Web query:
golangci-lint version flag syntax --version vs version command
💡 Result:
In current golangci-lint (v2.x) you can get the version in two ways:
- Flag (quick check):
golangci-lint --version(global flag; “Print version”). [1] - Command (more control):
golangci-lint version(supports--short,--json,--debug). [1]
Note: golangci-lint custom --version <ver> is a different flag meaning “which golangci-lint version to use to build the custom binary,” not “print my version.” [1]
Sources: [1] GolangCI-Lint CLI docs, [2] GolangCI-Lint local install docs (shows golangci-lint --version).
Make the golangci-lint bootstrap version-aware and use the official installer URL.
The binary existence check on line 90 only verifies file presence, so bumping GOLANGCI_LINT_VERSION will not refresh cached installs. Line 92 fetches the installer from the repository's moving master branch; the official golangci-lint documentation recommends https://golangci-lint.run/install.sh and explicitly advises pinning a specific release for reproducible builds.
Suggested change
golangci-lint:
- @[ -f $(GOLANGCI_LINT) ] || { \
+ @$(GOLANGCI_LINT) version 2>/dev/null | grep -q '$(GOLANGCI_LINT_VERSION)' || { \
set -e ;\
- curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) $(GOLANGCI_LINT_VERSION) ;\
+ curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) $(GOLANGCI_LINT_VERSION) ;\
}📝 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.
| # Download and install golangci-lint if not already installed | |
| .PHONY: golangci-lint | |
| golangci-lint: | |
| @[ -f $(GOLANGCI_LINT) ] || { \ | |
| set -e ;\ | |
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) $(GOLANGCI_LINT_VERSION) ;\ | |
| } | |
| # Download and install golangci-lint if not already installed | |
| .PHONY: golangci-lint | |
| golangci-lint: | |
| @$(GOLANGCI_LINT) version 2>/dev/null | grep -q '$(GOLANGCI_LINT_VERSION)' || { \ | |
| set -e ;\ | |
| curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b $(shell dirname $(GOLANGCI_LINT)) $(GOLANGCI_LINT_VERSION) ;\ | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 87 - 93, The golangci-lint Makefile target currently
only checks for the binary file and uses the moving master installer URL; update
the golangci-lint target (and references to GOLANGCI_LINT and
GOLANGCI_LINT_VERSION) to verify the installed binary's version (e.g., run
'$(GOLANGCI_LINT) --version' and compare to '$(GOLANGCI_LINT_VERSION)') and if
missing or mismatched remove/reinstall, and switch the installer URL to the
official pinned endpoint 'https://golangci-lint.run/install.sh' while passing
the pinned '$(GOLANGCI_LINT_VERSION)' to the installer so changing
GOLANGCI_LINT_VERSION triggers a reinstall.
| .PHONY: lint-backend | ||
| lint-backend: golangci-lint | ||
| go mod tidy | ||
| $(GOLANGCI_LINT) -c $(shell pwd)/.golangci-lint.yaml run --verbose |
There was a problem hiding this comment.
Keep lint-backend read-only.
go mod tidy rewrites go.mod and go.sum by adding missing requirements and removing unused ones, so make lint-backend — and now make deploy via Line 78 — can mutate the worktree as a side effect of linting. Keep linting as analysis-only and validate module hygiene in a separate target. (go.dev)
🔧 Suggested change
.PHONY: lint-backend
lint-backend: golangci-lint
- go mod tidy
$(GOLANGCI_LINT) -c $(shell pwd)/.golangci-lint.yaml run --verbose📝 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.
| .PHONY: lint-backend | |
| lint-backend: golangci-lint | |
| go mod tidy | |
| $(GOLANGCI_LINT) -c $(shell pwd)/.golangci-lint.yaml run --verbose | |
| .PHONY: lint-backend | |
| lint-backend: golangci-lint | |
| $(GOLANGCI_LINT) -c $(shell pwd)/.golangci-lint.yaml run --verbose |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 95 - 98, Remove the destructive "go mod tidy" from the
lint-backend target so lint-backend (and any targets depending on it) remain
read-only; instead create a separate target (e.g., check-modules or
verify-modules) that runs "go mod tidy" followed by a check like "git diff
--exit-code" to fail if tidy would modify go.mod/go.sum, and update any
higher-level targets that need module validation (e.g., deploy) to depend on
that new verify-modules target rather than lint-backend; look for the
lint-backend target and the golangci-lint symbol to apply this change.
There was a problem hiding this comment.
@simonpasquier this seems legitime. Should we just use the golangci-lint?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
the goal of running go mod tidy is indeed to ensure that the go.mod file is up-to-date.
openshift/release#76147 will check that after running make lint-backend , there's no uncommitted changes.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgbernalp, simonpasquier 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-ci
bot
added
the
approved
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@simonpasquier: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
See also openshift/release#76147
Summary by CodeRabbit
Chores
Refactor