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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃摉 Bump go minor versions in release branches if they are out of support #9415

Merged

Conversation

sbueringer
Copy link
Member

@sbueringer sbueringer commented Sep 13, 2023

Signed-off-by: Stefan B眉ringer buringerst@vmware.com

What this PR does / why we need it:
I would like to propose to change our backport policy. tl;dr is:

  • On main: let's align to the Go minor version used by our k/k dependencies (Makefile + go.mod)
  • On release-branches: let's bump the Go minor version used to build our controller images if the Go minor version goes out of support (only Makefile)

The reason why I'm bringing this up is because Go 1.19 is out of support as of now but we still use it on the release-1.4 branch.

The idea would be to only bump the Go version we use to build our controller images. This means bumping the version in the Makefile, but not in go.mod (go.mod would break library consumers), similar to what Kubernetes is doing (they also don't bump the go.mod file after the minor release is out).

In my opinion it is absolutely safe to do so as our Makefile version doesn't affect anyone that consumes our go code / module.

Overall we're aligning to the way Kubernetes is handling this, for more context see: https://github.com/kubernetes/enhancements/tree/master/keps/sig-release/3744-stay-on-supported-go-versions

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #7974

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 13, 2023
@sbueringer sbueringer changed the title 馃摉 Bump go minor version in release branches if necessary 馃摉 Bump go minor versions in release branches if they are out of support Sep 13, 2023
Copy link
Contributor

@killianmuldoon killianmuldoon left a comment

Choose a reason for hiding this comment

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

/area documentation

/lgtm
I think this is essential so our supported code is using support Go versions.

/hold
(for more input)

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. area/documentation Issues or PRs related to documentation and removed do-not-merge/needs-area PR is missing an area label labels Sep 13, 2023
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 58efe56177b27ce28b34af0a5bc87fc457a19552

to bump to a newer Go minor version (e.g. to pick up CVE fixes). This could have impact on everyone importing Cluster API.
- Changes to use the latest Go patch version.
- Changes to bump the Go minor version, if the Go minor version of a supported branch goes out of support (e.g. to pick up bug and CVE fixes).
This has no impact on folks importing Cluster API as we won't modify the version in `go.mod` and the version in the `Makefile` does not affect them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Please note that the bump will be automatically visible in release notes through the Go bump PR

Signed-off-by: Stefan B眉ringer buringerst@vmware.com
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2023
@sbueringer
Copy link
Member Author

I'll also bring it up in the office hours today

Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: c1a3190c944ef8dd7e48446b63fd2a3b387b1544

@chrischdi
Copy link
Member

/lgtm

Copy link
Member

@furkatgofurov7 furkatgofurov7 left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

@g-gaston g-gaston left a comment

Choose a reason for hiding this comment

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

/lgtm

@sbueringer just for background, have we ever had to modify the actual code when bumping go versions? Deprecations and minor things like that are not a problem, I'm only concerned about behavior changes in the standard library, defaulting changes, etc.

I know upstream k/k has run into this a lot which made version bumps painful and hence it used to be avoided in patch releases. I believe they are (were) now leveraging GODEBUG to battle this. I wonder if we need to care about this or if our codebase is "high level" enough (ha) that we might not be hit by the same issues. That said, 1.21 is supposed to handle this by default so even if this is an issue, it might only be until al our minor versions are using 1.21 or higher.

@sbueringer
Copy link
Member Author

sbueringer commented Sep 13, 2023

/lgtm

@sbueringer just for background, have we ever had to modify the actual code when bumping go versions? Deprecations and minor things like that are not a problem, I'm only concerned about behavior changes in the standard library, defaulting changes, etc.

I know upstream k/k has run into this a lot which made version bumps painful and hence it used to be avoided in patch releases. I believe they are (were) now leveraging GODEBUG to battle this. I wonder if we need to care about this or if our codebase is "high level" enough (ha) that we might not be hit by the same issues. That said, 1.21 is supposed to handle this by default so even if this is an issue, it might only be until al our minor versions are using 1.21 or higher.

IIRC in the past 2 years we never had to make a change when we bumped Go. I think the difference is that interactions with CAPI controllers are very limited and they are also well covered by e2e tests. Basically we're communicating with the apiserver and the apiserver communicates with us. I think a major difference for Kubernetes is that the kube-apiserver is accessed by everyone.

But I think if we didn't have to make any changes on main when bumping to a new Go minor version it should be fine to bump on release branches a few months later when the previous Go version goes out of support.

Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

+1 from me

@elmiko
Copy link
Contributor

elmiko commented Sep 13, 2023

@sbueringer will keep this open for a week lazy consensus

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Thanks for bringing up this topic!
As a follow-up, we could think of ways for surfacing go versions (go.mod, build) if they diverge in release notes and eventually in the book
/lgtm

@sbueringer
Copy link
Member Author

sbueringer commented Sep 15, 2023

Thanks for bringing up this topic! As a follow-up, we could think of ways for surfacing go versions (go.mod, build) if they diverge in release notes and eventually in the book /lgtm

One idea would be to simply make it part of the release notes. Just like we should surface a list with all our Go dependencies and their versions (like upstream k/k), we could do the same for the Go version(s). I think this would be more sustainable then adding a table somewhere which maps Cluster API minor+patch versions to Go compile & go.mod versions (I also think it's more realistic that folks find & look it up in the release notes vs. somewhere in our book)

@sbueringer
Copy link
Member Author

cc @kubernetes-sigs/cluster-api-release-team @furkatgofurov7 ^^

@fabriziopandini
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 15, 2023
@sbueringer
Copy link
Member Author

/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 Sep 20, 2023
@k8s-ci-robot k8s-ci-robot merged commit ea26509 into kubernetes-sigs:main Sep 20, 2023
25 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.6 milestone Sep 20, 2023
@sbueringer sbueringer deleted the pr-update-go-backport-policy branch September 20, 2023 08:05
@sbueringer
Copy link
Member Author

/cherry-pick release-1.5

for the book

@k8s-infra-cherrypick-robot

@sbueringer: new pull request created: #9470

In response to this:

/cherry-pick release-1.5

for the book

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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/documentation Issues or PRs related to documentation 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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Discussion: Bumping Go in older Cluster API releases
10 participants