Fix lint failing under Go 1.27 - #1335
Conversation
Signed-off-by: BobDu <i@bobdu.cc>
| go1.21.*) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.59.1;; \ | ||
| *) go install github.com/golangci/golangci-lint/cmd/golangci-lint@latest;; \ | ||
| esac | ||
| curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b $(GOPATH)/bin $(GOLANGCI_LINT_VER) |
There was a problem hiding this comment.
curl | sh is a definite blocker -- at least with go install we've got the benefit of the GOPROXY checking checksums for us
There was a problem hiding this comment.
This is the recommended installation method golangci-lint documents, and it says:
Using
go install/go get, "tools pattern", andtoolcommand/directives installations aren't guaranteed to work.
We recommend using binary installation.
https://golangci-lint.run/docs/welcome/install/local/
The version is pinned as GOLANGCI_LINT_VER and passed to the script as an argument, and the verification happens inside the script: it downloads the tarball and the checksums file from the same release tag and compares sha256 before unpacking.
If you are concerned about a supply chain attack on the script hosted at golangci-lint.run, maybe we could vendor the script locally? I am not sure that is needed.
There was a problem hiding this comment.
Vendoring the script is way more than it's worth, IMO -- I'd be more comfortable ditching golangci-lint entirely (or replicating all the types of checks it does by hand) than curl | sh
The amount of Go code we actually have here is tiny -- can we enumerate what it is we're actually getting from golangci-lint? I'm not certain, but I imagine that we get more value out of aggressively supporting a wide swath of Go versions than we do out of golangci-lint, so if it means we have to drop support for Go versions, I'd rather see us drop it.
There was a problem hiding this comment.
Maybe something like this?
| curl -sSfL https://golangci-lint.run/install.sh | sh -s -- -b $(GOPATH)/bin $(GOLANGCI_LINT_VER) | |
| wget -O golangci-lint.tar.gz 'https://github.com/golangci/golangci-lint/releases/download/v2.13.1/golangci-lint-2.13.1-linux-amd64.tar.gz' | |
| echo 'b17bfbc9d4aaa48be7f4f1ce3240bc3d8200c870c072bacf15c26219e2cfb9cc *golangci-lint.tar.gz' | sha256sum --strict --check - | |
| mkdir -p "$$(go env GOPATH)/bin" | |
| tar -xvf golangci-lint.tar.gz --strip-components=1 --directory "$$(go env GOPATH)/bin" --wildcards '*/golangci-lint' | |
| rm golangci-lint.tar.gz | |
| golangci-lint --version |
(honestly baking this into the Makefile itself is not a choice I would've made, but we are where we are -- IMO we should git rm Makefile entirely because it causes more harm than good)
There was a problem hiding this comment.
so if it means we have to drop support for Go versions, I'd rather see us drop it.
There is no trade-off to make here: we do not have to drop support for any Go version. The latest golangci-lint is compatible with older go.mod versions.
There was a problem hiding this comment.
Vendoring the script is way more than it's worth
There is a third option besides curl | sh and vendoring the script, and it is the one golangci-lint recommends the most for GitHub projects: their dedicated GitHub Action.
We recommend using our GitHub Action for running golangci-lint in CI for GitHub projects.
https://golangci-lint.run/docs/welcome/install/ci/
The trade-off is that lint would then only run in actions, make lint-go would no longer work locally.
There was a problem hiding this comment.
honestly baking this into the Makefile itself is not a choice I would've made
+1
There was a problem hiding this comment.
IMO we should git rm Makefile entirely because it causes more harm than good
How about we start by migrating the lint part from the makefile to native actions?
Or do you want to migrate all makefile targets at once?
| case "$$(go env GOVERSION)" in \ | ||
| go1.18.*) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.47.3;; \ | ||
| go1.19.*) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.54.1;; \ | ||
| go1.20.*) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.55.2;; \ | ||
| go1.21.*) go install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.59.1;; \ |
There was a problem hiding this comment.
I don't agree with dropping support for all previous versions of Go.
There was a problem hiding this comment.
Nothing is dropped, go.mod still declares 1.18, the CI matrix is unchanged, and the go1.18 job is still running.
The only change is that we no longer compile golangci-lint on the fly with the Go environment of the current CI job, and use the prebuilt binary that upstream provides instead.
golangci-lint v1 can no longer analyse under go1.27, so we had to migrate to v2.
The golangci-lint v1 that is installed today fails under Go 1.27. This updates it to v2 and converts the configuration with
golangci-lint migrate.It also moves gomodguard to
gomodguard_v2and fixes the staticcheck issue that the newer version reports.