Clear (existing) error cond from Subscription, once error resolved (#3166)

The func `removeSubsCond` takes in a list of pointers to Subscription objects, modifies the
objects that the pointers point to, but return a new list of those pointers. A [PR](operator-framework/operator-lifecycle-manager#2942) included in
the v0.25.0 release [changed the way the output of that function was being used](https://github.com/operator-framework/operator-lifecycle-manager/pull/2942/files#diff-a1760d9b7ac1e93734eea675d8d8938c96a50e995434b163c6f77c91bace9990R1146-R1155) leading to a regression. This PR fixes the `removeSubsCond` function,
fixing the regression as a result.

Closes #3162

Signed-off-by: Anik Bhattacharjee <[email protected]>
Upstream-repository: operator-lifecycle-manager
Upstream-commit: 54da66a9996632315827ba37e14823de6405b4d9

Author: anik120

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator.go

@@ -1393,16 +1393,14 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 	}
 
 	// Make sure that we no longer indicate unpacking progress
-	subs = o.setSubsCond(subs, v1alpha1.SubscriptionCondition{
-		Type:   v1alpha1.SubscriptionBundleUnpacking,
-		Status: corev1.ConditionFalse,
-	})
+	o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpacking)
 
 	// Remove BundleUnpackFailed condition from subscriptions
 	o.removeSubsCond(subs, v1alpha1.SubscriptionBundleUnpackFailed)
 
 	// Remove resolutionfailed condition from subscriptions
 	o.removeSubsCond(subs, v1alpha1.SubscriptionResolutionFailed)
+
 	newSub := true
 	for _, updatedSub := range updatedSubs {
 		updatedSub.Status.RemoveConditions(v1alpha1.SubscriptionResolutionFailed)
@@ -1679,13 +1677,9 @@ func (o *Operator) setSubsCond(subs []*v1alpha1.Subscription, cond v1alpha1.Subs
 	return subList
 }
 
-// removeSubsCond will remove the condition to the subscription if it exists
-// Only return the list of updated subscriptions
-func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) []*v1alpha1.Subscription {
-	var (
-		lastUpdated = o.now()
-	)
-	var subList []*v1alpha1.Subscription
+// removeSubsCond removes the given condition from all of the subscriptions in the input
+func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alpha1.SubscriptionConditionType) {
+	lastUpdated := o.now()
 	for _, sub := range subs {
 		cond := sub.Status.GetCondition(condType)
 		// if status is ConditionUnknown, the condition doesn't exist. Just skip
@@ -1694,9 +1688,7 @@ func (o *Operator) removeSubsCond(subs []*v1alpha1.Subscription, condType v1alph
 		}
 		sub.Status.LastUpdated = lastUpdated
 		sub.Status.RemoveConditions(condType)
-		subList = append(subList, sub)
 	}
-	return subList
 }
 
 func (o *Operator) updateSubscriptionStatuses(subs []*v1alpha1.Subscription) ([]*v1alpha1.Subscription, error) {

staging/operator-lifecycle-manager/pkg/controller/operators/catalog/operator_test.go

@@ -1261,13 +1261,6 @@ func TestSyncResolvingNamespace(t *testing.T) {
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "",
 						State:      "",
-						Conditions: []v1alpha1.SubscriptionCondition{
-							{
-								Type:   v1alpha1.SubscriptionBundleUnpacking,
-								Status: corev1.ConditionFalse,
-							},
-						},
-						LastUpdated: now,
 					},
 				},
 			},
@@ -1399,6 +1392,62 @@ func TestSyncResolvingNamespace(t *testing.T) {
 			},
 			wantErr: fmt.Errorf("some error"),
 		},
+		{
+			name: "HadErrorShouldClearError",
+			fields: fields{
+				clientOptions: []clientfake.Option{clientfake.WithSelfLinks(t)},
+				existingOLMObjs: []runtime.Object{
+					&v1alpha1.Subscription{
+						TypeMeta: metav1.TypeMeta{
+							Kind:       v1alpha1.SubscriptionKind,
+							APIVersion: v1alpha1.SchemeGroupVersion.String(),
+						},
+						ObjectMeta: metav1.ObjectMeta{
+							Name:      "sub",
+							Namespace: testNamespace,
+						},
+						Spec: &v1alpha1.SubscriptionSpec{
+							CatalogSource:          "src",
+							CatalogSourceNamespace: testNamespace,
+						},
+						Status: v1alpha1.SubscriptionStatus{
+							InstalledCSV: "sub-csv",
+							State:        "AtLatestKnown",
+							Conditions: []v1alpha1.SubscriptionCondition{
+								{
+									Type:    v1alpha1.SubscriptionResolutionFailed,
+									Reason:  "ConstraintsNotSatisfiable",
+									Message: "constraints not satisfiable: no operators found from catalog src in namespace testNamespace referenced by subscrition sub, subscription sub exists",
+									Status:  corev1.ConditionTrue,
+								},
+							},
+						},
+					},
+				},
+				resolveErr: nil,
+			},
+			wantSubscriptions: []*v1alpha1.Subscription{
+				{
+					TypeMeta: metav1.TypeMeta{
+						Kind:       v1alpha1.SubscriptionKind,
+						APIVersion: v1alpha1.SchemeGroupVersion.String(),
+					},
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "sub",
+						Namespace: testNamespace,
+					},
+					Spec: &v1alpha1.SubscriptionSpec{
+						CatalogSource:          "src",
+						CatalogSourceNamespace: testNamespace,
+					},
+					Status: v1alpha1.SubscriptionStatus{
+						InstalledCSV: "sub-csv",
+						State:        "AtLatestKnown",
+						LastUpdated:  now,
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

staging/operator-lifecycle-manager/test/e2e/subscription_e2e_test.go

@@ -2445,7 +2445,7 @@ var _ = Describe("Subscription", func() {
 				},
 				5*time.Minute,
 				interval,
-			).Should(Equal(corev1.ConditionFalse))
+			).Should(Equal(corev1.ConditionUnknown))
 
 			By("verifying that the subscription is not reporting unpacking errors")
 			Eventually(