Skip to content
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

Fetch go version using gimme if needed #115377

Merged
merged 3 commits into from
Feb 2, 2023

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jan 28, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

Allows kube::golang::verify_go_version to choose a go version. Defaults to .go-version, can be overridden by GO_VERSION=1.x or skipped entirely with FORCE_HOST_GO=y

Copies approach taken by https://github.com/kubernetes-sigs/kind/blob/main/hack/build/setup-go.sh

xref kubernetes/test-infra#28310

Special notes for your reviewer:

Does this PR introduce a user-facing change?

The go version defined in `.go-version` is now fetched when invoking test, build, and code generation targets if the current go version does not match it. Set $FORCE_HOST_GO=y while testing or building to skip this behavior, or set $GO_VERSION to override the selected go version.

/cc @dims @BenTheElder

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. labels Jan 28, 2023
@k8s-ci-robot k8s-ci-robot requested a review from dims January 28, 2023 17:01
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 28, 2023
@dims
Copy link
Member

dims commented Jan 28, 2023

definitely makes things easier. we should probably not get in the way of the local dev env as well (only warn them?) also not install something unless they explicitly ok us installing something in their env?

hack/lib/golang.sh Outdated Show resolved Hide resolved
@liggitt
Copy link
Member Author

liggitt commented Jan 28, 2023

also not install something unless they explicitly ok us installing something in their env?

that's my instinct as well... could make our CI jobs opt into this behavior, and indicate how to opt-into auto-version-setup in verify_go_version if people want it

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 29, 2023
@liggitt
Copy link
Member Author

liggitt commented Jan 29, 2023

awesome, consistently using go1.20rc3 for all our non-e2e presubmits caught an issue in verify-typecheck running with go1.20rc3 that #114502 missed:

ERROR(linux/ppc64le): /home/prow/go/src/k8s.io/kubernetes/_output/local/.gimme/versions/go1.20rc3.linux.amd64/src/os/user/cgo_lookup_unix.go:169:22: invalid argument: index startSize (variable of type bufferKind) must be integer

we already exclude some cgo things in the typecheck because the go type scanner can't reliably interpret it... added a commit to exclude error checking of stdlib packages, will open in a separate PR to merge ahead of #114502

@BenTheElder
Copy link
Member

I think we should consider opt-out.

If you want a custom go toolchain you already need to override any of the dockerized bits.

opt-out gives a smoother more reproducible developer experience for the 99% case.

KIND has opt-out + it will just use the existing toolchain if the version is correct.

@BenTheElder
Copy link
Member

I think the tricky part in k/k is aligning these knobs with the dockerized build bits.
IIRC kind builds are fully using dockerized but some other CI may be mixed.

@liggitt
Copy link
Member Author

liggitt commented Jan 30, 2023

interesting... by changing nothing other than the .go-version, this was able to build with go1.20rc3 even in the e2e:

I0129 05:08:21.171] Server Version: version.Info{Major:"1", Minor:"27+", GitVersion:"v1.27.0-alpha.1.79+87116b4ea03d66", GitCommit:"87116b4ea03d66a1723a16c6571931edf73d8b5f", GitTreeState:"clean", BuildDate:"2023-01-28T18:20:43Z", GoVersion:"go1.20rc3", Compiler:"gc", Platform:"linux/amd64"}

that's ... kind of awesome

@dims
Copy link
Member

dims commented Jan 31, 2023

very cool @liggitt !

@k8s-ci-robot k8s-ci-robot removed the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Feb 2, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 08c4d15a33357bce71f6de844c3d4cf3df04cf81

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@liggitt
Copy link
Member Author

liggitt commented Feb 2, 2023

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 2, 2023
@k8s-ci-robot k8s-ci-robot merged commit 9812eef into kubernetes:master Feb 2, 2023
@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Feb 2, 2023
@BenTheElder
Copy link
Member

belated
/lgtm
/approve
very happy to see this!

k8s-ci-robot added a commit that referenced this pull request Mar 1, 2023
…377-upstream-release-1.24

Automated cherry pick of #115377: Fetch go version using gimme if needed
k8s-ci-robot added a commit that referenced this pull request Mar 1, 2023
…377-upstream-release-1.26

Automated cherry pick of #115377: Fetch go version using gimme if needed
k8s-ci-robot added a commit that referenced this pull request Mar 1, 2023
…377-upstream-release-1.25

Automated cherry pick of #115377: Fetch go version using gimme if needed
@liggitt liggitt deleted the go-version branch May 9, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants