reduce complexity of reconcile method

- reduce cyclomatic complexity from 8 to 5
- make it more obvious when and how conditions are set when various
  errors occur
- when errors occur: always set ready condition, always return the error

Author: joelanford

api/v1alpha1/operator_types.go

@@ -17,8 +17,9 @@ limitations under the License.
 package v1alpha1
 
 import (
-	operatorutil "github.com/operator-framework/operator-controller/internal/util"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	operatorutil "github.com/operator-framework/operator-controller/internal/util"
 )
 
 // OperatorSpec defines the desired state of Operator
@@ -32,9 +33,10 @@ const (
 	// TODO(user): add more Types, here and into init()
 	TypeReady = "Ready"
 
-	ReasonNotImplemented      = "NotImplemented"
-	ReasonResolutionFailed    = "ResolutionFailed"
-	ReasonResolutionSucceeded = "ResolutionSucceeded"
+	ReasonResolutionSucceeded    = "ResolutionSucceeded"
+	ReasonResolutionFailed       = "ResolutionFailed"
+	ReasonBundleLookupFailed     = "BundleLookupFailed"
+	ReasonBundleDeploymentFailed = "BundleDeploymentFailed"
 )
 
 func init() {
@@ -44,7 +46,10 @@ func init() {
 	)
 	// TODO(user): add Reasons from above
 	operatorutil.ConditionReasons = append(operatorutil.ConditionReasons,
-		ReasonNotImplemented, ReasonResolutionSucceeded, ReasonResolutionFailed,
+		ReasonResolutionSucceeded,
+		ReasonResolutionFailed,
+		ReasonBundleLookupFailed,
+		ReasonBundleDeploymentFailed,
 	)
 }
 

controllers/operator_controller.go

@@ -18,7 +18,9 @@ package controllers
 
 import (
 	"context"
+	"fmt"
 
+	"github.com/operator-framework/deppy/pkg/deppy/solver"
 	rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
 	"k8s.io/apimachinery/pkg/api/equality"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
@@ -35,6 +37,7 @@ import (
 	operatorsv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
 	"github.com/operator-framework/operator-controller/internal/resolution"
 	"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/bundles_and_dependencies"
+	"github.com/operator-framework/operator-controller/internal/resolution/variable_sources/entity"
 )
 
 // OperatorReconciler reconciles a Operator object
@@ -105,54 +108,87 @@ func checkForUnexpectedFieldChange(a, b operatorsv1alpha1.Operator) bool {
 
 // Helper function to do the actual reconcile
 func (r *OperatorReconciler) reconcile(ctx context.Context, op *operatorsv1alpha1.Operator) (ctrl.Result, error) {
-	// define condition parameters
-	var status = metav1.ConditionTrue
-	var reason = operatorsv1alpha1.ReasonResolutionSucceeded
-	var message = "resolution was successful"
-
 	// run resolution
 	solution, err := r.Resolver.Resolve(ctx)
 	if err != nil {
-		status = metav1.ConditionFalse
-		reason = operatorsv1alpha1.ReasonResolutionFailed
-		message = err.Error()
-	} else {
-		// extract package bundle path from resolved variable
-		for _, variable := range solution.SelectedVariables() {
-			switch v := variable.(type) {
-			case *bundles_and_dependencies.BundleVariable:
-				packageName, err := v.BundleEntity().PackageName()
-				if err != nil {
-					return ctrl.Result{}, err
-				}
-				if packageName == op.Spec.PackageName {
-					bundlePath, err := v.BundleEntity().BundlePath()
-					if err != nil {
-						return ctrl.Result{}, err
-					}
-					// Create bundleDeployment if not found or Update if needed
-					dep := r.generateExpectedBundleDeployment(*op, bundlePath)
-					if err := r.ensureBundleDeployment(ctx, dep); err != nil {
-						return ctrl.Result{}, err
-					}
-					break
-				}
-			}
-		}
+		apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
+			Type:               operatorsv1alpha1.TypeReady,
+			Status:             metav1.ConditionFalse,
+			Reason:             operatorsv1alpha1.ReasonResolutionFailed,
+			Message:            err.Error(),
+			ObservedGeneration: op.GetGeneration(),
+		})
+		return ctrl.Result{}, err
+	}
+
+	// lookup the bundle entity in the solution that corresponds to the
+	// Operator's desired package name.
+	bundleEntity, err := r.getBundleEntityFromSolution(solution, op.Spec.PackageName)
+	if err != nil {
+		apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
+			Type:               operatorsv1alpha1.TypeReady,
+			Status:             metav1.ConditionFalse,
+			Reason:             operatorsv1alpha1.ReasonBundleLookupFailed,
+			Message:            err.Error(),
+			ObservedGeneration: op.GetGeneration(),
+		})
+		return ctrl.Result{}, err
+	}
+
+	// Get the bundle image reference for the bundle
+	bundleImage, err := bundleEntity.BundlePath()
+	if err != nil {
+		apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
+			Type:               operatorsv1alpha1.TypeReady,
+			Status:             metav1.ConditionFalse,
+			Reason:             operatorsv1alpha1.ReasonBundleLookupFailed,
+			Message:            err.Error(),
+			ObservedGeneration: op.GetGeneration(),
+		})
+		return ctrl.Result{}, err
+	}
+
+	// Ensure a BundleDeployment exists with its bundle source from the bundle
+	// image we just looked up in the solution.
+	dep := r.generateExpectedBundleDeployment(*op, bundleImage)
+	if err := r.ensureBundleDeployment(ctx, dep); err != nil {
+		apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
+			Type:               operatorsv1alpha1.TypeReady,
+			Status:             metav1.ConditionFalse,
+			Reason:             operatorsv1alpha1.ReasonBundleDeploymentFailed,
+			Message:            err.Error(),
+			ObservedGeneration: op.GetGeneration(),
+		})
+		return ctrl.Result{}, err
 	}
 
 	// update operator status
 	apimeta.SetStatusCondition(&op.Status.Conditions, metav1.Condition{
 		Type:               operatorsv1alpha1.TypeReady,
-		Status:             status,
-		Reason:             reason,
-		Message:            message,
+		Status:             metav1.ConditionTrue,
+		Reason:             operatorsv1alpha1.ReasonResolutionSucceeded,
+		Message:            "resolution was successful",
 		ObservedGeneration: op.GetGeneration(),
 	})
