Update golangci-lint, add more linters

.golangci.yml

@@ -46,23 +46,47 @@ output:
 
 linters:
   enable:
+    - deadcode
     - megacheck
     - govet
     - golint
     - interfacer
     - unconvert
     - dupl
     - goconst
+    - gocritic
     - gocyclo
     - goimports
+    - interfacer
     - maligned
     - megacheck
+    - scopelint
+    - staticcheck
+    - structcheck
     - misspell
     - unparam
     - nakedret
     - bodyclose
 
+linters-settings:
+  gocritic:
+    enabled-tags:
+      - performance
+      - opinionated
+      - diagnostic
+    disabled-checks:
+      - paramTypeCombine
+      - unnamedResult
+    settings: # settings passed to gocritic
+      hugeParam:
+        sizeThreshold: 512
+      rangeValCopy:
+        sizeThreshold: 512
+        skipTestFuncs: true
+
 issues:
   # This turns off the default excludes - which was causing the linter
   # to miss things like erroneous comments
-  exclude-use-default: false
+  exclude-use-default: false
+  exclude:
+    - Using the variable on range scope .* in function literal

build/Makefile

@@ -304,7 +304,7 @@ build-controller-binary: $(ensure-build-image)
 lint: LINT_TIMEOUT ?= 15m
 lint: $(ensure-build-image)
 	docker run -t -e "TERM=xterm-256color" -e "$(gomod_on)" --rm $(common_mounts) -w $(workdir_path) $(DOCKER_RUN_ARGS) $(build_tag) bash -c \
-		"golangci-lint run ./examples/... && golangci-lint run --deadline $(LINT_TIMEOUT) ./..."
+		"golangci-lint run ./examples/... && golangci-lint run --timeout $(LINT_TIMEOUT) ./..."
 
 # Extract licenses from source tree
 build-licenses:

build/build-image/Dockerfile

@@ -68,7 +68,7 @@ RUN curl -L  ${HELM_URL} > /tmp/helm.tar.gz \
 RUN echo "source <(helm completion bash)" >> /root/.bashrc
 
 # install golang-ci linter
-RUN curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $GOPATH/bin v1.18.0
+RUN curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh -s -- -b $GOPATH/bin v1.22.2
 
 #
 #  \ \      / /__| |__  ___(_) |_ ___

cmd/allocator/main.go

