From db904c395dc88b31e7083aebb320218cd3d4480d Mon Sep 17 00:00:00 2001 From: Cyril TOVENA Date: Thu, 6 Sep 2018 23:05:36 -0400 Subject: [PATCH] switch to golangci-lint and fixes new lint issues. --- .gitignore | 1 + .golangci.yml | 63 +++++++++++++++++++++++++++ build/Makefile | 2 +- build/build-image/Dockerfile | 8 ++-- build/build-image/gen-lint-exclude.sh | 18 -------- examples/simple-udp/main.go | 2 +- pkg/fleets/controller.go | 1 + pkg/fleets/controller_test.go | 3 ++ pkg/gameservers/controller.go | 1 + pkg/gameservers/controller_test.go | 17 ++++---- pkg/gameservers/localsdk_test.go | 4 +- pkg/gameservers/portallocator_test.go | 4 +- pkg/gameservers/sdkserver_test.go | 8 ++-- sdks/go/sdk.go | 2 +- sdks/go/sdk_test.go | 2 +- test/e2e/framework/framework.go | 3 +- test/e2e/gameserver_test.go | 2 + 17 files changed, 99 insertions(+), 42 deletions(-) create mode 100644 .golangci.yml delete mode 100755 build/build-image/gen-lint-exclude.sh diff --git a/.gitignore b/.gitignore index 052df9ee5b..40ba7c49b7 100644 --- a/.gitignore +++ b/.gitignore @@ -17,6 +17,7 @@ !.helmignore !.gitattributes !.dockerignore +!.golangci.yml *.iml bin *.o diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 0000000000..1ea074adce --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,63 @@ +# This file contains all available configuration options +# with their default values. + +# options for analysis running +run: + # default concurrency is a available CPU number + concurrency: 4 + + # timeout for analysis, e.g. 30s, 5m, default is 1m + deadline: 5m + + # exit code when at least one issue was found, default is 1 + issues-exit-code: 1 + + # include test files or not, default is true + tests: true + + # list of build tags, all linters use it. Default is empty list. + build-tags: + + # which dirs to skip: they won't be analyzed; + # can use regexp here: generated.*, regexp is applied on full path; + # default value is empty list, but next dirs are always skipped independently + # from this option's value: + # vendor$, third_party$, testdata$, examples$, Godeps$, builtin$ + skip-dirs: + + # which files to skip: they will be analyzed, but issues from them + # won't be reported. Default value is empty list, but there is + # no need to include all autogenerated files, we confidently recognize + # autogenerated files. If it's not please let us know. + skip-files: + + +# output configuration options +output: + # colored-line-number|line-number|json|tab|checkstyle, default is "colored-line-number" + format: colored-line-number + + # print lines of code with issue, default is true + print-issued-lines: true + + # print linter name in the end of issue text, default is true + print-linter-name: true + + +linters: + enable: + - megacheck + - govet + - golint + - interfacer + - unconvert + - dupl + - goconst + - gocyclo + - goimports + - maligned + - megacheck + - misspell + - unparam + - nakedret + diff --git a/build/Makefile b/build/Makefile index 5273f5d984..292d158f15 100644 --- a/build/Makefile +++ b/build/Makefile @@ -155,7 +155,7 @@ build-controller-binary: $(ensure-build-image) lint: LINT_TIMEOUT ?= 15m lint: $(ensure-build-image) docker run --rm $(common_mounts) -w $(mount_path) $(DOCKER_RUN_ARGS) $(build_tag) bash -c \ - "/root/gen-lint-exclude.sh && gometalinter --config .exclude.gometalinter.json --deadline=$(LINT_TIMEOUT) -t --skip vendor ./..." + "golangci-lint run ./examples/... && golangci-lint run --deadline $(LINT_TIMEOUT) ./..." # Build the image for the gameserver controller build-controller-image: $(ensure-build-image) build-controller-binary diff --git a/build/build-image/Dockerfile b/build/build-image/Dockerfile index 816d1a51ca..c419621cc5 100644 --- a/build/build-image/Dockerfile +++ b/build/build-image/Dockerfile @@ -65,9 +65,11 @@ RUN mkdir -p /go/src/github.com/golang && cd /go/src/github.com/golang && \ go install github.com/golang/protobuf/protoc-gen-go # install go tooling for development, building and testing -RUN go get -u github.com/golang/dep/cmd/dep && \ - go get -u github.com/alecthomas/gometalinter && \ - /go/bin/gometalinter --install +RUN go get -u github.com/golang/dep/cmd/dep + +# install golang-ci linter +RUN curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | \ + bash -s -- -b $GOPATH/bin v1.10.2 # install the release branch of the code generator tools RUN mkdir -p /go/src && cd /go/src && mkdir -p k8s.io && cd k8s.io && \ diff --git a/build/build-image/gen-lint-exclude.sh b/build/build-image/gen-lint-exclude.sh deleted file mode 100755 index 1ba09e984c..0000000000 --- a/build/build-image/gen-lint-exclude.sh +++ /dev/null @@ -1,18 +0,0 @@ -#!/usr/bin/env bash - -# Copyright 2018 Google Inc. All Rights Reserved. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -excluded=$(find . -name "*.go" | grep -v vendor | xargs grep autogenerated | cut -d ':' -f 1 | uniq | sed 's#\./##g') -jq -n --arg inarjq -n --arg inarr "${excluded}" '{ Exclude: ($inarr | split("\n")) }' > .exclude.gometalinter.json \ No newline at end of file diff --git a/examples/simple-udp/main.go b/examples/simple-udp/main.go index 9f6e43cd4e..a767a3166d 100644 --- a/examples/simple-udp/main.go +++ b/examples/simple-udp/main.go @@ -21,12 +21,12 @@ import ( "log" "net" "os" + "strconv" "strings" "time" coresdk "agones.dev/agones/pkg/sdk" "agones.dev/agones/sdks/go" - "strconv" ) // main starts a UDP server that received 1024 byte sized packets at at time diff --git a/pkg/fleets/controller.go b/pkg/fleets/controller.go index 80fefac5bf..a01aa15c4d 100644 --- a/pkg/fleets/controller.go +++ b/pkg/fleets/controller.go @@ -121,6 +121,7 @@ func NewController( // creationMutationHandler is the handler for the mutating webhook that sets the // the default values on the Fleet // Should only be called on fleet create operations. +// nolint:dupl func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) { c.logger.WithField("review", review).Info("creationMutationHandler") diff --git a/pkg/fleets/controller_test.go b/pkg/fleets/controller_test.go index a4ac9f24d0..2e0bf73506 100644 --- a/pkg/fleets/controller_test.go +++ b/pkg/fleets/controller_test.go @@ -12,6 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. +// nolint:goconst package fleets import ( @@ -74,6 +75,7 @@ func TestControllerSyncFleet(t *testing.T) { f.Spec.Strategy.Type = appsv1.RecreateDeploymentStrategyType c, m := newFakeController() gsSet := f.GameServerSet() + gsSet.ObjectMeta.Name = "gsSet1" gsSet.ObjectMeta.UID = "4321" gsSet.Spec.Replicas = f.Spec.Replicas @@ -318,6 +320,7 @@ func TestControllerUpdateFleetStatus(t *testing.T) { gsSet1.Status.AllocatedReplicas = 1 gsSet2 := fleet.GameServerSet() + // nolint:goconst gsSet2.ObjectMeta.Name = "gsSet2" gsSet2.Status.Replicas = 5 gsSet2.Status.ReadyReplicas = 5 diff --git a/pkg/gameservers/controller.go b/pkg/gameservers/controller.go index c8a69f07fb..f2d3da2795 100644 --- a/pkg/gameservers/controller.go +++ b/pkg/gameservers/controller.go @@ -149,6 +149,7 @@ func NewController( // creationMutationHandler is the handler for the mutating webhook that sets the // the default values on the GameServer // Should only be called on gameserver create operations. +// nolint:dupl func (c *Controller) creationMutationHandler(review admv1beta1.AdmissionReview) (admv1beta1.AdmissionReview, error) { c.logger.WithField("review", review).Info("creationMutationHandler") diff --git a/pkg/gameservers/controller_test.go b/pkg/gameservers/controller_test.go index c3fa63a057..f7b2f5cc19 100644 --- a/pkg/gameservers/controller_test.go +++ b/pkg/gameservers/controller_test.go @@ -41,6 +41,8 @@ import ( "k8s.io/client-go/tools/cache" ) +const ipFixture = "12.12.12.12" + func TestControllerSyncGameServer(t *testing.T) { t.Parallel() @@ -58,7 +60,6 @@ func TestControllerSyncGameServer(t *testing.T) { }, } - ipFixture := "12.12.12.12" node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} @@ -430,7 +431,7 @@ func TestControllerSyncGameServerPortAllocationState(t *testing.T) { }) t.Run("GameServer with non zero deletion datetime", func(t *testing.T) { - testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + testWithNonZeroDeletionTimestamp(t, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { return c.syncGameServerPortAllocationState(fixture) }) }) @@ -444,7 +445,6 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { return fixture } - ipFixture := "12.12.12.12" node := corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1"}, Status: corev1.NodeStatus{Addresses: []corev1.NodeAddress{{Address: ipFixture, Type: corev1.NodeExternalIP}}}} t.Run("Syncing from Created State, with no issues", func(t *testing.T) { @@ -563,7 +563,7 @@ func TestControllerSyncGameServerCreatingState(t *testing.T) { }) t.Run("GameServer with non zero deletion datetime", func(t *testing.T) { - testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + testWithNonZeroDeletionTimestamp(t, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { return c.syncGameServerCreatingState(fixture) }) }) @@ -654,7 +654,6 @@ func TestControllerApplyGameServerAddressAndPort(t *testing.T) { t.Parallel() c, m := newFakeController() - ipFixture := "12.12.12.12" gsFixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default"}, Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.RequestReady}} gsFixture.ApplyDefaults() @@ -721,7 +720,7 @@ func TestControllerSyncGameServerRequestReadyState(t *testing.T) { } t.Run("GameServer with non zero deletion datetime", func(t *testing.T) { - testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + testWithNonZeroDeletionTimestamp(t, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { return c.syncGameServerRequestReadyState(fixture) }) }) @@ -762,7 +761,7 @@ func TestControllerSyncGameServerShutdownState(t *testing.T) { }) t.Run("GameServer with non zero deletion datetime", func(t *testing.T) { - testWithNonZeroDeletionTimestamp(t, v1alpha1.Shutdown, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { + testWithNonZeroDeletionTimestamp(t, func(c *Controller, fixture *v1alpha1.GameServer) (*v1alpha1.GameServer, error) { return fixture, c.syncGameServerShutdownState(fixture) }) }) @@ -911,11 +910,11 @@ func testNoChange(t *testing.T, state v1alpha1.State, f func(*Controller, *v1alp // testWithNonZeroDeletionTimestamp runs a test with a given state, but // the DeletionTimestamp set to Now() -func testWithNonZeroDeletionTimestamp(t *testing.T, state v1alpha1.State, f func(*Controller, *v1alpha1.GameServer) (*v1alpha1.GameServer, error)) { +func testWithNonZeroDeletionTimestamp(t *testing.T, f func(*Controller, *v1alpha1.GameServer) (*v1alpha1.GameServer, error)) { c, mocks := newFakeController() now := metav1.Now() fixture := &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "test", Namespace: "default", DeletionTimestamp: &now}, - Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: state}} + Spec: newSingleContainerSpec(), Status: v1alpha1.GameServerStatus{State: v1alpha1.Shutdown}} fixture.ApplyDefaults() updated := false mocks.AgonesClient.AddReactor("update", "gameservers", func(action k8stesting.Action) (bool, runtime.Object, error) { diff --git a/pkg/gameservers/localsdk_test.go b/pkg/gameservers/localsdk_test.go index 5852feab01..36c5ff399b 100644 --- a/pkg/gameservers/localsdk_test.go +++ b/pkg/gameservers/localsdk_test.go @@ -70,6 +70,7 @@ func TestLocalSDKWithGameServer(t *testing.T) { assert.Equal(t, fixture.ObjectMeta.Name, gs.ObjectMeta.Name) } +// nolint:dupl func TestLocalSDKServerSetLabel(t *testing.T) { fixtures := map[string]struct { gs *v1alpha1.GameServer @@ -77,7 +78,7 @@ func TestLocalSDKServerSetLabel(t *testing.T) { "default": { gs: nil, }, - "no annotation": { + "no labels": { gs: &v1alpha1.GameServer{ObjectMeta: metav1.ObjectMeta{Name: "empty"}}, }, "empty": { @@ -115,6 +116,7 @@ func TestLocalSDKServerSetLabel(t *testing.T) { } } +// nolint:dupl func TestLocalSDKServerSetAnnotation(t *testing.T) { fixtures := map[string]struct { diff --git a/pkg/gameservers/portallocator_test.go b/pkg/gameservers/portallocator_test.go index 633203817c..e8c24bcca8 100644 --- a/pkg/gameservers/portallocator_test.go +++ b/pkg/gameservers/portallocator_test.go @@ -120,7 +120,7 @@ func TestPortAllocatorAllocate(t *testing.T) { // ports between 10 and 20 for i := 10; i <= 20; i++ { var p int32 - gs, err := pa.Allocate(fixture.DeepCopy()) // nolint: vetshadow + gs, err := pa.Allocate(fixture.DeepCopy()) assert.True(t, 10 <= gs.Spec.Ports[0].HostPort && gs.Spec.Ports[0].HostPort <= 20, "%v is not between 10 and 20", p) assert.Nil(t, err) } @@ -159,7 +159,7 @@ func TestPortAllocatorAllocate(t *testing.T) { for i := 10; i <= 14; i++ { copy := morePortFixture.DeepCopy() copy.ObjectMeta.UID = types.UID(strconv.Itoa(x) + ":" + strconv.Itoa(i)) - gs, err := pa.Allocate(copy) // nolint: vetshadow + gs, err := pa.Allocate(copy) logrus.WithField("uid", copy.ObjectMeta.UID).WithField("ports", gs.Spec.Ports).WithError(err).Info("Allocated Port") assert.Nil(t, err) for _, p := range gs.Spec.Ports { diff --git a/pkg/gameservers/sdkserver_test.go b/pkg/gameservers/sdkserver_test.go index 5008b9c8a8..a36b7ef43e 100644 --- a/pkg/gameservers/sdkserver_test.go +++ b/pkg/gameservers/sdkserver_test.go @@ -297,7 +297,7 @@ func TestSidecarHealthLastUpdated(t *testing.T) { wg := sync.WaitGroup{} wg.Add(1) go func() { - err := sc.Health(stream) // nolint: vetshadow + err := sc.Health(stream) assert.Nil(t, err) wg.Done() }() @@ -350,14 +350,14 @@ func TestSidecarHealthy(t *testing.T) { wg := sync.WaitGroup{} wg.Add(1) go func() { - err := sc.Health(stream) // nolint: vetshadow + err := sc.Health(stream) assert.Nil(t, err) wg.Done() }() fixtures := map[string]struct { - disabled bool timeAdd time.Duration + disabled bool expectedHealthy bool }{ "disabled, under timeout": {disabled: true, timeAdd: time.Second, expectedHealthy: true}, @@ -620,7 +620,7 @@ func waitConnectedStreamCount(sc *SDKServer, count int) error { }) } -func asyncWatchGameServer(t *testing.T, sc *SDKServer, stream *gameServerMockStream) { +func asyncWatchGameServer(t *testing.T, sc *SDKServer, stream sdk.SDK_WatchGameServerServer) { go func() { err := sc.WatchGameServer(&sdk.Empty{}, stream) assert.Nil(t, err) diff --git a/sdks/go/sdk.go b/sdks/go/sdk.go index cc56d0bbf5..bbfc18647f 100644 --- a/sdks/go/sdk.go +++ b/sdks/go/sdk.go @@ -17,13 +17,13 @@ package sdk import ( "fmt" + "io" "time" "agones.dev/agones/pkg/sdk" "github.com/pkg/errors" "golang.org/x/net/context" "google.golang.org/grpc" - "io" ) const port = 59357 diff --git a/sdks/go/sdk_test.go b/sdks/go/sdk_test.go index 9ec8404920..db110b42ec 100644 --- a/sdks/go/sdk_test.go +++ b/sdks/go/sdk_test.go @@ -16,13 +16,13 @@ package sdk import ( "testing" + "time" "agones.dev/agones/pkg/sdk" "github.com/stretchr/testify/assert" "golang.org/x/net/context" "google.golang.org/grpc" "google.golang.org/grpc/metadata" - "time" ) func TestSDK(t *testing.T) { diff --git a/test/e2e/framework/framework.go b/test/e2e/framework/framework.go index 063788a618..b60d938602 100644 --- a/test/e2e/framework/framework.go +++ b/test/e2e/framework/framework.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package framework is a package helping setting up end-to-end testing accross a +// Package framework is a package helping setting up end-to-end testing across a // Kubernetes cluster. package framework @@ -28,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" + // required to use gcloud login see: https://github.com/kubernetes/client-go/issues/242 _ "k8s.io/client-go/plugin/pkg/client/auth/gcp" "k8s.io/client-go/tools/clientcmd" diff --git a/test/e2e/gameserver_test.go b/test/e2e/gameserver_test.go index 9f5170733e..6d74d751cd 100644 --- a/test/e2e/gameserver_test.go +++ b/test/e2e/gameserver_test.go @@ -51,6 +51,7 @@ func TestCreateConnect(t *testing.T) { assert.Equal(t, reply, "ACK: Hello World !\n") } +// nolint:dupl func TestSDKSetLabel(t *testing.T) { t.Parallel() gs := defaultGameServer() @@ -82,6 +83,7 @@ func TestSDKSetLabel(t *testing.T) { assert.NotEmpty(t, gs.ObjectMeta.Labels["stable.agones.dev/sdk-timestamp"]) } +// nolint:dupl func TestSDKSetAnnotation(t *testing.T) { t.Parallel() gs := defaultGameServer()