Skip to content

Fix lint failing under Go 1.27 #1335

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
BobDu wants to merge 1 commit into
base: main
Choose a base branch
from
+40 −30
Open

Fix lint failing under Go 1.27 #1335

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 37 additions & 22 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,32 +1,47 @@
run:
timeout: 10m

version: "2"
linters:
disable-all: true
default: none
enable:
- dupl
- errorlint
- gofmt
- goimports
- gomodguard
- gosimple
- gomodguard_v2
- govet
- ineffassign
- misspell
- nakedret
- revive
- unused
- staticcheck

linters-settings:
gofmt:
simplify: true
gomodguard:
blocked:
modules:
- github.com/pkg/errors:
recommendations:
- errors
- fmt
dupl:
threshold: 400
- unused
settings:
dupl:
threshold: 400
gomodguard_v2:
blocked:
- module: github.com/pkg/errors
recommendations:
- errors
- fmt
exclusions:
generated: lax
presets:
- comments
- common-false-positives
- legacy
- std-error-handling
paths:
- third_party$
- builtin$
- examples$
formatters:
enable:
- gofmt
- goimports
settings:
gofmt:
simplify: true
exclusions:
generated: lax
paths:
- third_party$
- builtin$
- examples$
9 changes: 2 additions & 7 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ FIGURE_FILES := \
img/media-types.png

MARKDOWN_LINT_VER?=v0.8.1
GOLANGCI_LINT_VER?=v2.13.0

TOOLS := gitvalidation

Expand Down Expand Up @@ -118,13 +119,7 @@ install.tools: $(TOOLS:%=.install.%)

.PHONY: .install.lint
.install.lint:
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;; \
Comment on lines -121 to -125

@sudo-bmitch sudo-bmitch Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't agree with dropping support for all previous versions of Go.

@BobDu BobDu Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

*) 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)

@tianon tianon Aug 20, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curl | sh is a definite blocker -- at least with go install we've got the benefit of the GOPROXY checking checksums for us

@BobDu BobDu Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the recommended installation method golangci-lint documents, and it says:

Using go install/go get, "tools pattern", and tool command/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.

@tianon tianon Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@tianon tianon Aug 21, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe something like this?

Suggested change
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)

@BobDu BobDu Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@BobDu BobDu Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@BobDu BobDu Aug 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

honestly baking this into the Makefile itself is not a choice I would've made

+1

@BobDu BobDu Aug 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


.PHONY: .install.gitvalidation
.install.gitvalidation:
Expand Down
2 changes: 1 addition & 1 deletion schema/backwards_compatibility_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ var compatMap = map[string]string{
func convertFormats(input string) string {
out := input
for k, v := range compatMap {
out = strings.Replace(out, k, v, -1)
out = strings.ReplaceAll(out, k, v)
}
return out
}
Expand Down
Loading