Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 When infrastructureRef is nil, set InfrastructureReadyCondition to true #10909

Merged
merged 1 commit into from
Jul 22, 2024
Merged

🐛 When infrastructureRef is nil, set InfrastructureReadyCondition to true #10909

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions internal/controllers/cluster/cluster_controller_phases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ func (r *Reconciler) reconcileInfrastructure(ctx context.Context, cluster *clust
if cluster.Spec.InfrastructureRef == nil {
// If the infrastructure ref is not set, marking the infrastructure as ready (no-op).
cluster.Status.InfrastructureReady = true
conditions.MarkTrue(cluster, clusterv1.InfrastructureReadyCondition)
return ctrl.Result{}, nil
}

Expand Down
1 change: 1 addition & 0 deletions internal/controllers/cluster/cluster_controller_phases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ func TestClusterReconcilePhases(t *testing.T) {
expectErr: false,
check: func(g *GomegaWithT, in *clusterv1.Cluster) {
g.Expect(in.Status.InfrastructureReady).To(BeTrue())
g.Expect(conditions.IsTrue(in, clusterv1.InfrastructureReadyCondition)).To(BeTrue())
},
},
{
Expand Down
44 changes: 44 additions & 0 deletions internal/controllers/cluster/cluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you provide some context what use case this test is testing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be some other controller also patching InfrastructureReady to true in parallel to the cluster controller?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only added it for completeness, seems this really tests the patch helper?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same test, but for cluster.Status.InfrastructureReady = true is above this one

Copy link
Member

@sbueringer sbueringer Jul 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 := NewWithT(t)

// Setup
cluster := &clusterv1.Cluster{
ObjectMeta: metav1.ObjectMeta{
GenerateName: "test3-",
Namespace: ns.Name,
},
}
g.Expect(env.Create(ctx, cluster)).To(Succeed())
key := client.ObjectKey{Name: cluster.Name, Namespace: cluster.Namespace}
defer func() {
err := env.Delete(ctx, cluster)
g.Expect(err).ToNot(HaveOccurred())
}()

// Wait for reconciliation to happen.
g.Eventually(func() bool {
if err := env.Get(ctx, key, cluster); err != nil {
return false
}
return len(cluster.Finalizers) > 0
}, timeout).Should(BeTrue())

// Patch
g.Eventually(func() bool {
ph, err := patch.NewHelper(cluster, env)
g.Expect(err).ToNot(HaveOccurred())
conditions.MarkTrue(cluster, clusterv1.InfrastructureReadyCondition)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a diff? Isn't the cluster controller also setting this condition to true in reconcile?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the controller running? Wouldn't the above patch also be the same?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l.192 seems to suggest that the controller is running :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ran both tests through the debugger. The patch here is literally a no-op (same in the pre-existing test)

g.Expect(ph.Patch(ctx, cluster, patch.WithStatusObservedGeneration{})).To(Succeed())
return true
}, timeout).Should(BeTrue())

// Assertions
g.Eventually(func() bool {
instance := &clusterv1.Cluster{}
if err := env.Get(ctx, key, instance); err != nil {
return false
}
return conditions.IsTrue(cluster, clusterv1.InfrastructureReadyCondition)
}, timeout).Should(BeTrue())
})

t.Run("Should successfully patch a cluster object if both the spec diff and status diff are non empty", func(t *testing.T) {
g := NewWithT(t)

Expand Down
Loading