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

Require go 1.17+ #3335

Merged
merged 9 commits into from
Apr 1, 2024
Merged

Require go 1.17+ #3335

merged 9 commits into from
Apr 1, 2024

Conversation

BenTheElder
Copy link
Member

This enables better dependency management via https://go.dev/ref/mod#graph-pruning

We previously required 1.16+

We don't want to require especially recent versions because that will break users with go install using older packaged golang.

We use currently supported much more recent go for our own builds.

NOTE: there are no docs changes yet, because the docs have users installing v0.20 which is still compatible with go 1.16

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 24, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 24, 2023
@BenTheElder
Copy link
Member Author

had to update the tools while working on this, looks like we picked up some new linter issues, working on resolving.

@k8s-ci-robot k8s-ci-robot added area/provider/docker Issues or PRs related to docker area/provider/podman Issues or PRs related to podman labels Aug 24, 2023
@@ -252,7 +252,7 @@ func makeNodesReconciler(cniConfig *CNIConfigWriter, hostIP string, ipFamily IPF

// obtain the PodCIDR gateway
var nodeIPv4, nodeIPv6 string
for _, ip := range nodeIPs.List() {
for _, ip := range sets.List(nodeIPs) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[this is the non-deprecated way to use the sets package]

// See ./.go-version for the go compiler version used when building binaries
//
// https://go.dev/doc/modules/gomod-ref#go
go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well , this is changing in 1.21

For example, a go.mod that says go 1.21.0 with no toolchain line is interpreted as if it had a toolchain go1.21.0 line.

https://go.dev/doc/toolchain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rework everything around .go-version to switch to this toolchain line.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I forgot about this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, I added a commit to deal with this, but the remaining downside (Also true in k/k currently), is that IDEs won't see all our bash hacks.

what we probably need to do is pivot to setting toolchain in the go mods and then also for the image builds of external binaries, but that's going to force us to decentralize setting the go version unfortunately.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current PR is enough to ensure our builds / tools work at least

@aojea
Copy link
Contributor

aojea commented Aug 25, 2023

just the question about the behavior change in golang with 1.21, the rest LGTM

@BenTheElder BenTheElder added this to the v0.21.0 milestone Dec 14, 2023
@BenTheElder
Copy link
Member Author

I'm going to finish this up today, ahead of generally bumping things for a v0.21 cut.

@BenTheElder
Copy link
Member Author

It's going to be a bit awkward to re-work this around plumbing GOTOOLCHAIN and I'm not sure what the best answer is, we've definitely got downstreams depending on patching .go-version. It might be best to just break that in favor of toolchain in the modules and put a release note. Need to think a bit.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2024
@BenTheElder
Copy link
Member Author

I'm punting this to the next release because we really need to get that runc CVE out, but some veersion of the changes here probably need to be high on the list longterm as part of switching to go1.21+

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 2, 2024
@BenTheElder
Copy link
Member Author

rebased and started forcing GOTOOLCHAIN="go${GOVERSION}" everywhere for now.

@BenTheElder BenTheElder removed this from the v0.21.0 milestone Feb 2, 2024
@BenTheElder
Copy link
Member Author

BenTheElder commented Feb 2, 2024

pull-kind-conformance-parallel-ga-only — Pod got deleted unexpectedly 

  
/retest

@BenTheElder
Copy link
Member Author

@aojea go 1.22 released so go 1.20 will be EOL and we'll have to sort this out. Will aim to look again later this week for better iteration, but the current version should be usable.

@@ -1 +1 @@
1.20.13
1.21.6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be 1.22 no?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this PR has been around awaiting review for a while :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're still on 1.20.13 @ HEAD

@@ -141,6 +143,7 @@ RUN git clone --filter=tree:0 "${RUNC_CLONE_URL}" /runc \
&& cd /runc \
&& git checkout "${RUNC_VERSION}" \
&& eval "$(gimme "${GO_VERSION}")" \
&& export GOTOOLCHAIN="go${GO_VERSION}" \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line159 seems is missing this export too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes. I will add this and bump up the go versions again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed this one.

@@ -1,6 +1,6 @@
module sigs.k8s.io/kind/images/kindnetd

go 1.13
go 1.18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why don't you move to latest here then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it matched kubernetes at the time. note that this is not the go version used to compile, this is the language level features used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's leave this for a later PR, this PR is already a lot and I don't want to deal with any breakage while trying to get the rest of the PR in.

This enables better dependency management via  https://go.dev/ref/mod#graph-pruning

We previously required 1.16+

We don't want to require especially recent versions because that will break users with `go install` using older packaged golang.

We use currently supported much more recent go for our own builds.
kindnetd is typically pre-compiled and not installed with "go get", kind development already uses much more recent go so requiring 1.18 for kindnetd is fine
TODO: look at using deprecating .go-version in favor of GOTOOLCHAIN existing knobs
@BenTheElder
Copy link
Member Author

BenTheElder commented Apr 1, 2024

TODO:

  • rebump everything to latest
  • explore using go.mod gotoolchain instead (need to handle things like the gimme fallback, and approach to keep these in sync or not)

leaving for follow-ups, I want to get in the core "we're unblocked to manage go versions with 1.21+ without switching language version to 1.21+" so we can collectively iterate from there (requiring 1.17+ also cleans up dep management and go get vs go install, required some docs changes)

@BenTheElder BenTheElder changed the title [WIP] Require go 1.17+ Require go 1.17+ Apr 1, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2024
@BenTheElder
Copy link
Member Author

There is a new linter failure from nerdctl code, fixing ...

@aojea
Copy link
Contributor

aojea commented Apr 1, 2024

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 1, 2024
@BenTheElder
Copy link
Member Author

@k8s-ci-robot k8s-ci-robot merged commit 506824a into kubernetes-sigs:main Apr 1, 2024
29 checks passed
@BenTheElder BenTheElder deleted the go1.17+ branch April 2, 2024 00:12
@BenTheElder
Copy link
Member Author

@BenTheElder
Copy link
Member Author

ah, I know what this is. patch coming in a bit (dealing with some internal work atm)

@BenTheElder
Copy link
Member Author

base is building again

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/provider/docker Issues or PRs related to docker area/provider/podman Issues or PRs related to podman cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants