Merge pull request #189 from dana-team/refactor/changeErr

refactor: change error handling and message formatting

internal/common/validators.go

@@ -220,14 +220,14 @@ func ValidatePermittedGroups(ctx context.Context, user string, k8sClient client.
 			inGroup, err := CheckGroup(ctx, user, groupName, k8sClient)
 			if err != nil {
 				if errors.IsNotFound(err) {
-					logger.Info(fmt.Sprintf("group %s not found", groupName))
+					logger.Info(fmt.Sprintf("group %q not found", groupName))
 				} else {
 					logger.Error(err, "failed checking if user in group")
 					return false, nil
 				}
 			}
 			if inGroup {
-				logger.Info(fmt.Sprintf("user %s found in group %s", user, groupName))
+				logger.Info(fmt.Sprintf("user %q found in group %q", user, groupName))
 				return true, nil
 			}
 		}

internal/migrationhierarchy/related.go

@@ -23,28 +23,28 @@ import (
 func (r *MigrationHierarchyReconciler) updateRelatedObjects(mhObject, toNS, ns *objectcontext.ObjectContext) error {
 	ctx := mhObject.Ctx
 
-	if er := r.UpdateNSBasedOnParent(ctx, toNS, ns); er != nil {
-		err := r.updateMHStatus(mhObject, danav1.Error, er.Error())
-		if err != nil {
-			return fmt.Errorf("failed updating the status of object %q: "+err.Error(), mhObject.Name())
+	if err := r.UpdateNSBasedOnParent(ctx, toNS, ns); err != nil {
+		updateErr := updateMHStatus(mhObject, danav1.Error, err.Error())
+		if updateErr != nil {
+			return updateErr
 		}
-		return fmt.Errorf("failed updating the labels and annotations of namespace %q according to its parent %q: "+er.Error(), ns.Name(), toNS.Name())
+		return fmt.Errorf("failed updating the labels and annotations of namespace %q according to its parent %q: %v", ns.Name(), toNS.Name(), err.Error())
 	}
 
-	if er := r.UpdateAllNSChildrenOfNs(ctx, ns); er != nil {
-		err := r.updateMHStatus(mhObject, danav1.Error, er.Error())
-		if err != nil {
-			return fmt.Errorf("failed updating the status of object %q: "+err.Error(), mhObject.Name())
+	if err := r.UpdateAllNSChildrenOfNs(ctx, ns); err != nil {
+		updateErr := updateMHStatus(mhObject, danav1.Error, err.Error())
+		if updateErr != nil {
+			return updateErr
 		}
-		return fmt.Errorf("failed updating labels and annotations of child namespaces of sunamespace %q: "+er.Error(), ns.Name())
+		return fmt.Errorf("failed updating labels and annotations of child namespaces of sunamespace %q: %v", ns.Name(), err.Error())
 	}
 
-	if er := r.updateRole(toNS, danav1.NoRole); er != nil {
-		err := r.updateMHStatus(mhObject, danav1.Error, er.Error())
-		if err != nil {
-			return fmt.Errorf("failed updating the status of object %q: "+err.Error(), mhObject.Name())
+	if err := r.updateRole(toNS, danav1.NoRole); err != nil {
+		updateErr := updateMHStatus(mhObject, danav1.Error, err.Error())
+		if updateErr != nil {
+			return updateErr
 		}
-		return fmt.Errorf("failed updating role of subnamespace %q: "+er.Error(), toNS.Name())
+		return fmt.Errorf("failed updating role of subnamespace %q: %v", toNS.Name(), err.Error())
 	}
 
 	return nil

internal/migrationhierarchy/resources.go

@@ -18,11 +18,11 @@ import (
 func increaseRootResources(mhObject *objectcontext.ObjectContext, rootNSName string, sourceResources corev1.ResourceQuotaSpec) error {
 	rootNSQuotaObj, err := quota.RootNSObjectFromName(mhObject, rootNSName)
 	if err != nil {
-		return fmt.Errorf("failed to get root namespace quota object: " + err.Error())
+		return fmt.Errorf("failed to get root namespace quota object: %v", err.Error())
 	}
 
 	if err := addRootQuota(rootNSQuotaObj, sourceResources); err != nil {
-		return fmt.Errorf("failed to add resources to root namespace quota object" + err.Error())
+		return fmt.Errorf("failed to add resources to root namespace quota object: %v", err.Error())
 	}
 
 	return nil
@@ -33,11 +33,11 @@ func increaseRootResources(mhObject *objectcontext.ObjectContext, rootNSName str
 func decreaseRootResources(mhObject *objectcontext.ObjectContext, rootNSName string, sourceResources corev1.ResourceQuotaSpec) error {
 	rootNSQuotaObj, err := quota.RootNSObjectFromName(mhObject, rootNSName)
 	if err != nil {
-		return fmt.Errorf("failed to get root namespace quota object: " + err.Error())
+		return fmt.Errorf("failed to get root namespace quota object: %v", err.Error())
 	}
 
 	if err := subRootQuota(rootNSQuotaObj, sourceResources); err != nil {
-		return fmt.Errorf("failed to subtract resources from root namespace quota object" + err.Error())
+		return fmt.Errorf("failed to subtract resources from root namespace quota object: %v", err.Error())
 	}
 
 	return nil
@@ -85,7 +85,7 @@ func createMigrationUPQ(mhObject *objectcontext.ObjectContext, sourceResources c
 	mhName := mhObject.Name()
 
 	if err := createUpdateQuota(mhObject, mhName, sourceNS, destNS, sourceResources); err != nil {
-		return fmt.Errorf("failed to create updateQuota for migration %q", mhObject.Name())
+		return fmt.Errorf("failed to create updateQuota for migration %q: %v", mhObject.Name(), err.Error())
 	}
 
 	return nil
@@ -96,7 +96,7 @@ func createMigrationUPQ(mhObject *objectcontext.ObjectContext, sourceResources c
 func monitorMigrationUPQ(mhObject *objectcontext.ObjectContext, ns string) (ctrl.Result, error) {
 	upqPhase, err := getUpdateQuotaStatus(mhObject, ns)
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed getting updateQuota object status %q: "+err.Error(), mhObject.Name())
+		return ctrl.Result{}, fmt.Errorf("failed getting updateQuota object status %q: %v", mhObject.Name(), err.Error())
 	}
 
 	if upqPhase == danav1.Error {
@@ -129,7 +129,7 @@ func getUpdateQuotaStatus(mhObject *objectcontext.ObjectContext, upqNamespace st
 	upqObj, err := objectcontext.New(mhObject.Ctx, mhObject.Client, client.ObjectKey{Name: upqName, Namespace: upqNamespace}, &danav1.Updatequota{})
 
 	if err != nil {
-		return "", fmt.Errorf("failed getting updatequota object %q: "+err.Error(), upqName)
+		return "", fmt.Errorf("failed getting updatequota object %q: %v", upqName, err.Error())
 	}
 
 	return upqObj.Object.(*danav1.Updatequota).Status.Phase, nil

internal/namespace/cleanup.go

@@ -28,34 +28,34 @@ func (r *NamespaceReconciler) cleanUp(ctx context.Context, nsObject *objectconte
 	nsName := nsObject.Name()
 
 	if err := r.deleteNamespaceFromNamespaceDB(nsName); err != nil {
-		return fmt.Errorf("failed to delete namespace %q from namespacedb: "+err.Error(), nsName)
+		return fmt.Errorf("failed to delete namespace %q from namespacedb: %v", nsName, err.Error())
 	}
 	logger.Info("successfully deleted namespace from namespacedb", "namespace", nsName)
 
 	if err := deleteNamespaceQuotaObject(nsObject); err != nil {
-		return fmt.Errorf("failed to delete quota object of namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to delete quota object of namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully deleted quota object of namespace", "namespace", nsName)
 
 	if err := deleteSNSFromParentNS(nsName, nsObject); err != nil {
-		return fmt.Errorf("failed to delete subnamespace object '%s' from parent namespace: "+err.Error(), nsName)
+		return fmt.Errorf("failed to delete subnamespace object %q from parent namespace: %v", nsName, err.Error())
 	}
 	logger.Info("successfully deleted subnamespace object from parent namespace", "namespace", nsName)
 
 	if err := deleteNSHnsView(nsObject); err != nil {
-		return fmt.Errorf("failed to delete role and rolebinding objects associated with namespace '%s': "+err.Error(), nsName)
+		return fmt.Errorf("failed to delete role and rolebinding objects associated with namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully deleted role and rolebinding objects associated with namespace", "namespace", nsName)
 
 	// trigger reconciliation for parent subnamespace so that it can be aware of
 	// potential changes in one of its children
 	if err := r.enqueueParentSNSEvent(nsObject); err != nil {
-		return fmt.Errorf("failed to trigger sns event for parent of namespace '%s': "+err.Error(), nsName)
+		return fmt.Errorf("failed to trigger sns event for parent of namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully triggered sns event for parent of namespace", "namespace", nsName)
 
 	if err := deleteNSFinalizer(nsObject); err != nil {
-		return fmt.Errorf("failed to delete finalizer of namespace '%s': "+err.Error(), nsName)
+		return fmt.Errorf("failed to delete finalizer of namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully deleted finalizer of namespace", "namespace", nsName)
 
@@ -108,7 +108,7 @@ func (r *NamespaceReconciler) deleteNamespaceFromNamespaceDB(nsName string) erro
 			r.NamespaceDB.DeleteKey(keyNS)
 		} else {
 			if err := r.NamespaceDB.RemoveNS(nsName, keyNS); err != nil {
-				return fmt.Errorf("failed to remove namespace '%s' from key in DB: "+err.Error(), nsName)
+				return fmt.Errorf("failed to remove namespace %q from key in DB: %v", nsName, err.Error())
 			}
 		}
 	}

internal/namespace/controller.go

@@ -67,7 +67,7 @@ func (r *NamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	nsObject, err := objectcontext.New(ctx, r.Client, req.NamespacedName, &corev1.Namespace{})
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to get object %q: "+err.Error(), nsObject.Name())
+		return ctrl.Result{}, fmt.Errorf("failed to get object %q: %v", req.NamespacedName, err.Error())
 	}
 
 	if !nsObject.IsPresent() {

internal/namespace/init.go

@@ -24,17 +24,17 @@ func (r *NamespaceReconciler) init(nsObject *objectcontext.ObjectContext) error
 	nsName := nsObject.Name()
 
 	if err := addNSFinalizer(nsObject); err != nil {
-		return fmt.Errorf("failed to add finalizer for namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to add finalizer for namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully added finalizer of namespace", "namespace", nsName)
 
 	if err := rbutils.CreateHNSView(nsObject); err != nil {
-		return fmt.Errorf("failed to create role and roleBinding objects associated with namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to create role and roleBinding objects associated with namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully created role and roleBinding objects associated with namespace", "namespace", nsName)
 
 	if err := createParentRoleBindingsInNS(nsObject); err != nil {
-		return fmt.Errorf("failed to create parent roleBindings objects in namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to create parent roleBindings objects in namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully created parent roleBinding objects in namespace", "namespace", nsName)
 

internal/namespace/sync.go

@@ -25,26 +25,26 @@ func (r *NamespaceReconciler) sync(nsObject *objectcontext.ObjectContext) error
 	nsName := nsObject.Name()
 
 	if err := rbutils.CreateHNSView(nsObject); err != nil {
-		return fmt.Errorf("failed to create role and roleBinding objects associated with namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to create role and roleBinding objects associated with namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully created role and roleBinding objects associated with namespace", "namespace", nsName)
 
 	if nsutils.IsChildless(nsObject) {
 		if err := updateNSRole(nsObject, danav1.Leaf); err != nil {
-			return fmt.Errorf("failed to update role of namespace %q: "+err.Error(), nsName)
+			return fmt.Errorf("failed to update role of namespace %q: %v", nsName, err.Error())
 		}
 	} else if err := updateNSRole(nsObject, danav1.NoRole); err != nil {
-		return fmt.Errorf("failed to update role of namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to update role of namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully updated role of namespace", "namespace", nsName)
 
 	if err := ensureHierarchyLabels(nsObject); err != nil {
-		return fmt.Errorf("failed to set hierarchy labels of namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to set hierarchy labels of namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully set hierarchy labels of namespace", "namespace", nsName)
 
 	if err := ensureChildrenSNSResourcePoolLabel(nsObject); err != nil {
-		return fmt.Errorf("failed to set ResourcePool labels of children subnamespaces of namespace %q: "+err.Error(), nsName)
+		return fmt.Errorf("failed to set ResourcePool labels of children subnamespaces of namespace %q: %v", nsName, err.Error())
 	}
 	logger.Info("successfully set ResourcePool labels of children subnamespaces of namespace", "namespace", nsName)
 

internal/namespacedb/namespacedb.go

@@ -77,7 +77,7 @@ func Init(scheme *runtime.Scheme, logger logr.Logger) (*NamespaceDB, error) {
 	for _, ns := range nsWithSns.Items {
 		if ok := ns.Annotations[danav1.Role] == danav1.Leaf; ok {
 			if err := addHierarchy(ns, nsList, nDB, rootNS); err != nil {
-				return nDB, fmt.Errorf("failed to add hierarchy for %q: "+err.Error(), ns.Name)
+				return nDB, fmt.Errorf("failed to add hierarchy for %q: %v", ns.Name, err.Error())
 			}
 			logger.Info("successfully added hierarchy", "namespace", ns.Name)
 		}
@@ -123,14 +123,14 @@ func addHierarchy(ns corev1.Namespace, nsList corev1.NamespaceList, ndb *Namespa
 
 		if !ndb.doesKeyExist(keyName) {
 			if err := ndb.addNSToKey(keyName, keyName); err != nil {
-				return fmt.Errorf("failed to add namespace %q to key %q: "+err.Error(), keyName, keyName)
+				return fmt.Errorf("failed to add namespace %q to key %q: %v", keyName, keyName, err.Error())
 			}
 		}
 
 		for _, namespace := range nsListUp[rqDepth+1:] {
 			if !ndb.valInKeyExist(keyName, namespace.GetName()) {
 				if err := ndb.addNSToKey(keyName, namespace.GetName()); err != nil {
-					return fmt.Errorf("failed to add namespace %q to key %q: "+err.Error(), namespace.GetName(), keyName)
+					return fmt.Errorf("failed to add namespace %q to key %q: %v", namespace.GetName(), keyName, err.Error())
 				}
 			}
 		}
@@ -215,7 +215,7 @@ func AddNS(ctx context.Context, nDB *NamespaceDB, client client.Client, sns *dan
 
 	if !isKeyEmpty(keyNS) {
 		if err := nDB.addNSToKey(keyNS, sns.Name); err != nil {
-			return fmt.Errorf("failed to add namespace %q to key '%s': "+err.Error(), sns.Name, keyNS)
+			return fmt.Errorf("failed to add namespace %q to key %q: %v", sns.Name, keyNS, err.Error())
 		}
 		logger.Info("added namespace under key in namespacedb", "namespace", sns.Name, "key", keyNS)
 		return nil
@@ -232,7 +232,7 @@ func AddNS(ctx context.Context, nDB *NamespaceDB, client client.Client, sns *dan
 	// if a key does not already exist for the namespace, but the namespace has a CRQ then it means
 	// that the namespace itself should be the key in the DB.
 	if err := nDB.addNSToKey(sns.Name, sns.Name); err != nil {
-		return fmt.Errorf("failed to add namespace '%s' to key '%s': "+err.Error(), sns.Name, sns.Name)
+		return fmt.Errorf("failed to add namespace %q to key %q: %v", sns.Name, sns.Name, err.Error())
 	}
 	logger.Info("added namespace under key in namespacedb", "namespace", sns.Name, "key", sns.Name)
 
@@ -257,13 +257,13 @@ func MigrateNSHierarchy(ctx context.Context, ndb *NamespaceDB, client client.Cli
 
 		if !isKeyEmpty(oldKeyNS) && oldKeyNS != snsName {
 			if err := ndb.RemoveNS(childName, oldKeyNS); err != nil {
-				return fmt.Errorf("removing namespace '%s' from key '%s' failed: "+err.Error(), childName, oldKeyNS)
+				return fmt.Errorf("removing namespace %q from key %q failed: %v", childName, oldKeyNS, err.Error())
 			}
 		}
 
 		if !isKeyEmpty(newKeyNS) {
 			if err := ndb.addNSToKey(newKeyNS, childName); err != nil {
-				return fmt.Errorf("adding namespace '%s' to key '%s' failed: "+err.Error(), childName, newKeyNS)
+				return fmt.Errorf("adding namespace %q to key %q failed: %v", childName, newKeyNS, err.Error())
 			}
 		}
 	}
@@ -276,7 +276,7 @@ func (ndb *NamespaceDB) RemoveNS(nsname string, key string) error {
 	defer ndb.mutex.Unlock()
 
 	if _, ok := ndb.crqForest[key]; !ok {
-		return fmt.Errorf("key '%s' does not exist in NamespaceDB", key)
+		return fmt.Errorf("key %q does not exist in NamespaceDB", key)
 	}
 
 	for i, namespace := range ndb.crqForest[key] {

internal/rolebinding/cleanup.go

@@ -26,18 +26,18 @@ func (r *RoleBindingReconciler) cleanUp(rbObject *objectcontext.ObjectContext, s
 
 	if !rbutils.IsServiceAccount(rbObject.Object) {
 		if err := deleteSubjectsFromHNSViewClusterRoleBinding(rbObject); err != nil {
-			return fmt.Errorf("failed to delete subjects from roleBinding %q to HNS View ClusterRoleBinding: "+err.Error(), rbName)
+			return fmt.Errorf("failed to delete subjects from roleBinding %q to HNS View ClusterRoleBinding: %v", rbName, err.Error())
 		}
 		logger.Info("successfully deleted subjects from roleBinding to HNS View ClusterRoleBinding", "roleBinding", rbName)
 	}
 
 	if err := deleteRoleBindingsInSnsList(rbObject, snsList); err != nil {
-		return fmt.Errorf("failed to delete RoleBinding in every child of namespace %q: "+err.Error(), rbNamespace)
+		return fmt.Errorf("failed to delete RoleBinding in every child of namespace %q: %v", rbNamespace, err.Error())
 	}
 	logger.Info("successfully deleted RoleBinding in every child of namespace", "roleBinding namespace", rbNamespace)
 
 	if err := deleteRBFinalizer(rbObject); err != nil {
-		return fmt.Errorf("failed to delete finalizer to roleBinding %q: "+err.Error(), rbName)
+		return fmt.Errorf("failed to delete finalizer to roleBinding %q: %v", rbName, err.Error())
 	}
 	logger.Info("successfully deleted finalizer to roleBinding", "roleBinding", rbName)
 

internal/rolebinding/controller.go

@@ -70,7 +70,7 @@ func (r *RoleBindingReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	rbObject, err := objectcontext.New(ctx, r.Client, req.NamespacedName, &rbacv1.RoleBinding{})
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to get object %q: "+err.Error(), rbObject.Name())
+		return ctrl.Result{}, fmt.Errorf("failed to get object %q: %v", req.NamespacedName, err.Error())
 	}
 
 	if !rbObject.IsPresent() {
@@ -85,7 +85,7 @@ func (r *RoleBindingReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	snsList, err := objectcontext.NewList(ctx, r.Client, &danav1.SubnamespaceList{}, client.InNamespace(req.Namespace))
 	if err != nil {
-		return ctrl.Result{}, fmt.Errorf("failed to get list of subnamespaces in namespace %q: "+err.Error(), req.Namespace)
+		return ctrl.Result{}, fmt.Errorf("failed to get list of subnamespaces in namespace %q: %v", req.Namespace, err.Error())
 	}
 
 	isBeingDeleted := common.DeletionTimeStampExists(rbObject.Object)

internal/rolebinding/init.go

@@ -26,19 +26,19 @@ func (r *RoleBindingReconciler) init(rbObject *objectcontext.ObjectContext, snsL
 
 	if !DoesRBFinalizerExist(rbObject.Object) {
 		if err := addRBFinalizer(rbObject); err != nil {
-			return fmt.Errorf("failed to add finalizer to roleBinding %q: "+err.Error(), rbName)
+			return fmt.Errorf("failed to add finalizer to roleBinding %q: %v", rbName, err.Error())
 		}
 		logger.Info("successfully added finalizer to roleBinding", "roleBinding", rbName)
 	}
 
 	if err := createRoleBindingsInSNSList(rbObject, snsList); err != nil {
-		return fmt.Errorf("failed to create RoleBinding in every child of namespace %q: "+err.Error(), rbNamespace)
+		return fmt.Errorf("failed to create RoleBinding in every child of namespace %q: %v", rbNamespace, err.Error())
 	}
 	logger.Info("successfully created RoleBinding in every child of namespace", "roleBinding namespace", rbNamespace)
 
 	if !rbutils.IsServiceAccount(rbObject.Object) {
 		if err := addSubjectsToHNSViewClusterRoleBinding(rbObject); err != nil {
-			return fmt.Errorf("failed to add subjects from roleBinding %q to HNS View ClusterRoleBinding: "+err.Error(), rbName)
+			return fmt.Errorf("failed to add subjects from roleBinding %q to HNS View ClusterRoleBinding: %v", rbName, err.Error())
 		}
 		logger.Info("successfully added subjects from roleBinding to HNS View ClusterRoleBinding", "roleBinding", rbName)
 	}