-
 	return ctrl.Result{}, nil
 }
 
+func (r *OperatorReconciler) getBundleEntityFromSolution(solution *solver.Solution, packageName string) (*entity.BundleEntity, error) {
+	for _, variable := range solution.SelectedVariables() {
+		switch v := variable.(type) {
+		case *bundles_and_dependencies.BundleVariable:
+			entityPkgName, err := v.BundleEntity().PackageName()
+			if err != nil {
+				return nil, err
+			}
+			if packageName == entityPkgName {
+				return v.BundleEntity(), nil
+			}
+		}
+	}
+	return nil, fmt.Errorf("entity for package %q not found in solution", packageName)
+}
+
 func (r *OperatorReconciler) generateExpectedBundleDeployment(o operatorsv1alpha1.Operator, bundlePath string) *unstructured.Unstructured {
 	// We use unstructured here to avoid problems of serializing default values when sending patches to the apiserver.
 	// If you use a typed object, any default values from that struct get serialized into the JSON patch, which could

controllers/operator_controller_test.go

@@ -67,8 +67,7 @@ var _ = Describe("Reconcile Test", func() {
 				By("running reconcile")
 				res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
 				Expect(res).To(Equal(ctrl.Result{}))
-				// TODO: should resolution failure return an error?
-				Expect(err).NotTo(HaveOccurred())
+				Expect(err).To(MatchError(fmt.Sprintf("package '%s' not found", pkgName)))
 
 				By("fetching updated operator after reconcile")
 				Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
@@ -243,7 +242,7 @@ var _ = Describe("Reconcile Test", func() {
 				err := cl.Create(ctx, operator)
 				Expect(err).NotTo(HaveOccurred())
 			})
-			PIt("sets resolution failure status and returns an error", func() {
+			It("sets resolution failure status and returns an error", func() {
 				By("running reconcile")
 				res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
 				Expect(res).To(Equal(ctrl.Result{}))
@@ -253,7 +252,11 @@ var _ = Describe("Reconcile Test", func() {
 				Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())
 
 				By("checking the expected conditions")
-				// TODO: there should be a condition update that sets Ready false in this scenario
+				cond := apimeta.FindStatusCondition(operator.Status.Conditions, operatorsv1alpha1.TypeReady)
+				Expect(cond).NotTo(BeNil())
+				Expect(cond.Status).To(Equal(metav1.ConditionFalse))
+				Expect(cond.Reason).To(Equal(operatorsv1alpha1.ReasonBundleLookupFailed))
+				Expect(cond.Message).To(ContainSubstring(`error determining bundle path for entity`))
 			})
 		})
 		When("the operator specifies a duplicate package", func() {
@@ -277,8 +280,7 @@ var _ = Describe("Reconcile Test", func() {
 				By("running reconcile")
 				res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: opKey})
 				Expect(res).To(Equal(ctrl.Result{}))
-				// TODO: should this scenario return an error?
-				Expect(err).NotTo(HaveOccurred())
+				Expect(err).To(MatchError(Equal(`duplicate identifier "required package prometheus" in input`)))
 
 				By("fetching updated operator after reconcile")
 				Expect(cl.Get(ctx, opKey, operator)).NotTo(HaveOccurred())