review fixes

Author: chrischdi

internal/controllers/machinedeployment/machinedeployment_controller.go

@@ -313,7 +313,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, md *clusterv1.MachineD
 	for _, ms := range msList {
 		if ms.DeletionTimestamp.IsZero() {
 			log.Info("Deleting MachineSet", "MachineSet", klog.KObj(ms))
-			if err := r.Client.Delete(ctx, ms); err != nil {
+			if err := r.Client.Delete(ctx, ms); err != nil && !apierrors.IsNotFound(err) {
 				return errors.Wrapf(err, "failed to delete MachineSet %s", klog.KObj(ms))
 			}
 		}
@@ -335,7 +335,8 @@ func (r *Reconciler) getAndAdoptMachineSetsForDeployment(ctx context.Context, md
 	filtered := make([]*clusterv1.MachineSet, 0, len(machineSets.Items))
 	for idx := range machineSets.Items {
 		ms := &machineSets.Items[idx]
-		log.WithValues("MachineSet", klog.KObj(ms))
+		log := log.WithValues("MachineSet", klog.KObj(ms))
+		ctx := ctrl.LoggerInto(ctx, log)
 		selector, err := metav1.LabelSelectorAsSelector(&md.Spec.Selector)
 		if err != nil {
 			log.Error(err, "Skipping MachineSet, failed to get label selector from spec selector")

internal/controllers/machinedeployment/machinedeployment_controller_test.go

@@ -18,7 +18,6 @@ package machinedeployment
 
 import (
 	"context"
-	"sort"
 	"testing"
 	"time"
 
@@ -1001,12 +1000,15 @@ func TestReconciler_reconcileDelete(t *testing.T) {
 		"some": "labelselector",
 	}
 	md := builder.MachineDeployment("default", "md0").WithClusterName("test").Build()
+	md.Finalizers = []string{
+		clusterv1.MachineDeploymentFinalizer,
+	}
 	md.DeletionTimestamp = ptr.To(metav1.Now())
 	md.Spec.Selector = metav1.LabelSelector{
 		MatchLabels: labels,
 	}
 	mdWithoutFinalizer := md.DeepCopy()
-	mdWithoutFinalizer.Finalizers = nil
+	mdWithoutFinalizer.Finalizers = []string{}
 	tests := []struct {
 		name              string
 		machineDeployment *clusterv1.MachineDeployment
@@ -1057,7 +1059,6 @@ func TestReconciler_reconcileDelete(t *testing.T) {
 				Client:   c,
 				recorder: record.NewFakeRecorder(32),
 			}
-			// Mark machineDeployment to be in deletion.
 
 			err := r.reconcileDelete(ctx, tt.machineDeployment)
 			if tt.expectError {
@@ -1070,21 +1071,6 @@ func TestReconciler_reconcileDelete(t *testing.T) {
 
 			machineSetList := &clusterv1.MachineSetList{}
 			g.Expect(c.List(ctx, machineSetList, client.InNamespace("default"))).ToNot(HaveOccurred())
-
-			// Remove ResourceVersion so we can actually compare.
-			for i, m := range machineSetList.Items {
-				m.ResourceVersion = ""
-				machineSetList.Items[i] = m
-			}
-
-			sort.Slice(machineSetList.Items, func(i, j int) bool {
-				return machineSetList.Items[i].GetName() < machineSetList.Items[j].GetName()
-			})
-			sort.Slice(tt.wantMachineSets, func(i, j int) bool {
-				return tt.wantMachineSets[i].GetName() < tt.wantMachineSets[j].GetName()
-			})
-
-			g.Expect(machineSetList.Items).To(BeComparableTo(tt.wantMachineSets))
 		})
 	}
 }

internal/controllers/machineset/machineset_controller.go

@@ -348,7 +348,7 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, machineSet *clusterv1.
 	for _, machine := range machineList {
 		if machine.DeletionTimestamp.IsZero() {
 			log.Info("Deleting Machine", "Machine", klog.KObj(machine))
-			if err := r.Client.Delete(ctx, machine); err != nil {
+			if err := r.Client.Delete(ctx, machine); err != nil && !apierrors.IsNotFound(err) {
 				return errors.Wrapf(err, "failed to delete Machine %s", klog.KObj(machine))
 			}
 		}
@@ -381,6 +381,7 @@ func (r *Reconciler) getAndAdoptMachinesForMachineSet(ctx context.Context, machi
 	for idx := range allMachines.Items {
 		machine := &allMachines.Items[idx]
 		log := log.WithValues("Machine", klog.KObj(machine))
+		ctx := ctrl.LoggerInto(ctx, log)
 		if shouldExcludeMachine(machineSet, machine) {
 			continue
 		}

internal/controllers/machineset/machineset_controller_test.go

@@ -18,7 +18,6 @@ package machineset
 
 import (
 	"fmt"
-	"sort"
 	"testing"
 	"time"
 
@@ -2136,12 +2135,15 @@ func TestReconciler_reconcileDelete(t *testing.T) {
 		"some": "labelselector",
 	}
 	ms := builder.MachineSet("default", "ms0").WithClusterName("test").Build()
+	ms.Finalizers = []string{
+		clusterv1.MachineSetFinalizer,
+	}
 	ms.DeletionTimestamp = ptr.To(metav1.Now())
 	ms.Spec.Selector = metav1.LabelSelector{
 		MatchLabels: labels,
 	}
 	msWithoutFinalizer := ms.DeepCopy()
-	msWithoutFinalizer.Finalizers = nil
+	msWithoutFinalizer.Finalizers = []string{}
 	tests := []struct {
 		name         string
 		machineSet   *clusterv1.MachineSet
@@ -2207,19 +2209,11 @@ func TestReconciler_reconcileDelete(t *testing.T) {
 			g.Expect(c.List(ctx, machineList, client.InNamespace("default"))).ToNot(HaveOccurred())
 
 			// Remove ResourceVersion so we can actually compare.
-			for i, m := range machineList.Items {
-				m.ResourceVersion = ""
-				machineList.Items[i] = m
+			for i := range machineList.Items {
+				machineList.Items[i].ResourceVersion = ""
 			}
 
-			sort.Slice(machineList.Items, func(i, j int) bool {
-				return machineList.Items[i].GetName() < machineList.Items[j].GetName()
-			})
-			sort.Slice(tt.wantMachines, func(i, j int) bool {
-				return tt.wantMachines[i].GetName() < tt.wantMachines[j].GetName()
-			})
-
-			g.Expect(machineList.Items).To(BeComparableTo(tt.wantMachines))
+			g.Expect(machineList.Items).To(ConsistOf(tt.wantMachines))
 		})
 	}
 }

util/log/log.go

@@ -111,14 +111,25 @@ func getOwners(ctx context.Context, c client.Client, obj metav1.Object) ([]owner
 // five objects. On more than five objects it outputs the first five objects and
 // adds information about how much more are in the given list.
 func ObjNamesString[T client.Object](objs []T) string {
+	shortenedBy := 0
+	if len(objs) > 5 {
+		shortenedBy = len(objs) - 5
+		objs = objs[:5]
+	}
+
+	stringList := []string{}
+	for _, obj := range objs {
+		stringList = append(stringList, obj.GetName())
+	}
+
 	objNames := make([]string, len(objs))
 	for _, obj := range objs {
 		objNames = append(objNames, obj.GetName())
 	}
 
-	if len(objNames) <= 5 {
-		return strings.Join(objNames, ", ")
+	if shortenedBy > 0 {
+		stringList = append(stringList, fmt.Sprintf("... (%d more)", shortenedBy))
 	}
 
-	return fmt.Sprintf("%s and %d more", strings.Join(objNames[:5], ", "), len(objNames)-5)
+	return strings.Join(stringList, ", ")
 }