馃悰 Fix for TestServerSideApplyWithDefaulting #10307
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/XS
|
/area ci |
adilGhaffarDev
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/test pull-cluster-api-test-main |
2 similar comments
|
/test pull-cluster-api-test-main |
|
/test pull-cluster-api-test-main |
|
@fabriziopandini @chrischdi if this looks ok, can we merge this before today's release? I have run tests multiple times on this PR, I haven't seen any failure yet. |
| @@ -653,7 +653,7 @@ func TestServerSideApplyWithDefaulting(t *testing.T) { | |||
|
|
|||
| // Enable the new defaulting logic (i.e. simulate the Cluster API update). | |||
| // The webhook will default the users field to `[{Name: "default-user"}]`. | |||
| g.Expect(env.Create(ctx, mutatingWebhookConfiguration)).To(Succeed()) | |||
| g.Expect(env.CreateAndWait(ctx, mutatingWebhookConfiguration)).To(Succeed()) | |||
There was a problem hiding this comment.
Sorry but this doesn't make sense.
The test is failing in l.683 because the KubeadmConfigTemplate doesn't exist in the local cache. Here we are waiting for MutatingWebhookConfiguration.
It probably fixes the issue by accident but I would prefer waiting for the right object (which is kct)
There was a problem hiding this comment.
Let's add the following instead in l.652
// Let's wait until the initial KubeadmConfigTemplate is visible in the local cache. Otherwise the test fails below.
g.Eventually(ctx, func(g Gomega) {
g.Expect(env.Get(ctx, client.ObjectKeyFromObject(kct), &bootstrapv1.KubeadmConfigTemplate{})).To(Succeed())
}, 5*time.Second).Should(Succeed())There was a problem hiding this comment.
I have added the wait for kct too.
There was a problem hiding this comment.
Let's get rid of the CreateAndWait in l.661. We absolutely don't depend on mutatingWebhookConfiguration showing up in the local client cache
adilGhaffarDev
force-pushed
the
TestServerSideApplyWithDefaulting-fix/adil
branch
from
c16f2e2
to
885c5a5
Compare
Signed-off-by: muhammad adil ghaffar <muhammad.adil.ghaffar@est.tech>
adilGhaffarDev
force-pushed
the
TestServerSideApplyWithDefaulting-fix/adil
branch
from
885c5a5
to
6cb0a98
Compare
|
/test pull-cluster-api-test-main |
fabriziopandini
added
the
area/clusterclass
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: ac4382cac51f502e9d58d7f283e25ef7f904008c
|
|
/approve Thx! Feel free to cherry-pick if needed |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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
|
/cherry-pick release-1.6 |
|
@adilGhaffarDev: new pull request created: #10327 In response to this:
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. |
|
/cherry-pick release-1.5 |
|
/cherry-pick release-1.4 |
|
@adilGhaffarDev: new pull request created: #10344 In response to this:
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. |
|
@adilGhaffarDev: new pull request created: #10345 In response to this:
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. |
|
same failure is happening in release 1-5: https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/periodic-cluster-api-test-release-1-5/1773266012418871296 |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):possibly Fixes #10305