Address reviews (#887)

Signed-off-by: Varsha Prasad Narsing <[email protected]>

Author: varshaprasad96

api/v1alpha1/clusterextension_types.go

@@ -127,8 +127,6 @@ func init() {
 		ReasonUpgradeFailed,
 		ReasonBundleLoadFailed,
 		ReasonErrorGettingClient,
-		// TODO: this reason is not being used in the reconciler, it will be removed
-		// when we fix the tests. Avoiding removal here, to reduce diffs.
 		ReasonInstallationStatusUnknown,
 	)
 }

cmd/manager/main.go

@@ -184,7 +184,6 @@ func main() {
 		Client:             cl,
 		ReleaseNamespace:   systemNamespace,
 		BundleProvider:     catalogClient,
-		Scheme:             mgr.GetScheme(),
 		ActionClientGetter: acg,
 		Unpacker:           unpacker,
 		Storage:            localStorage,

internal/catalogmetadata/types.go

@@ -86,7 +86,7 @@ func (b *Bundle) loadRequiredPackages() error {
 	if b.requiredPackages == nil {
 		requiredPackages, err := loadFromProps[PackageRequired](b, property.TypePackageRequired, false)
 		if err != nil {
-			return fmt.Errorf("error determining bundle required packages for bundle %q: %s", b.Bundle.Name, err)
+			return fmt.Errorf("error determining bundle required packages for bundle %q: %s", b.Name, err)
 		}
 		for i := range requiredPackages {
 			semverRange, err := bsemver.ParseRange(requiredPackages[i].PackageRequired.VersionRange)

internal/controllers/clusterextension_controller.go

@@ -89,7 +89,6 @@ type ClusterExtensionReconciler struct {
 	ActionClientGetter helmclient.ActionClientGetter
 	Storage            storage.Storage
 	Handler            handler.Handler
-	Scheme             *runtime.Scheme
 	dynamicWatchMutex  sync.RWMutex
 	dynamicWatchGVKs   map[schema.GroupVersionKind]struct{}
 	controller         crcontroller.Controller
@@ -225,11 +224,9 @@ func (r *ClusterExtensionReconciler) handleResolutionErrors(ext *ocv1alpha1.Clus
 */
 //nolint:unparam
 func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (ctrl.Result, error) {
-	l := log.FromContext(ctx).WithName("operator-controller")
 	// run resolution
 	bundle, err := r.resolve(ctx, *ext)
 	if err != nil {
-		l.V(1).Info("bundle resolve error", "error", err)
 		return r.handleResolutionErrors(ext, err)
 	}
 
@@ -243,8 +240,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 	if err != nil {
 		ext.Status.ResolvedBundle = nil
 		ext.Status.InstalledBundle = nil
-		setResolvedStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonResolutionFailed, err), ext.GetGeneration())
-		setInstalledStatusConditionFailed(&ext.Status.Conditions, fmt.Sprintf("%s:%v", ocv1alpha1.ReasonInstallationFailed, err), ext.Generation)
+		setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
+		setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.Generation)
 		return ctrl.Result{}, err
 	}
 
@@ -382,7 +379,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 			if !isWatched {
 				if err := r.controller.Watch(
 					source.Kind(r.cache, unstructuredObj),
-					crhandler.EnqueueRequestForOwner(r.Scheme, r.Client.RESTMapper(), ext, crhandler.OnlyControllerOwner()),
+					crhandler.EnqueueRequestForOwner(r.Scheme(), r.RESTMapper(), ext, crhandler.OnlyControllerOwner()),
 					helmpredicate.DependentPredicateFuncs()); err != nil {
 					return err
 				}
@@ -620,11 +617,6 @@ func mapOwneeToOwnerHandler(cl client.Client, log logr.Logger, owner client.Obje
 			log.Error(err, "map ownee to owner: lookup GVK for owner")
 			return nil
 		}
-		owneeGVK, err := apiutil.GVKForObject(obj, cl.Scheme())
-		if err != nil {
-			log.Error(err, "map ownee to owner: lookup GVK for ownee")
-			return nil
-		}
 
 		type ownerInfo struct {
 			key types.NamespacedName
@@ -650,17 +642,6 @@ func mapOwneeToOwnerHandler(cl client.Client, log logr.Logger, owner client.Obje
 		if oi == nil {
 			return nil
 		}
-
-		if err := cl.Get(ctx, oi.key, owner); client.IgnoreNotFound(err) != nil {
-			log.Info("map ownee to owner: get owner",
-				"ownee", client.ObjectKeyFromObject(obj),
-				"owneeKind", owneeGVK,
-				"owner", oi.key,
-				"ownerKind", oi.gvk,
-				"error", err.Error(),
-			)
-			return nil
-		}
 		return []reconcile.Request{{NamespacedName: oi.key}}
 	})
 }
@@ -715,7 +696,7 @@ func (r *ClusterExtensionReconciler) installedBundle(ctx context.Context, allBun
 		catalogfilter.InMastermindsSemverRange(vr),
 	))
 	if len(resultSet) == 0 {
-		return nil, fmt.Errorf("bundle %q for package %q not found in available catalogs but is currently installed via helm chart %q in namespace %q", release.Labels[labels.BundleNameKey], ext.Spec.PackageName, release.Name, release.Namespace)
+		return nil, fmt.Errorf("bundle %q for package %q not found in available catalogs but is currently installed in namespace %q", release.Labels[labels.BundleNameKey], ext.Spec.PackageName, release.Namespace)
 	}
 
 	sort.SliceStable(resultSet, func(i, j int) bool {

internal/controllers/suite_test.go

@@ -53,7 +53,6 @@ func newClientAndReconciler(t *testing.T) (client.Client, *controllers.ClusterEx
 	reconciler := &controllers.ClusterExtensionReconciler{
 		Client:             cl,
 		BundleProvider:     &fakeCatalogClient,
-		Scheme:             scheme.Scheme,
 		ActionClientGetter: acg,
 		Unpacker:           unp,
 	}

internal/labels/labels.go

@@ -5,7 +5,8 @@ const (
 	OwnerNameKey = "olm.operatorframework.io/owner-name"
 
 	// Helm Secret annotations use the regex `(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])?`
-	// to validate labels. Open to any suggestions, keeping this for now.
+	// to validate labels. Which is why a similar format as OwnerKindKey/OwnerNameKey
+	// cannot be used as they do not conform to the regex requirements.
 	PackageNameKey   = "olm_operatorframework_io_package_name"
 	BundleNameKey    = "olm_operatorframework_io_bundle_name"
 	BundleVersionKey = "olm_operatorframework_io_bundle_version"