chore: enable and fix some gocritic issues

.golangci.yml

@@ -8,6 +8,7 @@ linters:
   default: none
   enable:
     - ginkgolinter
+    - gocritic
     - govet
     - ineffassign
     - misspell
@@ -16,6 +17,35 @@ linters:
     - unconvert
     - unused
   settings:
+    gocritic:
+      disabled-checks:
+        - appendAssign
+        - appendCombine
+        - assignOp
+        - badLock
+        - builtinShadow
+        - commentedOutCode
+        - deferInLoop
+        - emptyStringTest
+        - evalOrder
+        - exposedSyncMutex
+        - hugeParam
+        - importShadow
+        - nestingReduce
+        - nilValReturn
+        - octalLiteral
+        - paramTypeCombine
+        - rangeValCopy
+        - regexpSimplify
+        - singleCaseSwitch
+        - sloppyReassign
+        - typeAssertChain
+        - unlabelStmt
+        - unlambda
+        - unnamedResult
+        - whyNoLint
+        - yodaStyleExpr
+      enable-all: true
     govet:
       enable:
         - nilness

api/defaults/service.go

@@ -66,11 +66,9 @@ func InterpolateService(origSpec *api.ServiceSpec) *api.ServiceSpec {
 
 	if spec.Task.Restart == nil {
 		spec.Task.Restart = Service.Task.Restart.Copy()
-	} else {
-		if spec.Task.Restart.Delay == nil {
-			spec.Task.Restart.Delay = &gogotypes.Duration{}
-			deepcopy.Copy(spec.Task.Restart.Delay, Service.Task.Restart.Delay)
-		}
+	} else if spec.Task.Restart.Delay == nil {
+		spec.Task.Restart.Delay = &gogotypes.Duration{}
+		deepcopy.Copy(spec.Task.Restart.Delay, Service.Task.Restart.Delay)
 	}
 
 	if spec.Task.Placement == nil {
@@ -79,20 +77,16 @@ func InterpolateService(origSpec *api.ServiceSpec) *api.ServiceSpec {
 
 	if spec.Update == nil {
 		spec.Update = Service.Update.Copy()
-	} else {
-		if spec.Update.Monitor == nil {
-			spec.Update.Monitor = &gogotypes.Duration{}
-			deepcopy.Copy(spec.Update.Monitor, Service.Update.Monitor)
-		}
+	} else if spec.Update.Monitor == nil {
+		spec.Update.Monitor = &gogotypes.Duration{}
+		deepcopy.Copy(spec.Update.Monitor, Service.Update.Monitor)
 	}
 
 	if spec.Rollback == nil {
 		spec.Rollback = Service.Rollback.Copy()
-	} else {
-		if spec.Rollback.Monitor == nil {
-			spec.Rollback.Monitor = &gogotypes.Duration{}
-			deepcopy.Copy(spec.Rollback.Monitor, Service.Rollback.Monitor)
-		}
+	} else if spec.Rollback.Monitor == nil {
+		spec.Rollback.Monitor = &gogotypes.Duration{}
+		deepcopy.Copy(spec.Rollback.Monitor, Service.Rollback.Monitor)
 	}
 
 	return spec

ca/keyreadwriter.go

@@ -208,7 +208,7 @@ func (k *KeyReadWriter) Read() ([]byte, []byte, error) {
 	switch {
 	case err == nil:
 		_, err = tls.X509KeyPair(cert, keyBytes)
-	case os.IsNotExist(err): //continue to try temp location
+	case os.IsNotExist(err): // continue to try temp location
 		break
 	default:
 		return nil, nil, err

ca/renewer.go

@@ -95,15 +95,16 @@ func (t *TLSRenewer) Start(ctx context.Context) <-chan CertificateUpdate {
 			} else {
 				// If we have an expired certificate, try to renew immediately: the hope that this is a temporary clock skew, or
 				// we can issue our own TLS certs.
-				if validUntil.Before(time.Now()) {
+				switch {
+				case validUntil.Before(time.Now()):
 					logger.Warn("the current TLS certificate is expired, so an attempt to renew it will be made immediately")
 					// retry immediately(ish) with exponential backoff
 					retry = expBackoff.Proceed(nil)
-				} else if forceRetry {
+				case forceRetry:
 					// A forced renewal was requested, but did not succeed yet.
 					// retry immediately(ish) with exponential backoff
 					retry = expBackoff.Proceed(nil)
-				} else {
+				default:
 					// Random retry time between 50% and 80% of the total time to expiration
 					retry = calculateRandomExpiry(validFrom, validUntil)
 				}

cli/external_ca.go

@@ -75,7 +75,7 @@ func parseExternalCA(caSpec string) (*api.ExternalCA, error) {
 		switch strings.ToLower(key) {
 		case "protocol":
 			hasProtocol = true
-			if strings.ToLower(value) == "cfssl" {
+			if strings.EqualFold(value, "cfssl") {
 				externalCA.Protocol = api.ExternalCA_CAProtocolCFSSL
 			} else {
 				return nil, fmt.Errorf("unrecognized external CA protocol %s", value)

manager/controlapi/cluster.go

@@ -50,7 +50,7 @@ func validateClusterSpec(spec *api.ClusterSpec) error {
 	// TODO(diogo): Add a global list of acceptance algorithms. We only support bcrypt for now.
 	if len(spec.AcceptancePolicy.Policies) > 0 {
 		for _, policy := range spec.AcceptancePolicy.Policies {
-			if policy.Secret != nil && strings.ToLower(policy.Secret.Alg) != "bcrypt" {
+			if policy.Secret != nil && !strings.EqualFold(policy.Secret.Alg, "bcrypt") {
 				return status.Errorf(codes.InvalidArgument, "hashing algorithm is not supported: %s", policy.Secret.Alg)
 			}
 		}

manager/logbroker/broker.go

@@ -405,10 +405,8 @@ func (lb *LogBroker) PublishLogs(stream api.LogBroker_PublishLogsServer) (err er
 			if currentSubscription == nil {
 				return status.Errorf(codes.NotFound, "unknown subscription ID")
 			}
-		} else {
-			if logMsg.SubscriptionID != currentSubscription.ID() {
-				return status.Errorf(codes.InvalidArgument, "different subscription IDs in the same session")
-			}
+		} else if logMsg.SubscriptionID != currentSubscription.ID() {
+			return status.Errorf(codes.InvalidArgument, "different subscription IDs in the same session")
 		}
 
 		// if we have a close message, close out the subscription

manager/orchestrator/jobs/replicated/reconciler.go

@@ -139,13 +139,11 @@ func (r *Reconciler) ReconcileService(id string) error {
 						restartTasks = append(restartTasks, task.ID)
 					}
 				}
-			} else {
+			} else if task.Status.State <= api.TaskStateRunning && task.DesiredState != api.TaskStateRemove {
 				// tasks belonging to a previous iteration of the job may
 				// exist. if any such tasks exist, they should have their task
 				// state set to Remove
-				if task.Status.State <= api.TaskStateRunning && task.DesiredState != api.TaskStateRemove {
-					removeTasks = append(removeTasks, task.ID)
-				}
+				removeTasks = append(removeTasks, task.ID)
 			}
 		}
 	}

manager/orchestrator/restart/restart.go

@@ -152,11 +152,12 @@ func (r *Supervisor) Restart(ctx context.Context, tx store.Tx, cluster *api.Clus
 
 	var restartTask *api.Task
 
-	if orchestrator.IsReplicatedService(service) || orchestrator.IsReplicatedJob(service) {
+	switch {
+	case orchestrator.IsReplicatedService(service), orchestrator.IsReplicatedJob(service):
 		restartTask = orchestrator.NewTask(cluster, service, t.Slot, "")
-	} else if orchestrator.IsGlobalService(service) || orchestrator.IsGlobalJob(service) {
+	case orchestrator.IsGlobalService(service), orchestrator.IsGlobalJob(service):
 		restartTask = orchestrator.NewTask(cluster, service, 0, t.NodeID)
-	} else {
+	default:
 		log.G(ctx).Error("service not supported by restart supervisor")
 		return nil
 	}

manager/orchestrator/update/updater.go

@@ -332,15 +332,16 @@ func (u *Updater) worker(ctx context.Context, queue <-chan orchestrator.Slot, up
 				}
 			}
 		}
-		if runningTask != nil {
+		switch {
+		case runningTask != nil:
 			if err := u.useExistingTask(ctx, slot, runningTask); err != nil {
 				log.G(ctx).WithError(err).Error("update failed")
 			}
-		} else if cleanTask != nil {
+		case cleanTask != nil:
 			if err := u.useExistingTask(ctx, slot, cleanTask); err != nil {
 				log.G(ctx).WithError(err).Error("update failed")
 			}
-		} else {
+		default:
 			updated := orchestrator.NewTask(u.cluster, u.newService, slot[0].Slot, "")
 			if orchestrator.IsGlobalService(u.newService) {
 				updated = orchestrator.NewTask(u.cluster, u.newService, slot[0].Slot, slot[0].NodeID)

protobuf/plugin/deepcopy/deepcopy.go

@@ -157,7 +157,8 @@ func (d *deepCopyGen) genMap(_ *generator.Descriptor, f *descriptor.FieldDescrip
 	d.P("m.", fName, " = make(", typename, ", ", "len(o.", fName, "))")
 	d.P("for k, v := range o.", fName, " {")
 	d.In()
-	if mt.ValueField.IsMessage() {
+	switch {
+	case mt.ValueField.IsMessage():
 		if !gogoproto.IsNullable(f) {
 			d.P("n := ", d.TypeName(d.ObjectNamed(mt.ValueField.GetTypeName())), "{}")
 			d.genCopyFunc("&n", "&v")
@@ -166,10 +167,10 @@ func (d *deepCopyGen) genMap(_ *generator.Descriptor, f *descriptor.FieldDescrip
 			d.P("m.", fName, "[k] = &", d.TypeName(d.ObjectNamed(mt.ValueField.GetTypeName())), "{}")
 			d.genCopyFunc("m."+fName+"[k]", "v")
 		}
-	} else if mt.ValueField.IsBytes() {
+	case mt.ValueField.IsBytes():
 		d.P("m.", fName, "[k] = o.", fName, "[k]")
 		d.genCopyBytes("m."+fName+"[k]", "o."+fName+"[k]")
-	} else {
+	default:
 		d.P("m.", fName, "[k] = v")
 	}
 	d.Out()
@@ -192,7 +193,8 @@ func (d *deepCopyGen) genRepeated(m *generator.Descriptor, f *descriptor.FieldDe
 	d.P("if o.", fName, " != nil {")
 	d.In()
 	d.P("m.", fName, " = make(", typename, ", len(o.", fName, "))")
-	if f.IsMessage() {
+	switch {
+	case f.IsMessage():
 		// TODO(stevvooe): Handle custom type here?
 		goType := d.TypeName(d.ObjectNamed(f.GetTypeName())) // elides [] or *
 
@@ -206,13 +208,13 @@ func (d *deepCopyGen) genRepeated(m *generator.Descriptor, f *descriptor.FieldDe
 		}
 		d.Out()
 		d.P("}")
-	} else if f.IsBytes() {
+	case f.IsBytes():
 		d.P("for i := range m.", fName, " {")
 		d.In()
 		d.genCopyBytes("m."+fName+"[i]", "o."+fName+"[i]")
 		d.Out()
 		d.P("}")
-	} else {
+	default:
 		d.P("copy(m.", fName, ", ", "o.", fName, ")")
 	}
 	d.Out()
@@ -241,12 +243,13 @@ func (d *deepCopyGen) genOneOf(m *generator.Descriptor, oneof *descriptor.OneofD
 		d.In()
 
 		var rhs string
-		if f.IsMessage() {
+		switch {
+		case f.IsMessage():
 			goType := d.TypeName(d.ObjectNamed(f.GetTypeName())) // elides [] or *
 			rhs = "&" + goType + "{}"
-		} else if f.IsBytes() {
+		case f.IsBytes():
 			rhs = "make([]byte, len(o.Get" + fName + "()))"
-		} else {
+		default:
 			rhs = "o.Get" + fName + "()"
 		}
 		d.P(fName, ": ", rhs, ",")

swarmd/dockerexec/container.go

@@ -263,7 +263,7 @@ func (c *containerConfig) labels() map[string]string {
 
 	// finally, we apply the system labels, which override all labels.
 	for k, v := range system {
-		labels[strings.Join([]string{systemLabelPrefix, k}, ".")] = v
+		labels[systemLabelPrefix+"."+k] = v
 	}
 
 	return labels