From 8b997dc9afbd17e21e7bd4d4a39a85c2ef53f24c Mon Sep 17 00:00:00 2001 From: Varsha Date: Thu, 23 May 2024 12:22:18 -0700 Subject: [PATCH] Address reviews (#887) Signed-off-by: Varsha Prasad Narsing --- api/v1alpha1/clusterextension_types.go | 2 -- cmd/manager/main.go | 1 - internal/catalogmetadata/types.go | 2 +- .../clusterextension_controller.go | 27 +++---------------- internal/controllers/suite_test.go | 1 - internal/labels/labels.go | 3 ++- 6 files changed, 7 insertions(+), 29 deletions(-) diff --git a/api/v1alpha1/clusterextension_types.go b/api/v1alpha1/clusterextension_types.go index 223c308b1..97578bbf8 100644 --- a/api/v1alpha1/clusterextension_types.go +++ b/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, ) } diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 9937145f0..09c7df940 100644 --- a/cmd/manager/main.go +++ b/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, diff --git a/internal/catalogmetadata/types.go b/internal/catalogmetadata/types.go index 1bc6359a7..1cf662ef9 100644 --- a/internal/catalogmetadata/types.go +++ b/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) diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 88d91f17d..80a682eb4 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/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 { diff --git a/internal/controllers/suite_test.go b/internal/controllers/suite_test.go index 4b8bbe17c..9f4bf8cde 100644 --- a/internal/controllers/suite_test.go +++ b/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, } diff --git a/internal/labels/labels.go b/internal/labels/labels.go index 5a4bf373d..033f44e1e 100644 --- a/internal/labels/labels.go +++ b/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"