Skip to content
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

fix: set go module directive to 1.22.0 #1878

Merged
merged 3 commits into from
Nov 22, 2024
Merged

Conversation

dnwe
Copy link
Contributor

@dnwe dnwe commented Nov 5, 2024

Summary

The go directive sets the minimum version of Go required to use this module. There's no need to set this to the latest semver patch version of a given Go release unless the semantics of that version of Go are required to build/test/use the module.

Notably since PR #1865 any consumers of sigstore as a library would need to bump their own go directive to 1.22.8 in order to pickup the sigstore 1.8.10 release.

Ref: https://go.dev/ref/mod#go-mod-file-go

Fixes #1879

Release Note

None

Documentation

N/A

The go directive sets the minimum version of Go required to use this
module. There's no need to set this to the latest semver patch version
of a given Go release unless the semantics of that version of Go are
required to build/test/use the module.

Notably since PR sigstore#1865 any consumers of sigstore as a library would need
to bump their own go directive to 1.22.8 in order to pickup the sigstore
1.8.10 release.

Ref: https://go.dev/ref/mod#go-mod-file-go

Signed-off-by: Dominic Evans <[email protected]>
@@ -1,6 +1,6 @@
module github.com/sigstore/sigstore

go 1.22.8
go 1.22.0
Copy link
Member

Choose a reason for hiding this comment

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

we will need to update places like https://github.com/sigstore/sigstore/blob/main/.github/workflows/e2e_test.yml#L37C1-L37C34 to pin to 1.22 or 1.23 otherwise it will install the 1.22.0 only and i think we want to use the latest available always

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot, pushed up d8cb131 as a suggestion of how to address that

Copy link
Contributor

Choose a reason for hiding this comment

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

Caveat: I'm not an expert on Go version resolution

The reason we began to include the patch release version is because the tooling required us to either set the specific version of Go or add a toolchain directive. When @cpanato started moving us to 1.23, it seemed like something changed in the tooling and we didn't need to specify the patch version, only the minor version like what we used to do.

If it is possible to only specify 1.22 or 1.23, let's do that. I'd rather not have the complexity of CI tooling pulling a different version than what's in the go.mod file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Certainly you can't use go 1.22 in go.mod, in recent go1.22.x versions they'll just complain that go.mod needs tidying and will update it to 1.22.0 and also add a toolchain go1.22.8 directive:

% grep -E '^(go|toolchain)' go.mod
go 1.22
% go1.22.8 build ./...
go: updates to go.mod needed; to update it:
        go mod tidy
% go1.22.8 mod tidy
% grep -E '^(go|toolchain)' go.mod
go 1.22.0
toolchain go1.22.8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something does seem to have changed in 1.23 whereby '1.23' is seemingly accepted as a directive again

% grep -E '^(go|toolchain)' go.mod
go 1.23
% go1.23.2 build ./...
% go1.23.2 mod tidy
% grep -E '^(go|toolchain)' go.mod
go 1.23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change in behaviour in 1.23 is referenced in this comment on this well-cited GH issue on the confusion around the go directive changes:

image

golang/go#62278 (comment)

However, that's really saying that go itself will just silently assume 1.23.0 for module requirements and toolchain rather than the more useful 1.23.X.

Unfortunately from a GitHub Actions perspective, setup-go doesn't currently know or use go toolchain versions, it's just manually parsing the go.mod for 'go' directives itself and hasn't really kept up with the 1.22 and 1.23 changes here. There's an issue tracking that over here

Copy link
Member

Choose a reason for hiding this comment

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

thank you @dnwe for all the clarification TIL a lot about this fuzzy thing that i cant understand

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for digging into this!

Instead of using setup-go's built-in go-versions-file behaviour, grab
the MAJOR.MINOR directive from go.mod manually and use that as input to
request the latest `.PATCH` variant from setup-go

Signed-off-by: Dominic Evans <[email protected]>
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

some small comments

.github/workflows/codeql.yml Outdated Show resolved Hide resolved
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm

thanks

@haydentherapper your final review

@cpanato cpanato merged commit 257797e into sigstore:main Nov 22, 2024
9 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.

go directive in go.mod is unnecessarily specific
3 participants