Bump Go toolchain to v1.24.6 #5813
Conversation
k8s-ci-robot
added
the
release-note-none
k8s-ci-robot
added
kind/cleanup
k8s-ci-robot
added
the
size/XL
|
/hold because we may want to do the v1.21 release first |
k8s-ci-robot
added
the
do-not-merge/hold
|
@mboersma if this passes tests I'd be keen to include in 1.21 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5813 +/- ##
==========================================
- Coverage 53.07% 46.94% -6.14%
==========================================
Files 279 279
Lines 29687 29687
==========================================
- Hits 15757 13936 -1821
- Misses 13114 14938 +1824
+ Partials 816 813 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
1 similar comment
|
/retest |
|
Could we also bump the version here? diff --git a/cloudbuild.yaml b/cloudbuild.yaml
index 79dcdcec3..e584386e8 100644
--- a/cloudbuild.yaml
+++ b/cloudbuild.yaml
@@ -4,7 +4,7 @@ options:
substitution_option: ALLOW_LOOSE
machineType: 'E2_HIGHCPU_8'
steps:
- - name: 'gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250116-2a05ea7e3d' # Go 1.23
+ - name: 'gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250513-9264efb079' # Go 1.24
entrypoint: make
env:
- DOCKER_CLI_EXPERIMENTAL=enabledcurl -sL gcr.io/v2/k8s-staging-test-infra/gcb-docker-gcloud/tags/list | jq '.tags'% docker run --rm --entrypoint go gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20250513-9264efb079 version
go version go1.24.3 linux/amd64 |
There was a problem hiding this comment.
These couple comments are totally non-blocking. Would be nice to update the cloudbuild.yaml as part of this PR though.
And for next time I think it would help reviewing to split this into separate commits for a) bumping the Go version, and b) each logic group of linter issues fixed.
| cacheExpect: func(cache *mock_azure.MockCredentialCache) { | ||
| ctx := context.Background() | ||
| ctx := context.Background() //nolint:usetesting |
There was a problem hiding this comment.
I think ideally we refactor this function to take a context or *testing.T argument so we can satisfy the linter. Otherwise, moving this ctx to the top of TestGetTokenCredential also appeases the linter, in case either of those are worth doing now.
diff --git a/azure/scope/identity_test.go b/azure/scope/identity_test.go
index 669fe204e..bcaa4aabd 100644
--- a/azure/scope/identity_test.go
+++ b/azure/scope/identity_test.go
@@ -17,7 +17,6 @@ limitations under the License.
package scope
import (
- "context"
"os"
"reflect"
"testing"
@@ -208,6 +207,7 @@ func TestHasClientSecret(t *testing.T) {
}
func TestGetTokenCredential(t *testing.T) {
+ ctx := t.Context()
testCertPath := "../../test/setup/certificate"
tests := []struct {
@@ -439,7 +439,6 @@ func TestGetTokenCredential(t *testing.T) {
},
},
cacheExpect: func(cache *mock_azure.MockCredentialCache) {
- ctx := context.Background() //nolint:usetesting
credsPath := "../../test/setup/credentials.json" //nolint:gosec
clientOptions := azcore.ClientOptions{
Cloud: cloud.Configuration{There was a problem hiding this comment.
This test fails if we replace context.Background() with t.Context():
--- FAIL: TestGetTokenCredential (0.00s)
--- FAIL: TestGetTokenCredential/UserAssignedIdentityCredential (0.00s)
/Users/matt/projects/cluster-api-provider-azure/azure/scope/identity.go:169: Unexpected call to *mock_azure.MockCredentialCache.GetOrStoreUserAssignedManagedIdentityCredentials([context.Background ../../test/setup/credentials.json { {https://login.microsoftonline.com/ map[resourceManager:{https://management.core.windows.net/ https://management.azure.com}]} false {false [] []} {0 0s 0s 0s [] <nil>} { false} {0x104990dc0} <nil> [] []} &{0x14000452330 0}]) at /Users/matt/projects/cluster-api-provider-azure/azure/scope/identity.go:169 because:
expected call at /Users/matt/projects/cluster-api-provider-azure/azure/scope/identity_test.go:454 doesn't match the argument at index 0.
Got: context.Background (context.backgroundCtx)
Want: is equal to context.Background.WithCancel (*context.cancelCtx)
/Users/matt/projects/cluster-api-provider-azure/azure/scope/controller.go:97: missing call(s) to *mock_azure.MockCredentialCache.GetOrStoreUserAssignedManagedIdentityCredentials(is equal to context.Background.WithCancel (*context.cancelCtx), is equal to ../../test/setup/credentials.json (string), adheres to a custom condition, is anything) /Users/matt/projects/cluster-api-provider-azure/azure/scope/identity_test.go:454
/Users/matt/projects/cluster-api-provider-azure/azure/scope/controller.go:97: aborting test due to missing call(s)
FAIL
FAIL sigs.k8s.io/cluster-api-provider-azure/azure/scope 0.581s
FAILAnd I wasn't excited to debug why, so I thought this was an acceptable use of //nolint.
| mapper, err := AzureManagedClusterToAzureManagedMachinePoolsMapper(t.Context(), fakeClient, scheme, logr.New(sink)) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| requests := mapper(context.TODO(), &infrav1.AzureManagedCluster{ | ||
| requests := mapper(t.Context(), &infrav1.AzureManagedCluster{ |
There was a problem hiding this comment.
It would be great to eventually refactor the spots like this to define a root ctx in one place and pass that around instead of each reinstantiating a separate t.Context().
There was a problem hiding this comment.
That could be more explicit and coherent, but based on the implementation of testing.Context, I don't think there's any cost to the naive approach except a function call.
mboersma
force-pushed
the
bump-go-v1.24
branch
from
dc11b46 to
c2cf9c0
Compare
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 16659b5793f821f261aa8b0b377d06db20166357
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nojnhuh 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 |
k8s-ci-robot
added
the
approved
|
/retest |
|
@mboersma: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Updates the Go compiler and tools to v1.24.6.
I separated this out from #5661 because it's XL-sized on its own.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
TODOs:
Release note: