Improve toolchain handling #460
Conversation
|
Testing this action https://github.com/matthewhughes934/setup-go-test, see the workflow runs for details https://github.com/matthewhughes934/setup-go-test/actions |
switch off of `go-version-file` in the Github Actions, because it doesn't work great with the new `go mod tidy` format that go 1.22 does. See: * [Improve toolchain handling actions/setup-go#460](actions/setup-go#460) * [More specific handling/detection of Go toolchain versions actions/setup-go#457](actions/setup-go#457)
switch off of `go-version-file` in the Github Actions, because it doesn't work great with the new `go mod tidy` format that go 1.22 does. See: * [Improve toolchain handling actions/setup-go#460](actions/setup-go#460) * [More specific handling/detection of Go toolchain versions actions/setup-go#457](actions/setup-go#457)
switch off of `go-version-file` in the Github Actions, because it doesn't work great with the new `go mod tidy` format that go 1.22 does. See: * [Improve toolchain handling actions/setup-go#460](actions/setup-go#460) * [More specific handling/detection of Go toolchain versions actions/setup-go#457](actions/setup-go#457)
be5f1f1 to
145e58d
Compare
|
This PR effectively addresses and fixes #457. The implementation:
This change will prevent the unexpected behavior where specifying The breaking change is well-documented and justified - users who rely on automatic toolchain downloads will need to adjust their workflows, but this brings the action in line with official Go Docker images and provides more predictable behavior. |
|
Did you rebase already? GitHub doesn't allow me to see the parent commit. |
The vulnerability reported is also present on $ git checkout main
$ git rev-parse HEAD
8e57b58e57be52ac95949151e2777ffda8501267
$ npm audit --audit-level=high
# npm audit report
form-data >=4.0.0 <4.0.4 || <2.5.4
Severity: critical
form-data uses unsafe random function in form-data for choosing boundary - https://github.com/advisories/GHSA-fjxv-7rqg-78g4
form-data uses unsafe random function in form-data for choosing boundary - https://github.com/advisories/GHSA-fjxv-7rqg-78g4
fix available via `npm audit fix`
node_modules/@azure/core-http/node_modules/form-data
node_modules/@types/node-fetch/node_modules/form-data
node_modules/form-data
1 critical severity vulnerability
To address all issues, run:
npm audit fixThe vulnerability is two days old: GHSA-fjxv-7rqg-78g4, here's a separate PR for that #618 (though I'm not sure why |
|
Dependabot doesn't always work as you would expect, it's not resilient for example, a simple network error can disable updates. Here is your failed run from 2 days ago, only maintainers are allowed to read the logs: As long as AI reviews haven't been enabled, only manual maintainer work could speed-up reviews. |
145e58d to
c58ae12
Compare
|
I've dropped the commit that changed behaviour from install the Go version specified in the |
c58ae12 to
7d12308
Compare
|
I'm holding my breath based on #618 |
|
Thank you for your thoughtful work on this PR, After reviewing the code changes in detail, We wanted to highlight a specific scenario where the current implementation does not fully satisfy one of our core criteria: Scenario: When a workflow uses only a go.mod file that contains both a go directive and a toolchain directive (with the toolchain version being higher), and there is no explicit go-version or GOTOOLCHAIN=local set in the workflow. Current Code Behavior: The action installs and uses the Go version from the go directive, rather than the one specified in the toolchain directive. Expected Behavior (per our criteria and official Go documentation): In this situation, the action should detect the presence of the toolchain directive in go.mod and install or use the toolchain version (for example, go1.22.6). The toolchain directive is meant to indicate the intended Go toolchain for the project and should take precedence if no explicit workflow overrides are present.
Could you please update the logic so that when only a go.mod file is present and both directives exist, the action installs and uses the Go version from the toolchain directive unless overridden by the workflow or environment? You can also make use of implementing the go-version-directive option if required, which you have suggested as per your earlier comment. This adjustment will ensure that all scenarios are covered and that the action's behavior aligns with both our criteria and the expectations of Go users relying on toolchain directives. Additionally, please update the documentation to reflect any changes or new options you introduce, so users are aware of how the action now handles these scenarios and how to configure it for their needs. Once this is addressed, we can discuss progressing the PR further. Thank you again for your contribution ! |
7d12308 to
0348eaa
Compare
I've just rebased my changes on |
|
Thank you for your contribution and the improvements made to the toolchain handling logic in this PR. Upon reviewing the code changes, We noticed that the current implementation does not fully align with the specific criteria for Go version selection in workflows when both
Current Code Issue: The present code still prioritizes the Request for Changes:
Documentation: Please update the documentation to clearly describe:
This will help future contributors and users understand the behavior and reasoning behind version selection in various workflows. Once these changes are addressed to fully respect the discussed criteria, we can proceed with reviewing the PR for merge. Thanks for your contribution ! |
Force `go` to always use the local toolchain (i.e. the one the one that
shipped with the go command being run) via setting the `GOTOOLCHAIN`
environment variable to `local`[1]:
> When GOTOOLCHAIN is set to local, the go command always runs the
bundled Go toolchain.
This is how things are setup in the official Docker images (e.g.[2], see
also the discussion around that change[3]). The motivation behind this
is to:
* Reduce duplicate work: if the `toolchain` version in `go.mod` was
greated than the `go` version, the version from the `go` directive
would be installed, then Go would detect the `toolchain` version and
additionally install that
* Avoid Unexpected behaviour: if you specify this action runs with some Go
version (e.g. `1.21.0`) but your go.mod contains a `toolchain` or `go`
directive for a newer version (e.g. `1.22.0`) then, without any other
configuration/environment setup, any go commands will be run using go
`1.22.0`
This will be a **breaking change** for some workflows. Given a `go.mod`
like:
module proj
go 1.22.0
Then running any `go` command, e.g. `go mod tidy`, in an environment
where only go versions before `1.22.0` were installed would previously
trigger a toolchain download of Go `1.22.0` and that version being used
to execute the command. With this change the above would error out with
something like:
> go: go.mod requires go >= 1.22.0 (running go 1.21.7;
GOTOOLCHAIN=local)
[1] https://go.dev/doc/toolchain#select
[2] https://github.com/docker-library/golang/blob/dae3405a325073e8ad7c8c378ebdf2540d8565c4/Dockerfile-linux.template#L163
[3] docker-library/golang#472
Prefer this over the version from the `go` directive. Per the docs[1] > The toolchain line declares a suggested toolchain to use with the module or workspace It seems reasonable to use this, since running this action in a directory containing a `go.mod` (or `go.work`) suggests the user is wishing to work _with the module or workspace_. Link: https://go.dev/doc/toolchain#config [1] Issue: actions#457
Only modify env if `GOTOOLCHAIN` is not set
Avoid installing from `toolchain` if `GOTOOLCHAIN` is `local`, also better regex for matching toolchain directive
0348eaa to
b967a46
Compare
* Configure environment to avoid toolchain installs
Force `go` to always use the local toolchain (i.e. the one the one that
shipped with the go command being run) via setting the `GOTOOLCHAIN`
environment variable to `local`[1]:
> When GOTOOLCHAIN is set to local, the go command always runs the
bundled Go toolchain.
This is how things are setup in the official Docker images (e.g.[2], see
also the discussion around that change[3]). The motivation behind this
is to:
* Reduce duplicate work: if the `toolchain` version in `go.mod` was
greated than the `go` version, the version from the `go` directive
would be installed, then Go would detect the `toolchain` version and
additionally install that
* Avoid Unexpected behaviour: if you specify this action runs with some Go
version (e.g. `1.21.0`) but your go.mod contains a `toolchain` or `go`
directive for a newer version (e.g. `1.22.0`) then, without any other
configuration/environment setup, any go commands will be run using go
`1.22.0`
This will be a **breaking change** for some workflows. Given a `go.mod`
like:
module proj
go 1.22.0
Then running any `go` command, e.g. `go mod tidy`, in an environment
where only go versions before `1.22.0` were installed would previously
trigger a toolchain download of Go `1.22.0` and that version being used
to execute the command. With this change the above would error out with
something like:
> go: go.mod requires go >= 1.22.0 (running go 1.21.7;
GOTOOLCHAIN=local)
[1] https://go.dev/doc/toolchain#select
[2] https://github.com/docker-library/golang/blob/dae3405a325073e8ad7c8c378ebdf2540d8565c4/Dockerfile-linux.template#L163
[3] docker-library/golang#472
* Prefer installing version from `toolchain` directive
Prefer this over the version from the `go` directive. Per the docs[1]
> The toolchain line declares a suggested toolchain to use with the
module or workspace
It seems reasonable to use this, since running this action in a
directory containing a `go.mod` (or `go.work`) suggests the user is
wishing to work _with the module or workspace_.
Link: https://go.dev/doc/toolchain#config [1]
Issue: #457
* squash! Configure environment to avoid toolchain installs
Only modify env if `GOTOOLCHAIN` is not set
* squash! Prefer installing version from `toolchain` directive
Avoid installing from `toolchain` if `GOTOOLCHAIN` is `local`, also
better regex for matching toolchain directive
* CI workflows: upgrade setup-go action to v6 https://github.com/actions/setup-go/releases/tag/v6.0.0 to make use actions/setup-go#460 to specify Go version for development and CI pipelines in a single place (go.mod file). * Bumped Go toolchain from 1.24.4 to 1.24.7 for security fixes, eg grpc-ecosystem#267 (after a release)
Configure environment to avoid toolchain installs
Force
goto always use the local toolchain (i.e. the one the one thatshipped with the go command being run) via setting the
GOTOOLCHAINenvironment variable to
local[1]:This is how things are setup in the official Docker images (e.g.[2], see
also the discussion around that change[3]). The motivation behind this
is to:
toolchain will be detected, the toolchain will be detected and then
another version of Go installed[4]
version (e.g.
1.21.0) but your go.mod contains atoolchainorgodirective for a newer version (e.g.
1.22.0) then, without any otherconfiguration/environment setup, any go commands will be run using go
1.22.0This will be a breaking change for some workflows. Given a
go.modlike:
Then running any
gocommand, e.g.go mod tidy, in an environmentwhere only go versions before
1.22.0were installed would previouslytrigger a toolchain download of Go
1.22.0and that version being usedto execute the command. With this change the above would error out with
something like:
Link: https://go.dev/doc/toolchain#select [1]
Link: https://github.com/docker-library/golang/blob/dae3405a325073e8ad7c8c378ebdf2540d8565c4/Dockerfile-linux.template#L163 [2]
Link: proposal: set GOTOOLCHAIN=local (or =path) in our image docker-library/golang#472 [3]
Link: Tar errors on cache restore after toolchain installation #424 [4]
Issue: More specific handling/detection of Go toolchain versions #457
Prefer installing version from
toolchaindirectivePrefer this over the version from the
godirective. Per the docs[1]It seems reasonable to use this, since running this action in a
directory containing a
go.mod(orgo.work) suggests the user iswishing to work with the module or workspace.
Link: https://go.dev/doc/toolchain#config [1]
Issue: More specific handling/detection of Go toolchain versions #457