chore: enable gocritic (argoproj#680)

Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>

Author: mmorel-35

.golangci.yaml

@@ -34,15 +34,8 @@ linters-settings:
     disabled-checks:
       - appendAssign
       - assignOp # Keep it disabled for readability
-      - commentFormatting #FIXME
       - exitAfterDefer
-      - elseif #FIXME
-      - ifElseChain #FIXME
-      - mapKey
-      - singleCaseSwitch #FIXME
       - typeSwitchVar
-      - unlambda #FIXME
-      - wrapperFunc #FIXME
   goimports:
     local-prefixes: github.com/argoproj/gitops-engine
   importas:

pkg/cache/cluster_test.go

@@ -847,11 +847,12 @@ func ExampleNewClusterCache_resourceUpdatedEvents() {
 		panic(err)
 	}
 	unsubscribe := clusterCache.OnResourceUpdated(func(newRes *Resource, oldRes *Resource, _ map[kube.ResourceKey]*Resource) {
-		if newRes == nil {
+		switch {
+		case newRes == nil:
 			fmt.Printf("%s deleted\n", oldRes.Ref.String())
-		} else if oldRes == nil {
+		case oldRes == nil:
 			fmt.Printf("%s created\n", newRes.Ref.String())
-		} else {
+		default:
 			fmt.Printf("%s updated\n", newRes.Ref.String())
 		}
 	})
@@ -1378,7 +1379,7 @@ func BenchmarkIterateHierarchyV2(b *testing.B) {
 	}
 }
 
-//func BenchmarkIterateHierarchy(b *testing.B) {
+// func BenchmarkIterateHierarchy(b *testing.B) {
 //	cluster := newCluster(b)
 //	for _, resource := range testResources {
 //		cluster.setNode(resource)

pkg/cache/resource.go

@@ -92,10 +92,8 @@ func (r *Resource) iterateChildren(ns map[kube.ResourceKey]*Resource, parents ma
 			if parents[childKey] {
 				key := r.ResourceKey()
 				_ = action(fmt.Errorf("circular dependency detected. %s is child and parent of %s", childKey.String(), key.String()), child, ns)
-			} else {
-				if action(nil, child, ns) {
-					child.iterateChildren(ns, newResourceKeySet(parents, r.ResourceKey()), action)
-				}
+			} else if action(nil, child, ns) {
+				child.iterateChildren(ns, newResourceKeySet(parents, r.ResourceKey()), action)
 			}
 		}
 	}

pkg/diff/diff.go

