🐛 When infrastructureRef is nil, set InfrastructureReadyCondition to true #10909
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
/cherry-pick release-1.7 |
k8s-ci-robot
added
the
size/XS
|
@vincepri: once the present PR merges, I will cherry-pick it on top of release-1.7 in a new PR and assign it to you. 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-sigs/prow repository. |
vincepri
force-pushed
the
fix-condition-infraready2
branch
from
3b11062
to
e3dea4a
Compare
|
/test pull-cluster-api-e2e-main |
|
Change makes sense to me |
|
lgtm pending fixing the linter error |
…true Some clients, like MachineHealthCheck are checking this condition to continue operations. Set it for backward compatibility Signed-off-by: Vince Prignano <[email protected]>
vincepri
force-pushed
the
fix-condition-infraready2
branch
from
e3dea4a
to
4a0900c
Compare
k8s-ci-robot
added
size/M
| @@ -172,6 +172,50 @@ func TestClusterReconciler(t *testing.T) { | |||
| }, timeout).Should(BeTrue()) | |||
| }) | |||
|
|
|||
| t.Run("Should successfully patch a cluster object if the spec diff is empty but the status conditions diff is not", func(t *testing.T) { | |||
There was a problem hiding this comment.
Can you provide some context what use case this test is testing?
There was a problem hiding this comment.
This seems to be some other controller also patching InfrastructureReady to true in parallel to the cluster controller?
There was a problem hiding this comment.
I only added it for completeness, seems this really tests the patch helper?
There was a problem hiding this comment.
The same test, but for cluster.Status.InfrastructureReady = true is above this one
There was a problem hiding this comment.
Ah, I didn't realize this is ~ a copy of an existing test :D
I was wondering what use case you're trying to test here :)
| g.Eventually(func() bool { | ||
| ph, err := patch.NewHelper(cluster, env) | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| conditions.MarkTrue(cluster, clusterv1.InfrastructureReadyCondition) |
There was a problem hiding this comment.
Is this a diff? Isn't the cluster controller also setting this condition to true in reconcile?
There was a problem hiding this comment.
Is the controller running? Wouldn't the above patch also be the same?
There was a problem hiding this comment.
l.192 seems to suggest that the controller is running :)
There was a problem hiding this comment.
Ran both tests through the debugger. The patch here is literally a no-op (same in the pre-existing test)
|
Let's merge this PR to unblock the definitely good & useful change. Let's continue to discuss the unit test and then follow-up if necessart /lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 53ca4145c2927b93bbabd9117c975669fad07b99
|
|
[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
|
@vincepri: new pull request created: #10921 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-sigs/prow repository. |
… to true
Some clients, like MachineHealthCheck are checking this condition to continue operations. Set it for backward compatibility
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):Fixes #
/area api