Skip to content

fix: Relax go directive in go.mod to 1.22.0 - #3423

Merged
gmlewis merged 1 commit into
google:masterfrom
dnwe:go-version
Jan 9, 2025
Merged

fix: Relax go directive in go.mod to 1.22.0#3423
gmlewis merged 1 commit into
google:masterfrom
dnwe:go-version

Conversation

@dnwe

@dnwednwe commented Jan 7, 2025

Copy link
Copy Markdown
Contributor

Rather than require all consumers to be building with the latest 1.22.10 release of Go 1.22, match the support statement and permit any version of 1.22.x to be used. Since Go 1.21 the go directive in go.mod is considered a hard requirement version and anything older will refuse to build.

Update the N-1 go-version used in GitHub Actions to be the .0 release to help ensure this compatibility is maintained in the future.

Fixes#3422

@google-cla

google-claBot commented Jan 7, 2025

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Rather than require all consumers to be building with the latest 1.22.10
release of Go 1.22, match the support statement and permit any version
of 1.22.x to be used. Since Go 1.21 the go directive in go.mod is
considered a hard requirement version and anything older will refuse to
build.
Update the N-1 go-version used in GitHub Actions to be the .0 release to
help ensure this compatibility is maintained in the future.
Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com>
@gmlewisgmlewis changed the title fix: relax go directive in go.mod to 1.22.0fix: Relax go directive in go.mod to 1.22.0Jan 7, 2025
@codecov

codecovBot commented Jan 7, 2025

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 92.22%. Comparing base (2b8c7fa) to head (f648bc4).
Report is 218 commits behind head on master.

Files with missing linesPatch %Lines
example/verifyartifact/main.go0.00%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## master #3423 +/- ##
==========================================
- Coverage 97.72% 92.22% -5.51% 
==========================================
Files 153 173 +20 Lines 13390 14937 +1547 ==========================================
+ Hits 13085 13775 +690 - Misses 215 1068 +853 - Partials 90 94 +4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thank you, @dnwe !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewisgmlewis added the NeedsReview PR is awaiting a review before merging. label Jan 7, 2025
@stevehipwell

Copy link
Copy Markdown
Contributor

@dnwe is there a specific reason why the examples and tools are being downgraded to Go 1.22.0? The changes to the tests will validate the change to go.mod but as the examples and tools are consumers wouldn't it make more sense to have them configured as an end user would with a specific Go version?

Also it might improve the maintainability of the GH Actions automation to use https://endoflife.date/go (JSON API) to automate the N-1 version selection?

curl -sS https://endoflife.date/api/go.json | jq -r '[.[] | select(.eol | not)] | sort_by(.releaseDate) | reverse | .[1] | "\(.cycle).0"'

@dnwe

dnwe commented Jan 8, 2025

Copy link
Copy Markdown
ContributorAuthor

@dnwe is there a specific reason why the examples and tools are being downgraded to Go 1.22.0? The changes to the tests will validate the change to go.mod but as the examples and tools are consumers wouldn't it make more sense to have them configured as an end user would with a specific Go version?

The intention was that the examples should work out-of-the-box with any Go version within the supported range, so pinning to the same as the top-level module ensures that the union of dependencies they use are also compatible right back to 1.22.0. It's also convenient as they can be updated in sync in the future.

Also it might improve the maintainability of the GH Actions automation to use https://endoflife.date/go (JSON API) to automate the N-1 version selection?

curl -sS https://endoflife.date/api/go.json | jq -r '[.[] | select(.eol | not)] | sort_by(.releaseDate) | reverse | .[1] | "\(.cycle).0"'

I can look at doing this in a follow-up PR if it would be useful — we'd build the go-version matrix programmatically in one stage and feed it in

@stevehipwell

Copy link
Copy Markdown
Contributor

The intention was that the examples should work out-of-the-box with any Go version within the supported range, so pinning to the same as the top-level module ensures that the union of dependencies they use are also compatible right back to 1.22.0. It's also convenient as they can be updated in sync in the future.

I get the intention, but is it valid in the context of an example? An example is meant to show the correct way the system "SHOULD" be used, while supporting the (N-1).0 version is a way the system "COULD" be used. IMHO I can see the value in supporting (N-1).0 in a package, due to constraints ETC; but I can only see downsides in defaulting to (N-1).0 in the examples (I expect to see latest in examples unless there is a reason to hold back).

@dnwe

