From ef355d035fcc9f6a0b243aa0c9ae371300a7e983 Mon Sep 17 00:00:00 2001 From: everettraven Date: Mon, 8 Jan 2024 16:04:31 -0500 Subject: [PATCH] do some cleanup Signed-off-by: everettraven --- internal/catalogmetadata/client/client.go | 3 - .../clusterextension_controller.go | 83 ++++++++++++++----- .../variablesources/required_package_test.go | 40 +++++++++ 3 files changed, 103 insertions(+), 23 deletions(-) diff --git a/internal/catalogmetadata/client/client.go b/internal/catalogmetadata/client/client.go index af6506990..5ac01cff6 100644 --- a/internal/catalogmetadata/client/client.go +++ b/internal/catalogmetadata/client/client.go @@ -128,9 +128,6 @@ func PopulateExtraFields(catalogName string, channels []*catalogmetadata.Channel } } - // if package is deprecated, add deprecation to bundle - // if channel is deprecated, add deprecation to bundle - // if bundle is deprecated, add deprecation to bundle for i := range bundles { for _, dep := range deprecations { if bundles[i].Package == dep.Package { diff --git a/internal/controllers/clusterextension_controller.go b/internal/controllers/clusterextension_controller.go index 2eed22e8c..77443353e 100644 --- a/internal/controllers/clusterextension_controller.go +++ b/internal/controllers/clusterextension_controller.go @@ -28,7 +28,6 @@ import ( "github.com/operator-framework/operator-registry/alpha/declcfg" rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1" "k8s.io/apimachinery/pkg/api/equality" - "k8s.io/apimachinery/pkg/api/meta" apimeta "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -42,7 +41,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" - "github.com/operator-framework/operator-controller/api/v1alpha1" ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" "github.com/operator-framework/operator-controller/internal/catalogmetadata" "github.com/operator-framework/operator-controller/internal/controllers/validators" @@ -134,6 +132,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp // hasn't been attempted yet, due to the spec being invalid. ext.Status.ResolvedBundleResource = "" setResolvedStatusConditionUnknown(&ext.Status.Conditions, "validation has not been attempted as spec is invalid", ext.GetGeneration()) + + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as spec is invalid", ext.GetGeneration()) return ctrl.Result{}, nil } @@ -144,6 +144,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted due to failure to gather data for resolution", ext.GetGeneration()) ext.Status.ResolvedBundleResource = "" setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted due to failure to gather data for resolution", ext.GetGeneration()) return ctrl.Result{}, err } @@ -154,6 +156,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) ext.Status.ResolvedBundleResource = "" setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) return ctrl.Result{}, err } @@ -165,6 +169,8 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) ext.Status.ResolvedBundleResource = "" setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) return ctrl.Result{}, err } @@ -172,14 +178,18 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp ext.Status.ResolvedBundleResource = bundle.Image setResolvedStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("resolved to %q", bundle.Image), ext.GetGeneration()) + // TODO: Question - Should we set the deprecation statuses after we have successfully resolved instead of after a successful installation? + mediaType, err := bundle.MediaType() if err != nil { setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) return ctrl.Result{}, err } bundleProvisioner, err := mapBundleMediaTypeToBundleProvisioner(mediaType) if err != nil { setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) return ctrl.Result{}, err } // Ensure a BundleDeployment exists with its bundle source from the bundle @@ -189,6 +199,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp // originally Reason: ocv1alpha1.ReasonInstallationFailed ext.Status.InstalledBundleResource = "" setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) return ctrl.Result{}, err } @@ -198,6 +209,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp // originally Reason: ocv1alpha1.ReasonInstallationStatusUnknown ext.Status.InstalledBundleResource = "" setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) + setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration()) return ctrl.Result{}, err } @@ -275,12 +287,24 @@ func mapBDStatusToInstalledCondition(existingTypedBundleDeployment *rukpakv1alph // setDeprecationStatus will set the appropriate deprecation statuses for a ClusterExtension // based on the provided bundle -func setDeprecationStatus(ext *v1alpha1.ClusterExtension, bundle *catalogmetadata.Bundle) { - // unset conditions - meta.RemoveStatusCondition(&ext.Status.Conditions, v1alpha1.TypePackageDeprecated) - meta.RemoveStatusCondition(&ext.Status.Conditions, v1alpha1.TypeChannelDeprecated) - meta.RemoveStatusCondition(&ext.Status.Conditions, v1alpha1.TypeBundleDeprecated) - meta.RemoveStatusCondition(&ext.Status.Conditions, v1alpha1.TypeDeprecated) +func setDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetadata.Bundle) { + // reset conditions to false + conditionTypes := []string{ + ocv1alpha1.TypeDeprecated, + ocv1alpha1.TypePackageDeprecated, + ocv1alpha1.TypeChannelDeprecated, + ocv1alpha1.TypeBundleDeprecated, + } + + for _, conditionType := range conditionTypes { + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: conditionType, + Reason: ocv1alpha1.ReasonDeprecated, + Status: metav1.ConditionFalse, + Message: "", + ObservedGeneration: ext.Generation, + }) + } if !bundle.HasDeprecation() { return @@ -291,9 +315,9 @@ func setDeprecationStatus(ext *v1alpha1.ClusterExtension, bundle *catalogmetadat for _, deprecation := range bundle.Deprecations { switch deprecation.Reference.Schema { case declcfg.SchemaPackage: - meta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ - Type: v1alpha1.TypePackageDeprecated, - Reason: v1alpha1.ReasonDeprecated, + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypePackageDeprecated, + Reason: ocv1alpha1.ReasonDeprecated, Status: metav1.ConditionTrue, Message: deprecation.Message, ObservedGeneration: ext.Generation, @@ -303,17 +327,17 @@ func setDeprecationStatus(ext *v1alpha1.ClusterExtension, bundle *catalogmetadat continue } - meta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ - Type: v1alpha1.TypeChannelDeprecated, - Reason: v1alpha1.ReasonDeprecated, + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeChannelDeprecated, + Reason: ocv1alpha1.ReasonDeprecated, Status: metav1.ConditionTrue, Message: deprecation.Message, ObservedGeneration: ext.Generation, }) case declcfg.SchemaBundle: - meta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ - Type: v1alpha1.TypeBundleDeprecated, - Reason: v1alpha1.ReasonDeprecated, + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeBundleDeprecated, + Reason: ocv1alpha1.ReasonDeprecated, Status: metav1.ConditionTrue, Message: deprecation.Message, ObservedGeneration: ext.Generation, @@ -323,9 +347,9 @@ func setDeprecationStatus(ext *v1alpha1.ClusterExtension, bundle *catalogmetadat deprecationMessages = append(deprecationMessages, deprecation.Message) } - meta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ - Type: v1alpha1.TypeDeprecated, - Reason: v1alpha1.ReasonDeprecated, + apimeta.SetStatusCondition(&ext.Status.Conditions, metav1.Condition{ + Type: ocv1alpha1.TypeDeprecated, + Reason: ocv1alpha1.ReasonDeprecated, Status: metav1.ConditionTrue, Message: strings.Join(deprecationMessages, ";"), ObservedGeneration: ext.Generation, @@ -522,6 +546,25 @@ func setInstalledStatusConditionUnknown(conditions *[]metav1.Condition, message }) } +func setDeprecationStatusesUnknown(conditions *[]metav1.Condition, message string, generation int64) { + conditionTypes := []string{ + ocv1alpha1.TypeDeprecated, + ocv1alpha1.TypePackageDeprecated, + ocv1alpha1.TypeChannelDeprecated, + ocv1alpha1.TypeBundleDeprecated, + } + + for _, conditionType := range conditionTypes { + apimeta.SetStatusCondition(conditions, metav1.Condition{ + Type: conditionType, + Reason: ocv1alpha1.ReasonDeprecated, + Status: metav1.ConditionUnknown, + Message: message, + ObservedGeneration: generation, + }) + } +} + // Generate reconcile requests for all cluster extensions affected by a catalog change func clusterExtensionRequestsForCatalog(c client.Reader, logger logr.Logger) handler.MapFunc { return func(ctx context.Context, _ client.Object) []reconcile.Request { diff --git a/internal/resolution/variablesources/required_package_test.go b/internal/resolution/variablesources/required_package_test.go index bb9be0d89..164f9a411 100644 --- a/internal/resolution/variablesources/required_package_test.go +++ b/internal/resolution/variablesources/required_package_test.go @@ -82,6 +82,44 @@ func TestMakeRequiredPackageVariables(t *testing.T) { }, }, }, + "test-package.v4.1.0": { + Bundle: declcfg.Bundle{ + Name: "test-package.v4.1.0", + Package: "test-package", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "4.1.0"}`)}, + }, + }, + InChannels: []*catalogmetadata.Channel{&stableChannel}, + Deprecations: []declcfg.DeprecationEntry{ + { + Reference: declcfg.PackageScopedReference{ + Schema: declcfg.SchemaBundle, + Name: "test-package.v4.1.0", + }, + Message: "test-package.v4.1.0 has been deprecated", + }, + }, + }, + "test-package.v5.0.0": { + Bundle: declcfg.Bundle{ + Name: "test-package.v5.0.0", + Package: "test-package", + Properties: []property.Property{ + {Type: property.TypePackage, Value: json.RawMessage(`{"packageName": "test-package", "version": "5.0.0"}`)}, + }, + }, + InChannels: []*catalogmetadata.Channel{&stableChannel}, + Deprecations: []declcfg.DeprecationEntry{ + { + Reference: declcfg.PackageScopedReference{ + Schema: declcfg.SchemaBundle, + Name: "test-package.v5.0.0", + }, + Message: "test-package.v5.0.0 has been deprecated", + }, + }, + }, // We need at least one bundle from different package // to make sure that we are filtering it out. @@ -130,6 +168,8 @@ func TestMakeRequiredPackageVariables(t *testing.T) { bundleSet["test-package.v3.0.0"], bundleSet["test-package.v2.0.0"], bundleSet["test-package.v1.0.0"], + bundleSet["test-package.v5.0.0"], + bundleSet["test-package.v4.1.0"], bundleSet["test-package.v4.0.0"], }), },