@@ -116,15 +116,13 @@ func Diff(config, live *unstructured.Unstructured, opts ...Option) (*DiffResult,
 	orig, err := GetLastAppliedConfigAnnotation(live)
 	if err != nil {
 		o.log.V(1).Info(fmt.Sprintf("Failed to get last applied configuration: %v", err))
-	} else {
-		if orig != nil && config != nil {
-			Normalize(orig, opts...)
-			dr, err := ThreeWayDiff(orig, config, live)
-			if err == nil {
-				return dr, nil
-			}
-			o.log.V(1).Info(fmt.Sprintf("three-way diff calculation failed: %v. Falling back to two-way diff", err))
+	} else if orig != nil && config != nil {
+		Normalize(orig, opts...)
+		dr, err := ThreeWayDiff(orig, config, live)
+		if err == nil {
+			return dr, nil
 		}
+		o.log.V(1).Info(fmt.Sprintf("three-way diff calculation failed: %v. Falling back to two-way diff", err))
 	}
 	return TwoWayDiff(config, live)
 }
@@ -810,11 +808,12 @@ func Normalize(un *unstructured.Unstructured, opts ...Option) {
 	unstructured.RemoveNestedField(un.Object, "metadata", "creationTimestamp")
 
 	gvk := un.GroupVersionKind()
-	if gvk.Group == "" && gvk.Kind == "Secret" {
+	switch {
+	case gvk.Group == "" && gvk.Kind == "Secret":
 		NormalizeSecret(un, opts...)
-	} else if gvk.Group == "rbac.authorization.k8s.io" && (gvk.Kind == "ClusterRole" || gvk.Kind == "Role") {
+	case gvk.Group == "rbac.authorization.k8s.io" && (gvk.Kind == "ClusterRole" || gvk.Kind == "Role"):
 		normalizeRole(un, o)
-	} else if gvk.Group == "" && gvk.Kind == "Endpoints" {
+	case gvk.Group == "" && gvk.Kind == "Endpoints":
 		normalizeEndpoint(un, o)
 	}
 

pkg/health/health_deployment.go

@@ -35,22 +35,23 @@ func getAppsv1DeploymentHealth(deployment *appsv1.Deployment) (*HealthStatus, er
 	// Borrowed at kubernetes/kubectl/rollout_status.go https://github.com/kubernetes/kubernetes/blob/5232ad4a00ec93942d0b2c6359ee6cd1201b46bc/pkg/kubectl/rollout_status.go#L80
 	if deployment.Generation <= deployment.Status.ObservedGeneration {
 		cond := getAppsv1DeploymentCondition(deployment.Status, appsv1.DeploymentProgressing)
-		if cond != nil && cond.Reason == "ProgressDeadlineExceeded" {
+		switch {
+		case cond != nil && cond.Reason == "ProgressDeadlineExceeded":
 			return &HealthStatus{
 				Status:  HealthStatusDegraded,
 				Message: fmt.Sprintf("Deployment %q exceeded its progress deadline", deployment.Name),
 			}, nil
-		} else if deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas {
+		case deployment.Spec.Replicas != nil && deployment.Status.UpdatedReplicas < *deployment.Spec.Replicas:
 			return &HealthStatus{
 				Status:  HealthStatusProgressing,
 				Message: fmt.Sprintf("Waiting for rollout to finish: %d out of %d new replicas have been updated...", deployment.Status.UpdatedReplicas, *deployment.Spec.Replicas),
 			}, nil
-		} else if deployment.Status.Replicas > deployment.Status.UpdatedReplicas {
+		case deployment.Status.Replicas > deployment.Status.UpdatedReplicas:
 			return &HealthStatus{
 				Status:  HealthStatusProgressing,
 				Message: fmt.Sprintf("Waiting for rollout to finish: %d old replicas are pending termination...", deployment.Status.Replicas-deployment.Status.UpdatedReplicas),
 			}, nil
-		} else if deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas {
+		case deployment.Status.AvailableReplicas < deployment.Status.UpdatedReplicas:
 			return &HealthStatus{
 				Status:  HealthStatusProgressing,
 				Message: fmt.Sprintf("Waiting for rollout to finish: %d of %d updated replicas are available...", deployment.Status.AvailableReplicas, deployment.Status.UpdatedReplicas),

pkg/health/health_job.go

@@ -50,24 +50,26 @@ func getBatchv1JobHealth(job *batchv1.Job) (*HealthStatus, error) {
 			}
 		}
 	}
-	if !complete {
+	switch {
+	case !complete:
 		return &HealthStatus{
 			Status:  HealthStatusProgressing,
 			Message: message,
 		}, nil
-	} else if failed {
+	case failed:
 		return &HealthStatus{
 			Status:  HealthStatusDegraded,
 			Message: failMsg,
 		}, nil
-	} else if isSuspended {
+	case isSuspended:
 		return &HealthStatus{
 			Status:  HealthStatusSuspended,
 			Message: failMsg,
 		}, nil
+	default:
+		return &HealthStatus{
+			Status:  HealthStatusHealthy,
+			Message: message,
+		}, nil
 	}
-	return &HealthStatus{
-		Status:  HealthStatusHealthy,
-		Message: message,
-	}, nil
 }

pkg/sync/sync_context.go

@@ -718,9 +718,7 @@ func (sc *syncContext) getSyncTasks() (_ syncTasks, successful bool) {
 		task.liveObj = sc.liveObj(task.targetObj)
 	}
 
-	isRetryable := func(err error) bool {
-		return apierrors.IsUnauthorized(err)
-	}
+	isRetryable := apierrors.IsUnauthorized
 
 	serverResCache := make(map[schema.GroupVersionKind]*metav1.APIResource)
 
@@ -849,20 +847,19 @@ func (sc *syncContext) autoCreateNamespace(tasks syncTasks) syncTasks {
 		managedNs, err := kubeutil.ToUnstructured(nsSpec)
 		if err == nil {
 			liveObj, err := sc.kubectl.GetResource(context.TODO(), sc.config, managedNs.GroupVersionKind(), managedNs.GetName(), metav1.NamespaceNone)
-			if err == nil {
+			switch {
+			case err == nil:
 				nsTask := &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: liveObj}
 				_, ok := sc.syncRes[nsTask.resultKey()]
 				if ok {
 					tasks = sc.appendNsTask(tasks, nsTask, managedNs, liveObj)
-				} else {
-					if liveObj != nil {
-						sc.log.WithValues("namespace", sc.namespace).Info("Namespace already exists")
-						tasks = sc.appendNsTask(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: liveObj}, managedNs, liveObj)
-					}
+				} else if liveObj != nil {
+					sc.log.WithValues("namespace", sc.namespace).Info("Namespace already exists")
+					tasks = sc.appendNsTask(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: liveObj}, managedNs, liveObj)
 				}
-			} else if apierrors.IsNotFound(err) {
+			case apierrors.IsNotFound(err):
 				tasks = sc.appendNsTask(tasks, &syncTask{phase: common.SyncPhasePreSync, targetObj: managedNs, liveObj: nil}, managedNs, nil)
-			} else {
+			default:
 				tasks = sc.appendFailedNsTask(tasks, managedNs, fmt.Errorf("Namespace auto creation failed: %w", err))
 			}
 		} else {

pkg/sync/sync_context_test.go

@@ -131,12 +131,13 @@ func TestSyncCreateInSortedOrder(t *testing.T) {
 	assert.Len(t, resources, 2)
 	for i := range resources {
 		result := resources[i]
-		if result.ResourceKey.Kind == "Pod" {
+		switch result.ResourceKey.Kind {
+		case "Pod":
 			assert.Equal(t, synccommon.ResultCodeSynced, result.Status)
 			assert.Equal(t, "", result.Message)
-		} else if result.ResourceKey.Kind == "Service" {
+		case "Service":
 			assert.Equal(t, "", result.Message)
-		} else {
+		default:
 			t.Error("Resource isn't a pod or a service")
 		}
 	}
@@ -255,13 +256,14 @@ func TestSyncSuccessfully(t *testing.T) {
 	assert.Len(t, resources, 2)
 	for i := range resources {
 		result := resources[i]
-		if result.ResourceKey.Kind == "Pod" {
+		switch result.ResourceKey.Kind {
+		case "Pod":
 			assert.Equal(t, synccommon.ResultCodePruned, result.Status)
 			assert.Equal(t, "pruned", result.Message)
-		} else if result.ResourceKey.Kind == "Service" {
+		case "Service":
 			assert.Equal(t, synccommon.ResultCodeSynced, result.Status)
 			assert.Equal(t, "", result.Message)
-		} else {
+		default:
 			t.Error("Resource isn't a pod or a service")
 		}
 	}
@@ -284,13 +286,14 @@ func TestSyncDeleteSuccessfully(t *testing.T) {
 	assert.Equal(t, synccommon.OperationSucceeded, phase)
 	for i := range resources {
 		result := resources[i]
-		if result.ResourceKey.Kind == "Pod" {
+		switch result.ResourceKey.Kind {
+		case "Pod":
 			assert.Equal(t, synccommon.ResultCodePruned, result.Status)
 			assert.Equal(t, "pruned", result.Message)
-		} else if result.ResourceKey.Kind == "Service" {
+		case "Service":
 			assert.Equal(t, synccommon.ResultCodePruned, result.Status)
 			assert.Equal(t, "pruned", result.Message)
-		} else {
+		default:
 			t.Error("Resource isn't a pod or a service")
 		}
 	}

pkg/utils/kube/kube.go

@@ -215,7 +215,7 @@ func cleanKubectlOutput(s string) string {
 	s = kubectlErrOutRegexp.ReplaceAllString(s, "")
 	s = kubectlErrOutMapRegexp.ReplaceAllString(s, "")
 	s = kubectlApplyPatchErrOutRegexp.ReplaceAllString(s, "")
-	s = strings.Replace(s, "; if you choose to ignore these errors, turn validation off with --validate=false", "", -1)
+	s = strings.ReplaceAll(s, "; if you choose to ignore these errors, turn validation off with --validate=false", "")
 	return s
 }