dnwe commented Jan 8, 2025

Copy link
Copy Markdown
ContributorAuthor

The intention was that the examples should work out-of-the-box with any Go version within the supported range, so pinning to the same as the top-level module ensures that the union of dependencies they use are also compatible right back to 1.22.0. It's also convenient as they can be updated in sync in the future.

I get the intention, but is it valid in the context of an example? An example is meant to show the correct way the system "SHOULD" be used, while supporting the (N-1).0 version is a way the system "COULD" be used. IMHO I can see the value in supporting (N-1).0 in a package, due to constraints ETC; but I can only see downsides in defaulting to (N-1).0 in the examples (I expect to see latest in examples unless there is a reason to hold back).

Similar constraints apply though. If an enterprise developer is using RHEL in a constrained environment with the package-manager installed toolchain then they'll have go 1.22.9. They'll see the repo says "supports 1.22" and they'll clone it and attempt to build/run some examples and they get a rather unhelpful failure message of:

go: go.mod requires go >= 1.22.10 (running go 1.22.9; GOTOOLCHAIN=local)

Yes, they could manually edit the go.mod and continue, but it feels like the samples should just track the same supported levels as the main module, not require an explicit (latest) patch version and just work out of the box. There's no language changes between .9 and .10 so why force the requirement to use it?

There only real downside to this approach is that you can't pull in other dependencies in the examples that have similar made the (imo mistake) of setting an arbitrarily high 1.22.x or 1.23.x go version in their go.mod directive

@dnwe

dnwe commented Jan 8, 2025

Copy link
Copy Markdown
ContributorAuthor

Also note that the Go team had a change of heart in Go 1.23 and re-permitted go 1.23 as an alias for go 1.23.0

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)

@stevehipwellstevehipwell left a comment

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.

Approved as @gmlewis is happy.

@dnwe could you take a look at #3417 as it's also waiting on a second aproval.

@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you @dnwe and @stevehipwell !
Merging.

@gmlewis
gmlewis merged commit a44a24b into google:masterJan 9, 2025
}

var b *bundle.Bundle
var b *bundle.ProtobufBundle

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@dnwe - why did you make this change?
I'm now getting a deprecation warning:

deprecation-2025-01-09_10-26-23

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@gmlewis sigstore-go was rolled back to 0.5.1 in the sample, because in 0.6.2 they had similarly made a mistake of bumping the go directive to latest 1.23.2 (see here). They're addressing that according to this comment, but in the meantime we had to rollback in the sample in order to pin to Go 1.22. In 0.5.1 Bundle doesn't exist yet and ProtobufBundle isn't deprecated https://github.com/sigstore/sigstore-go/blob/v0.5.1/pkg/bundle/bundle.go#L51-L55

@gmlewis

Copy link
Copy Markdown
Collaborator

@dnwe - another question. Now, if I update a dependency in example/go.mod, no matter what I do, Go 1.23.4 changes the go.mod text from:

go 1.22.0

to:

go 1.22.5
toolchain go1.23.4

How do I get around this and still abide by your newly-added requirements?

If I revert this change, all the tooling then fails with this error message:

go: updates to go.mod needed; to update it:
go mod tidy

@dnwe
dnwe deleted the go-version branch January 9, 2025 14:41
@dnwe

dnwe commented Jan 9, 2025

Copy link
Copy Markdown
ContributorAuthor

@dnwe - another question. Now, if I update a dependency in example/go.mod, no matter what I do, Go 1.23.4 changes the go.mod text from:

go 1.22.0

to:

go 1.22.5
toolchain go1.23.4

How do I get around this and still abide by your newly-added requirements?

So this happens when you are pulling in a dependency update that requires a newer go version in its go.mod directive. You can prevent this by always including toolchain and go versions in your go get command for updates

e.g., to update all available deps (go get -u toolchain@none go@1.22.0) or just to update a specific dep (if available) go get toolchain@none go@1.22.0 github.com/ProtonMail/go-crypto@latest

@gmlewis

Copy link
Copy Markdown
Collaborator

Ah! That explains it. Thank you, @dnwe !
I thought I was going crazy there.
I was actually trying to solve the dependabot security issue, but maybe I'll just wait a few days and see what happens with that then.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NeedsReviewPR is awaiting a review before merging.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

top-level go directive in go.mod is now unnecessarily restrictive?

3 participants

@dnwe@stevehipwell@gmlewis