Code review 1 changes

Signed-off-by: Xavi Garcia <xavi.garcia@suse.com>

Author: 0xavi0

internal/cmd/controller/reconciler/schedule_controller.go

@@ -273,7 +273,7 @@ func setClusterScheduled(ctx context.Context, c client.Client, name, namespace s
 	old := cluster.DeepCopy()
 	cluster.Status.Scheduled = scheduled
 
-	// when this function is called is either because we're updating a
+	// This function is called either because we're updating a
 	// Schedule or because we're creating it.
 	// In both cases ActiveSchedule should be false as a Schedule
 	// always begins in OffSchedule mode until the first start call is executed.
@@ -414,7 +414,6 @@ func setClustersScheduled(ctx context.Context, c client.Client, clusters []strin
 }
 
 func updateScheduledClusters(ctx context.Context, scheduler quartz.Scheduler, c client.Client, clustersNew []string, clustersOld []string, namespace string) error {
-	// first look for clusters that are not scheduled yet and flag them as scheduled
 	for _, cluster := range clustersNew {
 		if err := setClusterScheduled(ctx, c, cluster, namespace, true); err != nil {
 			return err

internal/cmd/controller/reconciler/schedule_controller_test.go

@@ -19,6 +19,12 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
+type expected struct {
+	scheduledJob         bool
+	statusScheduled      bool
+	statusActiveSchedule bool
+}
+
 var _ = Describe("ScheduleReconciler", func() {
 	var (
 		ctx        context.Context
@@ -305,10 +311,14 @@ var _ = Describe("ScheduleReconciler", func() {
 
 			// clusters 1, 2 and 3 should be scheduled in the quartz.Scheduler and also
 			// be flagged as Scheduled
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true, false)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, false)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true, false)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false, false)
+			checkState(scheduler, k8sclient, "test-cluster", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false})
+			checkState(scheduler, k8sclient, "test-cluster2", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false})
+			checkState(scheduler, k8sclient, "test-cluster3", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false})
+			checkState(scheduler, k8sclient, "test-cluster4", "default",
+				expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false})
 
 			// force the start of the schedule (so it sets .Status.ActiveSchedule=true)
 			jobKey := scheduleKey(schedule)
@@ -323,10 +333,14 @@ var _ = Describe("ScheduleReconciler", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			// check now that the clusters have the expected values, specially Status.ActiveSchedule
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", true, true, true)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, true)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", true, true, true)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", false, false, false)
+			checkState(scheduler, k8sclient, "test-cluster", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: true})
+			checkState(scheduler, k8sclient, "test-cluster2", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: true})
+			checkState(scheduler, k8sclient, "test-cluster3", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: true})
+			checkState(scheduler, k8sclient, "test-cluster4", "default",
+				expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false})
 
 			// update the schedule, now it only looks for the label foo=bar
 			scheduleUpdated := &fleet.Schedule{}
@@ -345,29 +359,33 @@ var _ = Describe("ScheduleReconciler", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			// cluster 2 and 4 should be still targeted.
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster", "default", false, false, false)
+			checkState(scheduler, k8sclient, "test-cluster", "default",
+				expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false})
 			// cluster 2 had Status.ActiveSchedule set to true, but because we updated the Schedule
 			// it should be back to false.
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster2", "default", true, true, false)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster3", "default", false, false, false)
-			checkClusterIsScheduled(scheduler, k8sclient, "test-cluster4", "default", true, true, false)
+			checkState(scheduler, k8sclient, "test-cluster2", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false})
+			checkState(scheduler, k8sclient, "test-cluster3", "default",
+				expected{scheduledJob: false, statusScheduled: false, statusActiveSchedule: false})
+			checkState(scheduler, k8sclient, "test-cluster4", "default",
+				expected{scheduledJob: true, statusScheduled: true, statusActiveSchedule: false})
 		})
 	})
 })
 
-func checkClusterIsScheduled(
+func checkState(
 	scheduler quartz.Scheduler,
 	k8sclient client.Client,
 	cluster, namespace string,
-	scheduledExpected, flaggedAsScheduledExpected, activeScheduleExpected bool) {
+	expectedState expected) {
 	isScheduled, err := isClusterScheduled(scheduler, cluster, namespace)
 	Expect(err).NotTo(HaveOccurred())
-	Expect(isScheduled).To(Equal(scheduledExpected))
+	Expect(isScheduled).To(Equal(expectedState.scheduledJob))
 
 	key := client.ObjectKey{Name: cluster, Namespace: namespace}
 	clusterObj := &fleet.Cluster{}
 	err = k8sclient.Get(context.Background(), key, clusterObj)
 	Expect(err).NotTo(HaveOccurred())
-	Expect(clusterObj.Status.Scheduled).To(Equal(flaggedAsScheduledExpected))
-	Expect(clusterObj.Status.ActiveSchedule).To(Equal(activeScheduleExpected))
+	Expect(clusterObj.Status.Scheduled).To(Equal(expectedState.statusScheduled))
+	Expect(clusterObj.Status.ActiveSchedule).To(Equal(expectedState.statusActiveSchedule))
 }