-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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(ci): ensure 'auto' toolchain applies to generate #3436
base: master
Are you sure you want to change the base?
Conversation
@@ -6,6 +6,11 @@ set -e | |||
|
|||
CDPATH="" cd -- "$(dirname -- "$0")/.." | |||
|
|||
# force 1.22.0 default and "auto" toolchain (i.e., use the toolchain directive) | |||
# when running generate and mod tidy | |||
GOTOOLCHAIN="go1.22.0+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.
@dnwe is this intended to support the use of Go 1.21
? And how is it different than setting the toolchain in the go.mod file?
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.
No, this is intended to override whatever the particular developer might have set locally in their environment and to set a policy of "use go1.22.0 by default and then also consult the toolchain directive and fetch/cache that version if its newer and use that", ensuring that policy is applied throughout the generate.sh usage
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.
As noted above, starting in Go 1.21, Go toolchains refuse to run in workspaces or modules that require newer Go versions. Instead, they report an error and exit.
So given that Go 1.22
is required in the go.mod files, why is the env variable required?
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.
Because that’s an advisory directive that the developer themselves might have overridden locally in their own config by the envvar
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.
nit: instead of two lines, can you please combine them?
GOTOOLCHAIN="go1.22.0+auto" | |
export GOTOOLCHAIN="go1.22.0+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.
@dnwe am I misunderstanding the docs then because they clearly state that as of Go 1.21
it's no longer advisory?
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.
go directive is a mandatory minimum, then the toolchain directive is advisory. In the absence of a toolchain directive the go directive also becomes the toolchain directive.
In any case the nuance here is that we’re just enforcing a known value in the script to get repeatable behaviour. Otherwise a developer could (e.g.,) set GOTOOLCHAIN=local and have go 1.24rc1 installed and the generate/fit behaviour may differ
@dnwe - I would also personally appreciate it if you stopped using force-push in this repo, because this is not the same as what I originally reviewed, and it helps me to see the progression. Thank you! |
Apart from the first time (where I corrected a mistake pre-review) that was just me hitting rebase in the UI because my branch was out-of-date with master. The UI does show each of the commit refs for each rebase |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3436 +/- ##
==========================================
- Coverage 97.72% 92.26% -5.46%
==========================================
Files 153 174 +21
Lines 13390 15023 +1633
==========================================
+ Hits 13085 13861 +776
- Misses 215 1068 +853
- Partials 90 94 +4 ☔ View full report in Codecov by Sentry. |
No description provided.