-
Notifications
You must be signed in to change notification settings - Fork 917
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 Go version to 1.22.11 #6066
Conversation
Welcome @y1hao! It looks like this is your first PR to karmada-io/karmada 🎉 |
@@ -37,7 +37,7 @@ KARMADA_METRICS_ADAPTER_LABEL="karmada-metrics-adapter" | |||
|
|||
KARMADA_GO_PACKAGE="github.com/karmada-io/karmada" | |||
|
|||
MIN_Go_VERSION=go1.22.9 | |||
MIN_Go_VERSION=go1.22.11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The casing is a bit weird - should it be
MIN_GO_VERSION
? Maybe I can change it in a separate PR? - Is this version always in sync with the versions in
.go-version
andgo.mod
?How about just reading it fromActually, I think this can work:.go-version
?MIN_GO_VERSION="go$(go list -m -f {{.GoVersion}})"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both are great to me! Thanks.
The 2 failing unit tests in CI in |
Signed-off-by: Yihao Wang <[email protected]>
Yes, I think so. https://github.com/karmada-io/karmada/actions/runs/12851418469/job/35832193075?pr=6066
Let's give it another try. |
I can reproduce it on my side with Go 1.22.11, but it works fine with Go v1.22.09. The error shows there is something wrong with the certification verification:
I'm still exploring it... |
Looks like x509 does change because of the security patch: https://go-review.googlesource.com/c/go/+/643105/2/src/crypto/x509/verify.go I can take another look later today after work. |
Yeah, I'm looking at this change now. Just share something I found so far.
|
Signed-off-by: Yihao Wang <[email protected]>
Looks like the test cert used by the test server has been regenerated: This is on 16 Jan. I updated the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #6066 +/- ##
=======================================
Coverage 48.33% 48.34%
=======================================
Files 666 666
Lines 54858 54858
=======================================
+ Hits 26516 26519 +3
+ Misses 26617 26615 -2
+ Partials 1725 1724 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Unit tests have passed with the certificate updates. I think there's a better fix for this. For test that don't spin up a test server, any cert can work so no updates needed. For the two test cases that use the test server, however, we should retrieve the cert from the test server, something like this: s := http.NewTLSTestServer( //...
caPEM := new(bytes.Buffer)
pem.Encode(caPEM, &pem.Block{
Type: "CERTIFICATE",
Bytes: s.Certificate().Raw,
})
// ...
clusterv1alpha1.SecretCADataKey: caPEM.Bytes() But this is a bit too complex for a cherry-pick commit, so I prefer to make this change in a separate PR, and only do it in master. |
That makes 100% sense to me! Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: RainbowMango 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 |
By the way, I updated the PR description a little bit, so that it won't close the referencing issue as we still need to backport the changes. |
…stream-release-1.12 Automated cherry pick of #6066: Bump Go version to 1.22.11
…stream-release-1.11 Automated cherry pick of #6066: Bump Go version to 1.22.11
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Bump Go version to latest 1.22 release.
Which issue(s) this PR fixes:
Part of #6063
Special notes for your reviewer:
Does this PR introduce a user-facing change?: