Upgrade golangci-lint in CI runner and Makefile #4861
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| GOLANGCI_LINT_VERSION="v2.11.4" | ||
|
|
||
| # TODO: Re-enable errcheck and staticcheck once pre-existing issues are resolved. | ||
| LINT_ARGS="--disable errcheck,staticcheck --enable bodyclose,copyloopvar,misspell --timeout 10m" | ||
|
|
||
| GOBIN="$(go env GOPATH)/bin" | ||
| GOLANGCI_LINT="${GOBIN}/golangci-lint" | ||
|
|
||
| # Install the required version if missing or mismatched. | ||
| if [[ -x "${GOLANGCI_LINT}" ]] && "${GOLANGCI_LINT}" version 2>&1 | grep -q "${GOLANGCI_LINT_VERSION#v}"; then | ||
|
There was a problem hiding this comment. Unanchored version grep may match wrong versionsLow Severity The version check uses  
Contributor
Author
There was a problem hiding this comment. Patch updates are a non-issue I think
Contributor
There was a problem hiding this comment. I dug into this a little and added a comment below
Contributor
There was a problem hiding this comment. The version check uses Note that the commonly suggested A safer approach is to extract and compare the version exactly: installed_version=$("${GOLANGCI_LINT}" version 2>&1 | grep -oE '[0-9]+\.[0-9]+\.[0-9]+' | head -1)
if [[ "${installed_version}" == "${GOLANGCI_LINT_VERSION#v}" ]]; then
echo "golangci-lint ${GOLANGCI_LINT_VERSION} found"
else
echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION}..."
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b "${GOBIN}" "${GOLANGCI_LINT_VERSION}"
fi |
||
| echo "golangci-lint ${GOLANGCI_LINT_VERSION} found" | ||
| else | ||
| echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION}..." | ||
| go install github.com/golangci/golangci-lint/v2/cmd/golangci-lint@"${GOLANGCI_LINT_VERSION}" | ||
cursor[bot] marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| fi | ||
|
Comment on lines
+9
to
+18
Contributor
There was a problem hiding this comment. The script only looks for Consider checking if command -v golangci-lint &>/dev/null && golangci-lint version 2>&1 | grep -q "${GOLANGCI_LINT_VERSION#v}"; then
GOLANGCI_LINT="$(command -v golangci-lint)"
echo "golangci-lint ${GOLANGCI_LINT_VERSION} found at ${GOLANGCI_LINT}"
elif [[ -x "${GOLANGCI_LINT}" ]] && "${GOLANGCI_LINT}" version 2>&1 | grep -q "${GOLANGCI_LINT_VERSION#v}"; then
echo "golangci-lint ${GOLANGCI_LINT_VERSION} found at ${GOLANGCI_LINT}"
else
echo "Installing golangci-lint ${GOLANGCI_LINT_VERSION}..."
curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b "${GOBIN}" "${GOLANGCI_LINT_VERSION}"
fiThis preserves the version-pinning guarantee while respecting existing installations. |
||
|
|
||
| # shellcheck disable=SC2086 | ||
| "${GOLANGCI_LINT}" run ${LINT_ARGS} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabling default linters errcheck and staticcheck reduces coverage
Medium Severity
The new
LINT_ARGSadds--disable errcheck,staticcheck, which was not present in the old configuration. Previously, both CI and Makefile only used--enableflags on top of defaults, meaningerrcheck(unchecked error returns) andstaticcheck(comprehensive static analysis, now includinggosimpleandstylecheckin v2) were actively running. The codebase even has existingnolint:errcheckandnolint:staticcheckcomments proving these linters were in use. Silently disabling two core default linters significantly weakens lint coverage for a PR described only as an "upgrade."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't decided on this yet
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can leave a TODO comment and address this in a follow up PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, agreed. Let's create a ticket for this so we can make sure we re-enable these with v2.