chore: enable gocritic linter (argoproj#18633)

* chore: enable gocritic linter Signed-off-by: Matthieu MOREL <[email protected]> * Update settings.go Signed-off-by: Matthieu MOREL <[email protected]> * Update app.go Signed-off-by: Matthieu MOREL <[email protected]> * Update grpcproxy.go Signed-off-by: Matthieu MOREL <[email protected]> * Update grpcproxy.go Signed-off-by: Matthieu MOREL <[email protected]> * Update util.go Signed-off-by: Matthieu MOREL <[email protected]> * Update server.go Signed-off-by: Matthieu MOREL <[email protected]> * Update app_management_ns_test.go Signed-off-by: Matthieu MOREL <[email protected]> * Update app_management_test.go Signed-off-by: Matthieu MOREL <[email protected]> * Update path_traversal.go Signed-off-by: Matthieu MOREL <[email protected]> * Update sessionmanager.go Signed-off-by: Matthieu MOREL <[email protected]> * Update .golangci.yaml Signed-off-by: Matthieu MOREL <[email protected]> --------- Signed-off-by: Matthieu MOREL <[email protected]>
Canva · Jun 13, 2024 · 9f1e2e8 · 9f1e2e8
1 parent 9592b84
commit 9f1e2e8
Show file tree

Hide file tree

Showing 31 changed files with 98 additions and 111 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -1,5 +1,6 @@
 issues:
   exclude:
+    - SA1019
     - SA5011
   exclude-rules:
     - path: "(applicationset|cmd|cmpserver|controller|pkg|reposerver|server|test)/"
@@ -16,6 +17,7 @@ linters:
   enable:
     - errcheck
     - errorlint
+    - gocritic
     - gofumpt
     - goimports
     - gosimple
@@ -27,6 +29,17 @@ linters:
     - unused
     - whitespace 
 linters-settings:
+  gocritic:
+    disabled-checks:
+      - appendAssign
+      - assignOp  # Keep it disabled for readability
+      - badCond
+      - commentFormatting
+      - exitAfterDefer
+      - ifElseChain
+      - mapKey
+      - singleCaseSwitch
+      - typeSwitchVar
   goimports:
     local-prefixes: github.com/argoproj/argo-cd/v2
   testifylint:

diff --git a/applicationset/generators/git.go b/applicationset/generators/git.go
@@ -226,13 +226,13 @@ func (g *GitGenerator) generateParamsFromGitFile(filePath string, fileContent []
 	return res, nil
 }
 
-func (g *GitGenerator) filterApps(Directories []argoprojiov1alpha1.GitDirectoryGeneratorItem, allPaths []string) []string {
+func (g *GitGenerator) filterApps(directories []argoprojiov1alpha1.GitDirectoryGeneratorItem, allPaths []string) []string {
 	res := []string{}
 	for _, appPath := range allPaths {
 		appInclude := false
 		appExclude := false
 		// Iterating over each appPath and check whether directories object has requestedPath that matches the appPath
-		for _, requestedPath := range Directories {
+		for _, requestedPath := range directories {
 			match, err := path.Match(requestedPath.Path, appPath)
 			if err != nil {
 				log.WithError(err).WithField("requestedPath", requestedPath).

diff --git a/applicationset/generators/plugin_test.go b/applicationset/generators/plugin_test.go
@@ -645,7 +645,7 @@ func TestPluginGenerateParams(t *testing.T) {
 			handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 				authHeader := r.Header.Get("Authorization")
 				_, tokenKey := plugin.ParseSecretKey(testCase.configmap.Data["token"])
-				expectedToken := testCase.secret.Data[strings.Replace(tokenKey, "$", "", -1)]
+				expectedToken := testCase.secret.Data[strings.ReplaceAll(tokenKey, "$", "")]
 				if authHeader != "Bearer "+string(expectedToken) {
 					w.WriteHeader(http.StatusUnauthorized)
 					return

diff --git a/applicationset/utils/clusterUtils.go b/applicationset/utils/clusterUtils.go
@@ -61,10 +61,8 @@ func ValidateDestination(ctx context.Context, dest *appv1.ApplicationDestination
 				return fmt.Errorf("application references destination cluster %s which does not exist", dest.Name)
 			}
 			dest.SetInferredServer(server)
-		} else {
-			if !dest.IsServerInferred() {
-				return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server)
-			}
+		} else if !dest.IsServerInferred() {
+			return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server)
 		}
 	}
 	return nil

diff --git a/applicationset/utils/utils.go b/applicationset/utils/utils.go
@@ -268,8 +268,8 @@ func (r *Render) RenderTemplateParams(tmpl *argoappsv1.Application, syncPolicy *
 	// b) there IS a syncPolicy, but preserveResourcesOnDeletion is set to false
 	// See TestRenderTemplateParamsFinalizers in util_test.go for test-based definition of behaviour
 	if (syncPolicy == nil || !syncPolicy.PreserveResourcesOnDeletion) &&
-		((*replacedTmpl).ObjectMeta.Finalizers == nil || len((*replacedTmpl).ObjectMeta.Finalizers) == 0) {
-		(*replacedTmpl).ObjectMeta.Finalizers = []string{"resources-finalizer.argocd.argoproj.io"}
+		(replacedTmpl.ObjectMeta.Finalizers == nil || len(replacedTmpl.ObjectMeta.Finalizers) == 0) {
+		replacedTmpl.ObjectMeta.Finalizers = []string{"resources-finalizer.argocd.argoproj.io"}
 	}
 
 	return replacedTmpl, nil

diff --git a/cmd/argocd/commands/admin/project_allowlist.go b/cmd/argocd/commands/admin/project_allowlist.go
@@ -120,7 +120,7 @@ func generateProjectAllowList(serverResources []*metav1.APIResourceList, cluster
 
 	resourceList := make([]metav1.GroupKind, 0)
 	for _, rule := range clusterRole.Rules {
-		if len(rule.APIGroups) <= 0 {
+		if len(rule.APIGroups) == 0 {
 			continue
 		}
 

diff --git a/cmd/argocd/commands/app.go b/cmd/argocd/commands/app.go
@@ -2544,10 +2544,8 @@ func setParameterOverrides(app *argoappv1.Application, parameters []string, sour
 		sourceType = *st
 	} else if app.Status.SourceType != "" {
 		sourceType = app.Status.SourceType
-	} else {
-		if len(strings.SplitN(parameters[0], "=", 2)) == 2 {
-			sourceType = argoappv1.ApplicationSourceTypeHelm
-		}
+	} else if len(strings.SplitN(parameters[0], "=", 2)) == 2 {
+		sourceType = argoappv1.ApplicationSourceTypeHelm
 	}
 
 	switch sourceType {

diff --git a/cmd/util/app.go b/cmd/util/app.go
@@ -434,10 +434,8 @@ func SetParameterOverrides(app *argoappv1.Application, parameters []string, inde
 		sourceType = *st
 	} else if app.Status.SourceType != "" {
 		sourceType = app.Status.SourceType
-	} else {
-		if len(strings.SplitN(parameters[0], "=", 2)) == 2 {
-			sourceType = argoappv1.ApplicationSourceTypeHelm
-		}
+	} else if len(strings.SplitN(parameters[0], "=", 2)) == 2 {
+		sourceType = argoappv1.ApplicationSourceTypeHelm
 	}
 
 	switch sourceType {

diff --git a/controller/appcontroller.go b/controller/appcontroller.go
@@ -305,13 +305,11 @@ func (ctrl *ApplicationController) InvalidateProjectsCache(names ...string) {
 		for _, name := range names {
 			ctrl.projByNameCache.Delete(name)
 		}
-	} else {
-		if ctrl != nil {
-			ctrl.projByNameCache.Range(func(key, _ interface{}) bool {
-				ctrl.projByNameCache.Delete(key)
-				return true
-			})
-		}
+	} else if ctrl != nil {
+		ctrl.projByNameCache.Range(func(key, _ interface{}) bool {
+			ctrl.projByNameCache.Delete(key)
+			return true
+		})
 	}
 }
 

diff --git a/controller/metrics/metrics.go b/controller/metrics/metrics.go
@@ -62,21 +62,21 @@ var (
 		append(descAppDefaultLabels, "autosync_enabled", "repo", "dest_server", "dest_namespace", "sync_status", "health_status", "operation"),
 		nil,
 	)
-	// DEPRECATED
+	// Deprecated
 	descAppCreated = prometheus.NewDesc(
 		"argocd_app_created_time",
 		"Creation time in unix timestamp for an application.",
 		descAppDefaultLabels,
 		nil,
 	)
-	// DEPRECATED: superseded by sync_status label in argocd_app_info
+	// Deprecated: superseded by sync_status label in argocd_app_info
 	descAppSyncStatusCode = prometheus.NewDesc(
 		"argocd_app_sync_status",
 		"The application current sync status.",
 		append(descAppDefaultLabels, "sync_status"),
 		nil,
 	)
-	// DEPRECATED: superseded by health_status label in argocd_app_info
+	// Deprecated: superseded by health_status label in argocd_app_info
 	descAppHealthStatus = prometheus.NewDesc(
 		"argocd_app_health_status",
 		"The application current health status.",

diff --git a/notification_controller/controller/controller.go b/notification_controller/controller/controller.go
@@ -166,9 +166,7 @@ func newInformer(resClient dynamic.ResourceInterface, controllerNamespace string
 		&unstructured.Unstructured{},
 		resyncPeriod,
 		cache.Indexers{
-			cache.NamespaceIndex: func(obj interface{}) ([]string, error) {
-				return cache.MetaNamespaceIndexFunc(obj)
-			},
+			cache.NamespaceIndex: cache.MetaNamespaceIndexFunc,
 		},
 	)
 	return informer

diff --git a/pkg/apis/application/v1alpha1/generated.proto b/pkg/apis/application/v1alpha1/generated.proto
diff --git a/pkg/apis/application/v1alpha1/openapi_generated.go b/pkg/apis/application/v1alpha1/openapi_generated.go
diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go
@@ -1770,10 +1770,10 @@ type Cluster struct {
 	Name string `json:"name" protobuf:"bytes,2,opt,name=name"`
 	// Config holds cluster information for connecting to a cluster
 	Config ClusterConfig `json:"config" protobuf:"bytes,3,opt,name=config"`
-	// DEPRECATED: use Info.ConnectionState field instead.
+	// Deprecated: use Info.ConnectionState field instead.
 	// ConnectionState contains information about cluster connection state
 	ConnectionState ConnectionState `json:"connectionState,omitempty" protobuf:"bytes,4,opt,name=connectionState"`
-	// DEPRECATED: use Info.ServerVersion field instead.
+	// Deprecated: use Info.ServerVersion field instead.
 	// The server version
 	ServerVersion string `json:"serverVersion,omitempty" protobuf:"bytes,5,opt,name=serverVersion"`
 	// Holds list of namespaces which are accessible in that cluster. Cluster level resources will be ignored if namespace list is not empty.
@@ -2472,10 +2472,8 @@ func (w *SyncWindows) hasDeny() (bool, bool) {
 		if a.Kind == "deny" {
 			if !denyFound {
 				manualEnabled = a.ManualSync
-			} else {
-				if manualEnabled {
-					manualEnabled = a.ManualSync
-				}
+			} else if manualEnabled {
+				manualEnabled = a.ManualSync
 			}
 			denyFound = true
 		}

diff --git a/server/application/application.go b/server/application/application.go
@@ -1091,11 +1091,9 @@ func (s *Server) Delete(ctx context.Context, q *application.ApplicationDeleteReq
 			a.SetCascadedDeletion(policyFinalizer)
 			patchFinalizer = true
 		}
-	} else {
-		if a.CascadedDeletion() {
-			a.UnSetCascadedDeletion()
-			patchFinalizer = true
-		}
+	} else if a.CascadedDeletion() {
+		a.UnSetCascadedDeletion()
+		patchFinalizer = true
 	}
 
 	if patchFinalizer {

diff --git a/server/badge/badge.go b/server/badge/badge.go
@@ -132,10 +132,8 @@ func (h *Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 				if app.Status.OperationState != nil && app.Status.OperationState.SyncResult != nil {
 					revision = app.Status.OperationState.SyncResult.Revision
 				}
-			} else {
-				if errors.IsNotFound(err) {
-					notFound = true
-				}
+			} else if errors.IsNotFound(err) {
+				notFound = true
 			}
 		} else {
 			w.WriteHeader(http.StatusBadRequest)

diff --git a/server/project/project_test.go b/server/project/project_test.go
@@ -646,7 +646,7 @@ p, role:admin, projects, update, *, allow`)
 
 		projWithRole := existingProj.DeepCopy()
 		role := v1alpha1.ProjectRole{Name: roleName, JWTTokens: []v1alpha1.JWTToken{{IssuedAt: 1}}}
-		noSpacesPolicyTemplate := strings.Replace(policyTemplate, " ", "", -1)
+		noSpacesPolicyTemplate := strings.ReplaceAll(policyTemplate, " ", "")
 		invalidPolicy := fmt.Sprintf(noSpacesPolicyTemplate, projWithRole.Name, roleName, action, projWithRole.Name, object, effect)
 		role.Policies = append(role.Policies, invalidPolicy)
 		projWithRole.Spec.Roles = append(projWithRole.Spec.Roles, role)

diff --git a/test/e2e/app_management_ns_test.go b/test/e2e/app_management_ns_test.go
@@ -1092,10 +1092,8 @@ func assertNSResourceActions(t *testing.T, appName string, successful bool) {
 	assertError := func(err error, message string) {
 		if successful {
 			assert.NoError(t, err)
-		} else {
-			if assert.Error(t, err) {
-				assert.Contains(t, err.Error(), message)
-			}
+		} else if assert.Error(t, err) {
+			assert.Contains(t, err.Error(), message)
 		}
 	}
 

diff --git a/test/e2e/app_management_test.go b/test/e2e/app_management_test.go
@@ -1450,10 +1450,8 @@ func assertResourceActions(t *testing.T, appName string, successful bool) {
 	assertError := func(err error, message string) {
 		if successful {
 			assert.NoError(t, err)
-		} else {
-			if assert.Error(t, err) {
-				assert.Contains(t, err.Error(), message)
-			}
+		} else if assert.Error(t, err) {
+			assert.Contains(t, err.Error(), message)
 		}
 	}
 

diff --git a/util/argo/argo.go b/util/argo/argo.go
@@ -501,10 +501,8 @@ func ValidateDestination(ctx context.Context, dest *argoappv1.ApplicationDestina
 				return fmt.Errorf("application references destination cluster %s which does not exist", dest.Name)
 			}
 			dest.SetInferredServer(server)
-		} else {
-			if !dest.IsServerInferred() {
-				return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server)
-			}
+		} else if !dest.IsServerInferred() {
+			return fmt.Errorf("application destination can't have both name and server defined: %s %s", dest.Name, dest.Server)
 		}
 	}
 	return nil
@@ -1005,7 +1003,7 @@ func GenerateSpecIsDifferentErrorMessage(entity string, a, b interface{}) string
 	if len(difference) == 0 {
 		return basicMsg
 	}
-	return fmt.Sprintf("%s; difference in keys \"%s\"", basicMsg, strings.Join(difference[:], ","))
+	return fmt.Sprintf("%s; difference in keys \"%s\"", basicMsg, strings.Join(difference, ","))
 }
 
 func GetDifferentPathsBetweenStructs(a, b interface{}) ([]string, error) {

diff --git a/util/cache/cache.go b/util/cache/cache.go
@@ -96,7 +96,7 @@ func (o *Options) callOnClientCreated(client *redis.Client) {
 }
 
 func (o *Options) getEnvPrefix() string {
-	return strings.Replace(strings.ToUpper(o.FlagPrefix), "-", "_", -1)
+	return strings.ReplaceAll(strings.ToUpper(o.FlagPrefix), "-", "_")
 }
 
 func mergeOptions(opts ...Options) Options {

diff --git a/util/cert/cert.go b/util/cert/cert.go
@@ -271,8 +271,8 @@ func TokenizedDataToPublicKey(hostname string, subType string, rawKeyData string
 
 // Returns the requested pattern with all possible square brackets escaped
 func nonBracketedPattern(pattern string) string {
-	ret := strings.Replace(pattern, "[", `\[`, -1)
-	return strings.Replace(ret, "]", `\]`, -1)
+	ret := strings.ReplaceAll(pattern, "[", `\[`)
+	return strings.ReplaceAll(ret, "]", `\]`)
 }
 
 // We do not use full fledged regular expression for matching the hostname.

diff --git a/util/db/certificate.go b/util/db/certificate.go
@@ -289,13 +289,11 @@ func (db *db) CreateRepoCertificate(ctx context.Context, certificates *appsv1.Re
 					Data:    string(certificate.CertData),
 				}
 				tlsCertificates = append(tlsCertificates, tlsCertificate)
-			} else {
+			} else if tlsCertificate.Data != string(certificate.CertData) {
 				// We have made sure the upsert flag was set above. Now just figure out
 				// again if we have to actually update the data in the existing cert.
-				if tlsCertificate.Data != string(certificate.CertData) {
-					tlsCertificate.Data = string(certificate.CertData)
-					upserted = true
-				}
+				tlsCertificate.Data = string(certificate.CertData)
+				upserted = true
 			}
 
 			if newEntry || upserted {

diff --git a/util/dex/dex.go b/util/dex/dex.go
@@ -102,11 +102,11 @@ func NewDexHTTPReverseProxy(serverAddr string, baseHRef string, tlsConfig *DexTL
 }
 
 // NewDexRewriteURLRoundTripper creates a new DexRewriteURLRoundTripper
-func NewDexRewriteURLRoundTripper(dexServerAddr string, T http.RoundTripper) DexRewriteURLRoundTripper {
+func NewDexRewriteURLRoundTripper(dexServerAddr string, t http.RoundTripper) DexRewriteURLRoundTripper {
 	dexURL, _ := url.Parse(dexServerAddr)
 	return DexRewriteURLRoundTripper{
 		DexURL: dexURL,
-		T:      T,
+		T:      t,
 	}
 }
 

diff --git a/util/grpc/sanitizer.go b/util/grpc/sanitizer.go
@@ -64,7 +64,7 @@ func NewSanitizer() *sanitizer {
 // AddReplacement adds a replacement to the Sanitizer
 func (s *sanitizer) AddReplacement(val string, replacement string) {
 	s.replacers = append(s.replacers, func(in string) string {
-		return strings.Replace(in, val, replacement, -1)
+		return strings.ReplaceAll(in, val, replacement)
 	})
 }