make default go version the latest stable if unspecified#538
make default go version the latest stable if unspecified#538k8s-ci-robot merged 2 commits intokubernetes:masterfrom
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
|
/approve cc @akhilerm Ben, please remove hold when ready |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, dims The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Did a little debugging locally, I think this is good to go. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
Going through this, just want to do some tests related to how the script works when multiple go versions are involved. |
|
|
||
| defaultGoVersion := deprecatedDefaultGoVersion | ||
| // Respect the default go version if set, otherwise attempt to use current stable go | ||
| // with GOTOOLCHAIN we will fetch dynamically the branch / module specific version anyhow. |
There was a problem hiding this comment.
One small thing here is the go.mod in the branch will always have the .0 version, so when the GOTOOLCHAIN fetches it will be using that, but the go version that we specify is the exact patch version.
There was a problem hiding this comment.
That should be fine. If .0 doesn't work then we should increase the version we specify.
the only reason I can think not to is if the tests etc pass and the code compiles but somehow there's a problem with some command used only by the publishing bot but not by users and that seems incredibly unlikely, anything we're doing to interact with a go toolchain and these modules needs to also work for the consumers of these libraries, so if it doesn't then we increase the patch version.
If for some reason that thinking turns out to be flawed, we can instead use .go-version from k/k which will be more accurate as we have no mechanism to keep the rules files in sync with the stable branches, we only had a rule to keep the default-go-version in sync on the branch the rules file was in ...
| if len(lines) == 0 { | ||
| return "", fmt.Errorf("empty response from go.dev") | ||
| } |
There was a problem hiding this comment.
This check will never be triggered right. Since the strings.Split() will return an array with 1 item, even if the response body is empty.
There was a problem hiding this comment.
Hmmm yeah. Checking this value is not particularly robust, I generally expect an error code over malformed content or it will be an invalid Go version that will fail later but that's not a huge problem.
This case can also only be triggered in what we currently document as a deprecated mode and would've given you go 1.14 (!)
|
Maybe instead we should make the default statically 1.21.0 (oldest with GOTOOLCHAIN), and only increase it again when we observe that binary is no longer available or fails to function (unlikely but plausible) we can test that behavior in k/k without any code changes here (by explicitly setting 1.21.0) and then reconsider. |
4 participants
part of #529, also cleans up stale TODO