Directly grab map values instead of using loop-clause variables when setting up federated sync controller tests. #47061
Conversation
…setting up federated sync controller tests. Go's loop-clause variables are allocated once and the items are copied to that variable while iterating through the loop. This means, these variables can't escape the scope since closures are bound to loop-clause variables whose value change during each iteration. Doing so would lead to undesired behavior. For more on this topic see: https://github.com/golang/go/wiki/CommonMistakes So in order to workaround this problem in sync controller e2e tests, we iterate through the map and copy the map value to a variable inside the loop before using it in closures. Fixes issue: kubernetes#47059
k8s-ci-robot
added
sig/federation
cncf-cla: yes
k8s-github-robot
added
approved
|
Superb @madhusudancs, you nailed the issue. During the ginkgo tree construction time (explained in onsi/ginkgo#168 (comment)) probably only one federated type is picked by the function closure. The fix looks good to me. Now after this PR all tests will start to fail i think, because of daemonset test failure :( |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: madhusudancs, shashidharatd Associated issue: 47059 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
@shashidharatd that's true. I am not entirely sure if I should be happy or sad about this fix :P
Agreed. I had a chat with @janetkuo. Updated the issue with the resolution - #46925 (comment). Please take a look and comment. |
|
@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this |
|
/priority failing-test |
k8s-ci-robot
added
the
kind/failing-test
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
Automatic merge from submit-queue (batch tested with PRs 46977, 47005, 47018, 47061, 46809) |
Go's loop-clause variables are allocated once and the items are copied to that variable while iterating through the loop. This means, these variables can't escape the scope since closures are bound to loop-clause variables whose value change during each iteration. Doing so would lead to undesired behavior. For more on this topic see: https://github.com/golang/go/wiki/CommonMistakes
So in order to workaround this problem in sync controller e2e tests, we iterate through the map and copy the map value to a variable inside the loop before using it in closures.
Fixes issue: #47059
Release note:
/assign @marun @shashidharatd @perotinus
cc @csbell @nikhiljindal
/sig federation