controller/workload: Clean up status reporting and test infrastructure

Replace inline error message loops with errors.Join, use ptr.Deref for
replicas. Add status.NewVersionGetter and podListErr support to tests,
remove unnecessary Template/Labels from test fixtures.

Author: tchap

pkg/operator/apiserver/controller/workload/workload.go

@@ -16,6 +16,7 @@ import (
 	corev1listers "k8s.io/client-go/listers/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/client-go/util/workqueue"
+	"k8s.io/utils/ptr"
 
 	operatorv1 "github.com/openshift/api/operator/v1"
 	applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"
@@ -218,8 +219,8 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 
 	if !preconditionsReady {
 		var message string
-		for _, err := range errs {
-			message = message + err.Error() + "\n"
+		if len(errs) > 0 {
+			message = errors.Join(errs...).Error()
 		}
 		if len(message) == 0 {
 			message = "the operator didn't specify what preconditions are missing"
@@ -249,14 +250,10 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 	}
 
 	if len(errs) > 0 {
-		message := ""
-		for _, err := range errs {
-			message = message + err.Error() + "\n"
-		}
 		workloadDegradedCondition = workloadDegradedCondition.
 			WithStatus(operatorv1.ConditionTrue).
 			WithReason("SyncError").
-			WithMessage(message)
+			WithMessage(errors.Join(errs...).Error())
 	} else if workload == nil {
 		workloadDegradedCondition = workloadDegradedCondition.
 			WithStatus(operatorv1.ConditionTrue).
@@ -298,10 +295,7 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o
 			WithReason("AsExpected")
 	}
 
-	desiredReplicas := int32(1)
-	if workload.Spec.Replicas != nil {
-		desiredReplicas = *(workload.Spec.Replicas)
-	}
+	desiredReplicas := ptr.Deref(workload.Spec.Replicas, 1)
 
 	// If the workload is up to date, then we are no longer progressing
 	workloadAtHighestGeneration := workload.ObjectMeta.Generation == workload.Status.ObservedGeneration

pkg/operator/apiserver/controller/workload/workload_test.go

@@ -15,6 +15,7 @@ import (
 	operatorv1 "github.com/openshift/api/operator/v1"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
+	"github.com/openshift/library-go/pkg/operator/status"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 	appsv1 "k8s.io/api/apps/v1"
 	"k8s.io/apimachinery/pkg/api/equality"
@@ -57,13 +58,15 @@ func TestUpdateOperatorStatus(t *testing.T) {
 
 		workload                        *appsv1.Deployment
 		pods                            []*corev1.Pod
+		podListErr                      error
 		operatorConfigAtHighestRevision bool
 		operatorPreconditionsNotReady   bool
 		preconditionError               error
 		errors                          []error
 		previousConditions              []operatorv1.OperatorCondition
 
-		validateOperatorStatus func(*operatorv1.OperatorStatus) error
+		validateOperatorStatus  func(*operatorv1.OperatorStatus) error
+		validateVersionRecorder func(status.VersionGetter) error
 	}{
 		{
 			name: "scenario: no workload, no errors thus we are degraded and we are progressing",
@@ -112,7 +115,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
 					{
 						Type:    fmt.Sprintf("%sWorkloadDegraded", defaultControllerName),
 						Status:  operatorv1.ConditionTrue,
-						Message: "nasty error\n",
+						Message: "nasty error",
 						Reason:  "SyncError",
 					},
 					{
@@ -140,7 +143,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
 					Namespace: "openshift-apiserver",
 				},
 				Spec: appsv1.DeploymentSpec{
-					Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
 					Replicas: ptr.To[int32](3),
 				},
 				Status: appsv1.DeploymentStatus{
@@ -152,7 +154,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
 			},
 			pods: []*corev1.Pod{
 				{
-					ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver", Labels: map[string]string{"foo": "bar"}},
+					ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver"},
 					Status: corev1.PodStatus{
 						Phase: corev1.PodPending,
 						ContainerStatuses: []corev1.ContainerStatus{
@@ -214,7 +216,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
 					Namespace: "openshift-apiserver",
 				},
 				Spec: appsv1.DeploymentSpec{
-					Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
 					Replicas: ptr.To[int32](3),
 				},
 				Status: appsv1.DeploymentStatus{
@@ -226,7 +227,7 @@ func TestUpdateOperatorStatus(t *testing.T) {
 			},
 			pods: []*corev1.Pod{
 				{
-					ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver", Labels: map[string]string{"foo": "bar"}},
+					ObjectMeta: metav1.ObjectMeta{Name: "apiserver", Namespace: "openshift-apiserver"},
 					Status: corev1.PodStatus{
 						Phase: corev1.PodPending,
 						ContainerStatuses: []corev1.ContainerStatus{
@@ -287,7 +288,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
 					Namespace: "openshift-apiserver",
 				},
 				Spec: appsv1.DeploymentSpec{
-					Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
 					Replicas: ptr.To[int32](3),
 				},
 				Status: appsv1.DeploymentStatus{
@@ -635,8 +635,6 @@ func TestUpdateOperatorStatus(t *testing.T) {
 					Namespace: "openshift-apiserver",
 				},
 				Spec: appsv1.DeploymentSpec{
-					Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}},
-					Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}},
 					Replicas: ptr.To[int32](3),
 				},
 				Status: appsv1.DeploymentStatus{
@@ -704,16 +702,20 @@ func TestUpdateOperatorStatus(t *testing.T) {
 				syncErrrors:             scenario.errors,
 			}
 
+			versionRecorder := status.NewVersionGetter()
+
 			// act
 			target := &Controller{
-				operatorClient:  fakeOperatorClient,
-				targetNamespace: targetNs,
-				podsLister:      &fakePodLister{pods: scenario.pods},
-				delegate:        delegate,
+				operatorClient:       fakeOperatorClient,
+				targetNamespace:      targetNs,
+				targetOperandVersion: "v1.0.0-test",
+				podsLister:           &fakePodLister{pods: scenario.pods, err: scenario.podListErr},
+				delegate:             delegate,
+				versionRecorder:      versionRecorder,
 			}
 
 			err := target.sync(context.TODO(), factory.NewSyncContext("workloadcontroller_test", events.NewInMemoryRecorder("workloadcontroller_test", clocktesting.NewFakePassiveClock(time.Now()))))
-			if err != nil && len(scenario.errors) == 0 {
+			if err != nil && len(scenario.errors) == 0 && scenario.podListErr == nil {
 				t.Fatal(err)
 			}
 
@@ -726,28 +728,34 @@ func TestUpdateOperatorStatus(t *testing.T) {
 			if err != nil {
 				t.Fatal(err)
 			}
+			if scenario.validateVersionRecorder != nil {
+				if err := scenario.validateVersionRecorder(versionRecorder); err != nil {
+					t.Fatal(err)
+				}
+			}
 		})
 	}
 }
 
 type fakePodLister struct {
 	pods []*corev1.Pod
+	err  error
 }
 
 type fakePodNamespaceLister struct {
 	lister *fakePodLister
 }
 
 func (f *fakePodNamespaceLister) List(selector labels.Selector) (ret []*corev1.Pod, err error) {
-	return f.lister.pods, nil
+	return f.lister.pods, f.lister.err
 }
 
 func (f *fakePodNamespaceLister) Get(name string) (*corev1.Pod, error) {
 	panic("implement me")
 }
 
 func (f *fakePodLister) List(selector labels.Selector) (ret []*corev1.Pod, err error) {
-	return f.pods, nil
+	return f.pods, f.err
 }
 
 func (f *fakePodLister) Pods(namespace string) corev1listers.PodNamespaceLister {