-
Notifications
You must be signed in to change notification settings - Fork 660
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: use go-version-file to read go-version from go.mod #914
base: main
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
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. Nice clean up. Thanks, @mmorel-35.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ivanvc, mmorel-35 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
By the way, I just checked, and our gofail and raft repositories have the same way of defining the go version. I believe it comes from the original workflows from the etcd repository. Feel free to update those as well :) |
OK ! I will. |
- uses: actions/setup-go@f111f3307d8850f501ac008e886eec1fd1932a34 # v5.3.0 | ||
with: | ||
go-version: ${{ steps.goversion.outputs.goversion }} | ||
go-version-file: .go-version |
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.
Question: Based on the official doc, The go-version-file input accepts a path to a go.mod file or a go.work file that contains the version of Go to be used by a project.
But here it's not a go.mod file, nor go.work file. Is it guaranteed to work?
Based on the workflow logs, it confirmed that it indeed works. Just to double confirm whether there is any potential issue or is this the recommended usage?
Run actions/setup-go@f111f3307d8850f501ac008e886eec1fd1932a34
Setup go version spec 1.23.6
Found in cache @ /opt/hostedtoolcache/go/1.23.6/x64
Added go to the path
Successfully set up Go version 1.23.6
/opt/hostedtoolcache/go/1.23.6/x64/bin/go env GOMODCACHE
/opt/hostedtoolcache/go/1.23.6/x64/bin/go env GOCACHE
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 can replace with gomod if you prefer ?
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 use gomod in other repositories (i.e., auger). So, I think it would be a better approach.
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.
If we use go.mod, then it use the go version in the go directive (i.e. go 1.23
) instead of the toolchain directive (i.e. toolchain go1.23.6
). It isn't expected.
Setup go version spec 1.23
Found in cache @ /opt/hostedtoolcache/go/1.23.6/x64
Added go to the path
Successfully set up Go version 1.23
I do not see a clear answer to my previous question in #914 (comment) so far. So I suggest that we follow the same solution as what the etcd is using for now,
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.
FYI. Just raised a feature request for actions/setup-go, actions/setup-go#557
Signed-off-by: Matthieu MOREL <[email protected]>
faf41f8
to
f9c424e
Compare
Description
go-version can be read directly from .go-version or go.mod files using go-version-file in setup-go.
This PR uses .go-version to setup-go and allows the removal 'goversion' steps