@@ -243,19 +243,20 @@ func getCACertPool(path string) (*x509.CertPool, error) {
 	}
 
 	for _, file := range filesInfo {
-		if strings.HasSuffix(file.Name(), ".crt") || strings.HasSuffix(file.Name(), ".pem") {
-			certFile := filepath.Join(path, file.Name())
-			caCert, err := ioutil.ReadFile(certFile)
-			if err != nil {
-				logger.Errorf("ca cert is not readable or missing: %s", err.Error())
-				continue
-			}
-			if !caCertPool.AppendCertsFromPEM(caCert) {
-				logger.Errorf("client cert %s cannot be installed", certFile)
-				continue
-			}
-			logger.Infof("client cert %s is installed", certFile)
+		if !strings.HasSuffix(file.Name(), ".crt") && !strings.HasSuffix(file.Name(), ".pem") {
+			continue
+		}
+		certFile := filepath.Join(path, file.Name())
+		caCert, err := ioutil.ReadFile(certFile)
+		if err != nil {
+			logger.Errorf("ca cert is not readable or missing: %s", err.Error())
+			continue
+		}
+		if !caCertPool.AppendCertsFromPEM(caCert) {
+			logger.Errorf("client cert %s cannot be installed", certFile)
+			continue
 		}
+		logger.Infof("client cert %s is installed", certFile)
 	}
 
 	return caCertPool, nil
@@ -323,8 +324,7 @@ func (h *httpHandler) allocateHandler(w http.ResponseWriter, r *http.Request) {
 
 func httpCode(err error) int {
 	code := http.StatusInternalServerError
-	switch t := err.(type) {
-	case k8serror.APIStatus:
+	if t, ok := err.(k8serror.APIStatus); ok {
 		code = int(t.Status().Code)
 	}
 	return code

cmd/controller/main.go

@@ -359,7 +359,7 @@ type config struct {
 }
 
 // validate ensures the ctlConfig data is valid.
-func (c config) validate() error {
+func (c *config) validate() error {
 	if c.MinPort <= 0 || c.MaxPort <= 0 {
 		return errors.New("min Port and Max Port values are required")
 	}

cmd/ping/udp_test.go

@@ -120,9 +120,9 @@ func TestUDPServerHealth(t *testing.T) {
 	assert.Nil(t, err)
 }
 
-func defaultFixture(clock clock.Clock) (*udpServer, error) {
+func defaultFixture(cl clock.Clock) (*udpServer, error) {
 	u := newUDPServer(5)
-	u.clock = clock
+	u.clock = cl
 	var err error
 	u.conn, err = net.ListenPacket("udp", ":0")
 	return u, err

cmd/sdk-server/main.go

@@ -90,7 +90,8 @@ func main() {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	if ctlConf.IsLocal {
+	switch {
+	case ctlConf.IsLocal:
 		localSDK, err := registerLocal(grpcServer, ctlConf)
 		if err != nil {
 			logger.WithError(err).Fatal("Could not start local sdk server")
@@ -103,7 +104,7 @@ func main() {
 				close(timedStop)
 			}()
 		}
-	} else if ctlConf.Test != "" {
+	case ctlConf.Test != "":
 		localSDK, err := registerTestSdkServer(grpcServer, ctlConf)
 		if err != nil {
 			logger.WithError(err).Fatal("Could not start test sdk server")
@@ -116,7 +117,7 @@ func main() {
 				close(timedStop)
 			}()
 		}
-	} else {
+	default:
 		var config *rest.Config
 		config, err := rest.InClusterConfig()
 		if err != nil {

pkg/allocation/converters/converter.go

@@ -127,6 +127,7 @@ func convertInternalLabelSelectorToLabelSelector(in *metav1.LabelSelector) *pb.L
 func convertInternalLabelSelectorsToLabelSelectors(in []metav1.LabelSelector) []*pb.LabelSelector {
 	var result []*pb.LabelSelector
 	for _, l := range in {
+		l := l
 		c := convertInternalLabelSelectorToLabelSelector(&l)
 		result = append(result, c)
 	}

pkg/apis/agones/v1/common.go

@@ -74,7 +74,7 @@ func validateGSSpec(gs gsSpec) []metav1.StatusCause {
 
 // validateObjectMeta Check ObjectMeta specification
 // Used by Fleet, GameServerSet and GameServer
-func validateObjectMeta(objMeta metav1.ObjectMeta) []metav1.StatusCause {
+func validateObjectMeta(objMeta *metav1.ObjectMeta) []metav1.StatusCause {
 	var causes []metav1.StatusCause
 
 	errs := metav1validation.ValidateLabels(objMeta.Labels, field.NewPath("labels"))

pkg/apis/agones/v1/fleet.go

@@ -179,7 +179,7 @@ func (f *Fleet) Validate() ([]metav1.StatusCause, bool) {
 	if len(gsCauses) > 0 {
 		causes = append(causes, gsCauses...)
 	}
-	objMetaCauses := validateObjectMeta(f.Spec.Template.ObjectMeta)
+	objMetaCauses := validateObjectMeta(&f.Spec.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)
 	}

pkg/apis/agones/v1/gameserver.go

@@ -312,7 +312,7 @@ func (gss *GameServerSpec) applySchedulingDefaults() {
 // devAddress is a specific IP address used for local Gameservers, for fleets "" is used
 // If a GameServer Spec is invalid there will be > 0 values in
 // the returned array
-func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bool) {
+func (gss *GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, bool) {
 	var causes []metav1.StatusCause
 	if devAddress != "" {
 		// verify that the value is a valid IP address.
@@ -342,7 +342,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo
 		}
 	} else {
 		// make sure a name is specified when there is multiple containers in the pod.
-		if len(gss.Container) == 0 && len(gss.Template.Spec.Containers) > 1 {
+		if gss.Container == "" && len(gss.Template.Spec.Containers) > 1 {
 			causes = append(causes, metav1.StatusCause{
 				Type:    metav1.CauseTypeFieldValueInvalid,
 				Field:   "container",
@@ -389,7 +389,7 @@ func (gss GameServerSpec) Validate(devAddress string) ([]metav1.StatusCause, boo
 			})
 		}
 	}
-	objMetaCauses := validateObjectMeta(gss.Template.ObjectMeta)
+	objMetaCauses := validateObjectMeta(&gss.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)
 	}

pkg/apis/agones/v1/gameserverset.go

@@ -79,9 +79,9 @@ type GameServerSetStatus struct {
 
 // ValidateUpdate validates when updates occur. The argument
 // is the new GameServerSet, being passed into the old GameServerSet
-func (gsSet *GameServerSet) ValidateUpdate(new *GameServerSet) ([]metav1.StatusCause, bool) {
-	causes := validateName(new)
-	if !apiequality.Semantic.DeepEqual(gsSet.Spec.Template, new.Spec.Template) {
+func (gsSet *GameServerSet) ValidateUpdate(newGSS *GameServerSet) ([]metav1.StatusCause, bool) {
+	causes := validateName(newGSS)
+	if !apiequality.Semantic.DeepEqual(gsSet.Spec.Template, newGSS.Spec.Template) {
 		causes = append(causes, metav1.StatusCause{
 			Type:    metav1.CauseTypeFieldValueInvalid,
 			Field:   "template",
@@ -102,7 +102,7 @@ func (gsSet *GameServerSet) Validate() ([]metav1.StatusCause, bool) {
 		causes = append(causes, gsCauses...)
 	}
 
-	objMetaCauses := validateObjectMeta(gsSet.Spec.Template.ObjectMeta)
+	objMetaCauses := validateObjectMeta(&gsSet.Spec.Template.ObjectMeta)
 	if len(objMetaCauses) > 0 {
 		causes = append(causes, objMetaCauses...)
 	}

pkg/apis/agones/v1/register.go

@@ -53,15 +53,15 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&GameServer{},
 		&GameServerList{},
 		&GameServerSet{},
 		&GameServerSetList{},
 		&Fleet{},
 		&FleetList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/apis/allocation/v1/gameserverallocation.go

@@ -101,6 +101,7 @@ func (gsas *GameServerAllocationSpec) PreferredSelectors() ([]labels.Selector, e
 
 	var err error
 	for i, p := range gsas.Preferred {
+		p := p
 		list[i], err = metav1.LabelSelectorAsSelector(&p)
 		if err != nil {
 			break

pkg/apis/allocation/v1/register.go

@@ -49,11 +49,11 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&GameServerAllocation{},
 		&GameServerAllocationList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/apis/autoscaling/v1/register.go

@@ -49,11 +49,11 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&FleetAutoscaler{},
 		&FleetAutoscalerList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/apis/multicluster/v1alpha1/gameserverallocationpolicy.go

@@ -175,11 +175,11 @@ func selectRandomWeighted(connections []*ClusterConnectionInfo, weights []int) *
 		return nil
 	}
 
-	rand := rand.Intn(sum)
+	randValue := rand.Intn(sum)
 	sum = 0
 	for i, weight := range weights {
 		sum += weight
-		if rand < sum {
+		if randValue < sum {
 			return connections[i]
 		}
 	}

pkg/apis/multicluster/v1alpha1/register.go

@@ -49,11 +49,11 @@ func init() {
 }
 
 // Adds the list of known types to api.Scheme.
-func addKnownTypes(scheme *k8sruntime.Scheme) error {
-	scheme.AddKnownTypes(SchemeGroupVersion,
+func addKnownTypes(apiScheme *k8sruntime.Scheme) error {
+	apiScheme.AddKnownTypes(SchemeGroupVersion,
 		&GameServerAllocationPolicy{},
 		&GameServerAllocationPolicyList{},
 	)
-	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
+	metav1.AddToGroupVersion(apiScheme, SchemeGroupVersion)
 	return nil
 }

pkg/fleets/controller.go

@@ -386,16 +386,17 @@ func (c *Controller) deleteEmptyGameServerSets(fleet *agonesv1.Fleet, list []*ag
 // GameServerSets, and return the replica count for the active GameServerSet
 func (c *Controller) recreateDeployment(fleet *agonesv1.Fleet, rest []*agonesv1.GameServerSet) (int32, error) {
 	for _, gsSet := range rest {
-		if gsSet.Spec.Replicas != 0 {
-			c.loggerForFleet(fleet).WithField("gameserverset", gsSet.ObjectMeta.Name).Info("applying recreate deployment: scaling to 0")
-			gsSetCopy := gsSet.DeepCopy()
-			gsSetCopy.Spec.Replicas = 0
-			if _, err := c.gameServerSetGetter.GameServerSets(gsSetCopy.ObjectMeta.Namespace).Update(gsSetCopy); err != nil {
-				return 0, errors.Wrapf(err, "error updating gameserverset %s", gsSetCopy.ObjectMeta.Name)
-			}
-			c.recorder.Eventf(fleet, corev1.EventTypeNormal, "ScalingGameServerSet",
-				"Scaling inactive GameServerSet %s from %d to %d", gsSetCopy.ObjectMeta.Name, gsSet.Spec.Replicas, gsSetCopy.Spec.Replicas)
+		if gsSet.Spec.Replicas == 0 {
+			continue
+		}
+		c.loggerForFleet(fleet).WithField("gameserverset", gsSet.ObjectMeta.Name).Info("applying recreate deployment: scaling to 0")
+		gsSetCopy := gsSet.DeepCopy()
+		gsSetCopy.Spec.Replicas = 0
+		if _, err := c.gameServerSetGetter.GameServerSets(gsSetCopy.ObjectMeta.Namespace).Update(gsSetCopy); err != nil {
+			return 0, errors.Wrapf(err, "error updating gameserverset %s", gsSetCopy.ObjectMeta.Name)
 		}
+		c.recorder.Eventf(fleet, corev1.EventTypeNormal, "ScalingGameServerSet",
+			"Scaling inactive GameServerSet %s from %d to %d", gsSetCopy.ObjectMeta.Name, gsSet.Spec.Replicas, gsSetCopy.Spec.Replicas)
 	}
 
 	return fleet.LowerBoundReplicas(fleet.Spec.Replicas - agonesv1.SumStatusAllocatedReplicas(rest)), nil

pkg/fleets/controller_test.go

@@ -340,7 +340,7 @@ func TestControllerRun(t *testing.T) {
 
 	// test updating fleet
 	fCopy := fleet.DeepCopy()
-	fCopy.Spec.Replicas = fCopy.Spec.Replicas + 10
+	fCopy.Spec.Replicas += 10
 	fleetWatch.Modify(fCopy)
 	assert.Equal(t, expected, f())
 

pkg/gameserverallocations/allocator.go

@@ -238,11 +238,12 @@ func (c *Allocator) allocateFromLocalCluster(gsa *allocationv1.GameServerAllocat
 		return nil, err
 	}
 
-	if err == ErrNoGameServerReady {
+	switch err {
+	case ErrNoGameServerReady:
 		gsa.Status.State = allocationv1.GameServerAllocationUnAllocated
-	} else if err == ErrConflictInGameServerSelection {
+	case ErrConflictInGameServerSelection:
 		gsa.Status.State = allocationv1.GameServerAllocationContention
-	} else {
+	default:
 		gsa.ObjectMeta.Name = gs.ObjectMeta.Name
 		gsa.Status.State = allocationv1.GameServerAllocationAllocated
 		gsa.Status.GameServerName = gs.ObjectMeta.Name
@@ -532,7 +533,7 @@ func (c *Allocator) allocationUpdateWorkers(workerCount int, stop <-chan struct{
 			for {
 				select {
 				case res := <-updateQueue:
-					gs, err := c.readyGameServerCache.PatchGameServerMetadata(res.request.gsa.Spec.MetaPatch, *res.gs)
+					gs, err := c.readyGameServerCache.PatchGameServerMetadata(res.request.gsa.Spec.MetaPatch, res.gs)
 					if err != nil {
 						// since we could not allocate, we should put it back
 						c.readyGameServerCache.AddToReadyGameServer(gs)

pkg/gameserverallocations/cache_test.go

@@ -44,11 +44,8 @@ func TestGameServerCacheEntry(t *testing.T) {
 
 	count := 0
 	cache.Range(func(key string, gs *agonesv1.GameServer) bool {
-		if count++; count == 2 {
-			return false
-		}
-
-		return true
+		count++
+		return count != 2
 	})
 
 	assert.Equal(t, 2, count, "Should only process one item")