Skip to content

Conversation

@pkazmierczak
Copy link

This PR sets Go mod version for the package to 1.24 and retracts package v2.1.4 which set an unnecessarily high requirement for Go to be 1.25.1.

@pkazmierczak pkazmierczak changed the title build(deps: set go.mod to 1.24 and retract v2.1.4 build(deps): set go.mod to 1.24 and retract v2.1.4 Sep 23, 2025
@pkazmierczak pkazmierczak marked this pull request as ready for review September 23, 2025 15:53
@pkazmierczak pkazmierczak requested review from a team as code owners September 23, 2025 15:53
.go-version Outdated
@@ -1 +1 @@
1.25.1
1.24.0
Copy link
Member

Choose a reason for hiding this comment

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

Should we be using the oldest 1.24 or the current 1.24 here?

Copy link
Author

Choose a reason for hiding this comment

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

These need to match either the go directive or the toolchain directive in the mod file. Otherwise linter will protest that we need to tidy.

Copy link
Author

Choose a reason for hiding this comment

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

all this is to say: we should be setting go in go.mod and .go-version here to the same exact version. Or have a toolchain directive in the mod file.

Copy link
Member

Choose a reason for hiding this comment

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

What I'm concerned about here is that there's going to be a CVE in Go and someone is going to end up rev'ing the minimum version again instead of fixing the toolchain itself. So while I think the linter is frankly just wrong, if we need to set a toolchain directive to shut it up, that seems fine to me.

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM

@pkazmierczak pkazmierczak merged commit 60665ea into main Sep 24, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants