-
Notifications
You must be signed in to change notification settings - Fork 168
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.
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
chore: relax go directive to permit 1.22.x #2323
Conversation
See also this comment around standardising the sigstore repos, and sigstore/sigstore#1878 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2323 +/- ##
===========================================
- Coverage 66.46% 49.88% -16.58%
===========================================
Files 92 192 +100
Lines 9258 24683 +15425
===========================================
+ Hits 6153 12314 +6161
- Misses 2359 11278 +8919
- Partials 746 1091 +345
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
LGTM, just one question about toolchain version, thanks!
Setting this to 1.23.4 requires all consumers to be building with the absolute latest 1.23.4 or newer release of Go 1.23 and to update their own go.mod accordingly — this seems unnecessarily restrictive for a library module, particularly as the code itself doesn't currently use any modern language constructs and builds fine even with older Go versions. Instead set the go directive to 1.22.0 and use the toolchain directive to recommend the latest 1.23.x when building locally. Note: this also upgrades google/trillian to v1.7.1 and downgrades sigs.k8s.io/release-utils to v0.8.4, which are both required in order to allow the go directive to be relaxed here. Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
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.
Thank you! @cpanato any other comments?
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.
have some comments
go 1.23.4 | ||
go 1.22.0 | ||
|
||
toolchain go1.23.4 |
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.
this will make/force to download Go1.23.4 and then you will not build with go1.22 anymore
so i think here we need to be in go1.23.4 and the toolchain request that
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.
I think you're possibly misunderstanding here? The go
directive specifies the api compatibility of the module and the toolchain
directive specifies a suggested/recommended toolchain to use when building. Only the former has an effect on Go code that imports rekor as a module. The latter only applies as a hint to the rekor build itself and even then it is advisory and can be superseded in various ways.
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.
@dnwe thanks for the clarification but my understanding was that it will require updating go when building, and if any downstream projects import rekor they will be required to use the go 1.23 as well and not 1.22
But I think I did not understand that. I think I am totally wrong. This isn't very clear to me.
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.
Yeah for anything importing rekor toolchain is completely ignored, that’s only used if people go install rekor standalone or clone and build the repo (and even then only if their GOTOOLCHAIN contains auto)
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.
From what I've learned, the motivation to set toolchain
is so that local development matches what we build the release binaries with, given we're keeping the Dockerfile Go container up to date (to pull in the latest security patches), and we'll keep toolchain
in sync.
We should set the go.mod version to be as low as possible, based on if we need features from a newer version of Go. This is for libraries that depend on Rekor, which they themselves can choose to build with either the newest Go binary or one that matches the go.mod.
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.
+1 toolchain does not cascade to imports, go directive can stay 1.22 alongside using a newer toolchain to build.
@@ -1,6 +1,6 @@ | |||
module github.com/sigstore/rekor/hack/tools | |||
|
|||
go 1.23.2 | |||
go 1.22.0 |
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.
why we are downgrading and not keep in go1.23?
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.
We're not downgrading, we are permissively declaring "this go.mod file and the module source code is API compatible with the Go standard library and toolchain from go version 1.22.0 onwards"
go1.22 will be deprecated very soon, go1.24 is around the corner |
go1.24 is still a month away and we can trivially bump the go directive again when it releases if you feel that is desirable — although it is somewhat unnecessary. The idea here is to cut a versioned release that people using go1.22.x can upgrade to |
@cpanato, since we're not using any features from 1.23.x, we can set go.mod to 1.22.x. At some point, we'll update only once we need a feature from the standard library (for example, support for iterators), or once a dependency is bumped up to >= 1.23. Security fixes are pulled in based on the Go binary we build with. I laid out this plan here - #2323 (comment) |
thank you, for all clarifications |
@cpanato please can you start the process to cut a v1.3.8 tag of rekor so we can start pulling this fix into the consuming modules like sigstore-go? |
add a changelog #2331 lets wait for @haydentherapper to we sync |
Summary
Setting this to 1.23.4 requires all consumers to be building with the absolute latest 1.23.4 or newer release of Go 1.23 and to update their own go.mod accordingly — this seems unnecessarily restrictive for a library module, particularly as the code itself doesn't currently use any modern language constructs and builds fine even with older Go versions.
Instead set the go directive to 1.22.0 and use the toolchain directive to recommend the latest 1.22.x when building locally.
Note: this also upgrades google/trillian to v1.7.1 and downgrades sigs.k8s.io/release-utils to v0.8.4, which are both required in order to allow the go directive to be relaxed here.
Release Note
NONE
Documentation
NONE