chore: follow up review comments from previous PR (#858)

* update: follow up comments

- cleanup commented out code
- rename function
- cleanup unnecessary sleep

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: add check on return err + remove apierrs.IsNotFound check

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* Update pkg/deploy/deploy.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

* update(review): create new function DeleteSubscription

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* update: return for get and delete subscription

- get: return 'sub, nil' or 'nil, err' here error can be real one or
notfound

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

* Update pkg/deploy/deploy.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

* fix(linter)

Signed-off-by: Wen Zhou <wenzhou@redhat.com>

---------

Signed-off-by: Wen Zhou <wenzhou@redhat.com>
Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

pkg/deploy/deploy.go

@@ -407,34 +407,44 @@ func ApplyParams(componentPath string, imageParamsMap map[string]string, isUpdat
 	return nil
 }
 
-// SubscriptionExists checks if a Subscription for the operator exists in the given namespace.
-// if exsit, return object; if not exsit, return nil.
-func SubscriptionExists(cli client.Client, namespace string, name string) (*ofapiv1alpha1.Subscription, error) {
+// GetSubscription checks if a Subscription for the operator exists in the given namespace.
+// if exist, return object; otherwise, return error.
+func GetSubscription(cli client.Client, namespace string, name string) (*ofapiv1alpha1.Subscription, error) {
 	sub := &ofapiv1alpha1.Subscription{}
 	if err := cli.Get(context.TODO(), client.ObjectKey{Namespace: namespace, Name: name}, sub); err != nil {
-		if apierrs.IsNotFound(err) {
-			return nil, nil
-		}
+		// real error or 'not found' both return here
 		return nil, err
 	}
-
 	return sub, nil
 }
 
+// Delete given Subscription if it exists
+// Do not error if the Subscription does not exist.
+func DeleteExistingSubscription(cli client.Client, operatorNs string, subsName string) error {
+	sub, err := GetSubscription(cli, operatorNs, subsName)
+	if err != nil {
+		return client.IgnoreNotFound(err)
+	}
+
+	if err := cli.Delete(context.TODO(), sub); client.IgnoreNotFound(err) != nil {
+		return fmt.Errorf("error deleting subscription %s: %w", sub.Name, err)
+	}
+
+	return nil
+}
+
 // OperatorExists checks if an Operator with 'operatorPrefix' is installed.
 // Return true if found it, false if not.
 // if we need to check exact version of the operator installed, can append vX.Y.Z later.
 func OperatorExists(cli client.Client, operatorPrefix string) (bool, error) {
 	opConditionList := &ofapiv2.OperatorConditionList{}
-	if err := cli.List(context.TODO(), opConditionList); err != nil {
-		if !apierrs.IsNotFound(err) { // real error to run List()
-			return false, err
-		}
-	} else {
-		for _, opCondition := range opConditionList.Items {
-			if strings.HasPrefix(opCondition.Name, operatorPrefix) {
-				return true, nil
-			}
+	err := cli.List(context.TODO(), opConditionList)
+	if err != nil {
+		return false, err
+	}
+	for _, opCondition := range opConditionList.Items {
+		if strings.HasPrefix(opCondition.Name, operatorPrefix) {
+			return true, nil
 		}
 	}
 

pkg/deploy/setup.go

@@ -62,9 +62,6 @@ func isManagedRHODS(cli client.Client) (Platform, error) {
 	expectedCatlogSource := &ofapi.CatalogSourceList{}
 	err = cli.List(context.TODO(), expectedCatlogSource)
 	if err != nil {
-		if apierrs.IsNotFound(err) {
-			return Unknown, nil
-		}
 		return Unknown, err
 	}
 	if len(expectedCatlogSource.Items) > 0 {

pkg/upgrade/upgrade.go

@@ -108,18 +108,11 @@ func OperatorUninstall(cli client.Client, cfg *rest.Config) error {
 	} else if platform == deploy.ManagedRhods {
 		subsName = "addon-managed-odh"
 	}
-	sub, _ := deploy.SubscriptionExists(cli, operatorNs, subsName)
-	if sub == nil {
-		fmt.Printf("Could not find subscription %s in namespace %s. Maybe you have a different one", subsName, operatorNs)
-	} else {
-		if err := cli.Delete(context.TODO(), sub); err != nil {
-			return fmt.Errorf("error deleting subscription %s: %w", sub.Name, err)
-		}
+	if err := deploy.DeleteExistingSubscription(cli, operatorNs, subsName); err != nil {
+		return err
 	}
 
 	fmt.Printf("Removing the operator CSV in turn remove operator deployment\n")
-	time.Sleep(5 * time.Second)
-
 	err = removeCSV(cli, cfg)
 
 	fmt.Printf("All resources deleted as part of uninstall.")
@@ -325,10 +318,6 @@ func UpdateFromLegacyVersion(cli client.Client, platform deploy.Platform, appNS
 		kfDefList := &kfdefv1.KfDefList{}
 		err := cli.List(context.TODO(), kfDefList)
 		if err != nil {
-			if apierrs.IsNotFound(err) {
-				// If no KfDefs, do nothing and return
-				return nil
-			}
 			return fmt.Errorf("error getting kfdef instances: : %w", err)
 		}
 		if len(kfDefList.Items) > 0 {
@@ -421,10 +410,6 @@ func RemoveKfDefInstances(cli client.Client, _ deploy.Platform) error {
 	expectedKfDefList := &kfdefv1.KfDefList{}
 	err = cli.List(context.TODO(), expectedKfDefList)
 	if err != nil {
-		if apierrs.IsNotFound(err) {
-			// If no KfDefs, do nothing and return
-			return nil
-		}
 		return fmt.Errorf("error getting list of kfdefs: %w", err)
 	}
 	// Delete kfdefs

tests/e2e/dsc_cfmap_deletion_test.go

@@ -46,11 +46,6 @@ func cfgMapDeletionTestSuite(t *testing.T) {
 			require.NoError(t, err, "Error while deleting owned namespaces")
 		})
 
-		// t.Run("DS Projects should be deleted", func(t *testing.T) {
-		// 	err = testCtx.testDSProjectDeletion()
-		// 	require.NoError(t, err, "Error while deleting DS Projectss")
-		// })
-
 		t.Run("dsci should be deleted", func(t *testing.T) {
 			err = testCtx.testDSCIDeletion()
 			require.NoError(t, err, "failed deleting DSCI")