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

🌱 Bump golang to v1.22 #1671

Merged
merged 2 commits into from
Jun 3, 2024
Merged

🌱 Bump golang to v1.22 #1671

merged 2 commits into from
Jun 3, 2024

Conversation

kashifest
Copy link
Member

@kashifest kashifest commented Apr 16, 2024

This PR bumps the golang version to v1.22 in different go modules, tests and Dockerfile.

@metal3-io-bot metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 16, 2024
@kashifest
Copy link
Member Author

/hold
This requires more discussion and project infra changes, this PR should not go in now until we have decided to bump the golang to v1.22

@metal3-io-bot metal3-io-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 16, 2024
@kashifest
Copy link
Member Author

@dtantsur @honza @zaneb when do you think we would be ready to bump golang to v1.22 for main branch in BMO, once we decide I will push changes here and in other metal3 repo's main branches accordingly

@kashifest
Copy link
Member Author

/retest

@tuminoid
Copy link
Member

/retest
Checking github action retriggering via prow.

@tuminoid
Copy link
Member

/ok-to-test

@metal3-io-bot metal3-io-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label May 16, 2024
@tuminoid
Copy link
Member

/retest

@dtantsur
Copy link
Member

Do we have a need to bump it? Doing so complicates local development for everyone who is not on the bleeding edge like me.

$ go version
go version go1.21.9 linux/amd64

We're also going to force the bump on any downstream consumers, whether they're ready or not.

@tuminoid
Copy link
Member

Golang 1.21 end of support date is 2024-08-08, so we need to bump to it sooner or later. Since we need to maintain our releases for roughly a year, we'll need to bump to it release branches as well (we did the same for 1.20 -> 1.21 too when it went EOS).

So we might hold off for a while, but we need to bump it before next minor anyways. The later we do it, the less testing it gets, so there is trade-off.

@tuminoid
Copy link
Member

Also, we already have dependencies we cannot bump, because they have moved to 1.22, like metal3-io/cluster-api-provider-metal3#1617
There will be more and more the closer we move to EOS.

@kashifest
Copy link
Member Author

Yes, I agree with @tuminoid , the sooner we do it, the better, projects like k8s itself, CAPI etc have already bumped it to 1.22 in main branch, I was waiting for BMO to be ready to make it in all metal3 main branches where applicable.

@metal3-io-bot metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label May 24, 2024
@dtantsur
Copy link
Member

Golang 1.21 end of support date is 2024-08-08, so we need to bump to it sooner or later.

I'm not following the logic. I thought the directive in go.mod only states the minimum version and users are welcome to use newer ones? My objection holds: we're pushing the change even on users of distributions that will maintain Go beyond its EOS date.

If/when we're forced to make the change because of a dependency bump, it's fine by me.

@tuminoid
Copy link
Member

Golang 1.21 end of support date is 2024-08-08, so we need to bump to it sooner or later.

I'm not following the logic. I thought the directive in go.mod only states the minimum version and users are welcome to use newer ones? My objection holds: we're pushing the change even on users of distributions that will maintain Go beyond its EOS date.

If/when we're forced to make the change because of a dependency bump, it's fine by me.

You're correct, go.mod bump is not strictly necessary. However, when Go 1.21 reaches EOS, is there an actual reason to keep it? Users should be using Go 1.22 at that point and it is only 2 months away. We do have some deps requiring it lined up, so it will be soon at least partially forced on us as well, so IMO it is cleaner to also switch directive to go 1.22 right away when we merge.

@Rozzii
Copy link
Member

Rozzii commented May 28, 2024

The lack of go 1.22 in the workflows blocks this PR #1751
If we want to continue towards the gopehercloud v2 as I would suppose from the fact that we are using the beta5 version of V2, we would anyhow need to transition to Go 1.22

@dtantsur
Copy link
Member

is there an actual reason to keep it?

The distributions don't move so quickly as we'd wish. Also, bumping Go versions should not be taken easily: I've just spent a whole morning on an enormous patch to move one of our components to Go 1.22 because the old controller-runtime segfaulted on it.

If we're forced by gophercloud, so be it. But I don't want such bumps (in go.mod specifically, not in workflows) to become a standard practice.

@kashifest
Copy link
Member Author

kashifest commented May 30, 2024

looks like the failure in E2E fixture test was related to the test itself, it needed to setup the correct go version, so my worry about fixing the go patch version in toolchain doesnt seem to be valid which is good.

@tuminoid
Copy link
Member

/retest

@kashifest
Copy link
Member Author

/test metal3-bmo-e2e-test-pull

@Rozzii
Copy link
Member

Rozzii commented May 31, 2024

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label May 31, 2024
@kashifest
Copy link
Member Author

/hold cancel
/cc @dtantsur @lentzi90 @honza

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 3, 2024
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

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

/approve

@metal3-io-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lentzi90

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2024
@metal3-io-bot metal3-io-bot merged commit 7cb4063 into metal3-io:main Jun 3, 2024
18 checks passed
@metal3-io-bot metal3-io-bot deleted the bump/golang branch June 3, 2024 07:58
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. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants