From fd19c6a64e7bd1c2c0aa5e119bdd3b1bbec53ae6 Mon Sep 17 00:00:00 2001 From: Michael Cristina Date: Tue, 11 Feb 2020 11:37:40 -0600 Subject: [PATCH] Fix linter errors, update and move lint to config file --- .golangci.yml | 22 +++++++++++ hack/check-everything.sh | 2 +- hack/verify.sh | 26 +------------ pkg/client/client_cache.go | 6 +-- pkg/client/fake/client_test.go | 39 ++++++++++--------- pkg/controller/controller_integration_test.go | 4 +- pkg/handler/eventhandler_test.go | 1 - pkg/internal/controller/controller.go | 2 +- pkg/leaderelection/fake/leader_election.go | 2 +- pkg/log/zap/zap_test.go | 2 + pkg/manager/internal.go | 16 +++----- pkg/manager/signals/signal_test.go | 2 +- pkg/reconcile/example_test.go | 1 - pkg/webhook/server.go | 2 +- 14 files changed, 60 insertions(+), 67 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index a418256a4a..2994215587 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,5 +1,27 @@ +linters: + disable-all: true + enable: + - misspell + - structcheck + - golint + - deadcode + - errcheck + - varcheck + - goconst + - unparam + - ineffassign + - nakedret + - interfacer + - gocyclo + - lll + - dupl + - goimports + linters-settings: lll: line-length: 170 dupl: threshold: 400 + +run: + timeout: 5m diff --git a/hack/check-everything.sh b/hack/check-everything.sh index dd8b472fd8..6117618f18 100755 --- a/hack/check-everything.sh +++ b/hack/check-everything.sh @@ -78,7 +78,7 @@ function golangci_lint_has_version { function fetch_go_tools { header_text "Checking for golangci-lint" - local golangci_lint_version="v1.21.0" + local golangci_lint_version="v1.23.6" if ! is_installed golangci-lint || ! golangci_lint_has_version "${golangci_lint_version}"; then header_text "Installing golangci-lint" curl -sfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh| sh -s -- -b $(go env GOPATH)/bin "${golangci_lint_version}" diff --git a/hack/verify.sh b/hack/verify.sh index 9b25f7c487..2a8a902de4 100755 --- a/hack/verify.sh +++ b/hack/verify.sh @@ -24,30 +24,6 @@ go vet ${MOD_OPT} ./... header_text "running golangci-lint" -golangci-lint run --disable-all \ - --deadline 5m \ - --enable=misspell \ - --enable=structcheck \ - --enable=golint \ - --enable=deadcode \ - --enable=errcheck \ - --enable=varcheck \ - --enable=goconst \ - --enable=unparam \ - --enable=ineffassign \ - --enable=nakedret \ - --enable=interfacer \ - --enable=misspell \ - --enable=gocyclo \ - --enable=lll \ - --enable=dupl \ - --enable=goimports \ - --enable=bodyclose \ - ./pkg/... ./examples/... . - -# TODO: Enable these as we fix them to make them pass -# --enable=gosec \ -# --enable=maligned \ -# --enable=safesql \ +golangci-lint run ./pkg/... ./examples/... . GO111MODULES=on go list -mod=readonly ./... diff --git a/pkg/client/client_cache.go b/pkg/client/client_cache.go index 2a1ff05d50..f6a7e82e86 100644 --- a/pkg/client/client_cache.go +++ b/pkg/client/client_cache.go @@ -124,10 +124,8 @@ type resourceMeta struct { // isNamespaced returns true if the type is namespaced func (r *resourceMeta) isNamespaced() bool { - if r.mapping.Scope.Name() == meta.RESTScopeNameRoot { - return false - } - return true + return r.mapping.Scope.Name() != meta.RESTScopeNameRoot + } // resource returns the resource name of the type diff --git a/pkg/client/fake/client_test.go b/pkg/client/fake/client_test.go index 63d64d061c..d8536ea387 100644 --- a/pkg/client/fake/client_test.go +++ b/pkg/client/fake/client_test.go @@ -17,6 +17,7 @@ limitations under the License. package fake import ( + "context" "encoding/json" . "github.com/onsi/ginkgo" @@ -85,7 +86,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns1", } obj := &appsv1.Deployment{} - err := cl.Get(nil, namespacedName, obj) + err := cl.Get(context.Background(), namespacedName, obj) Expect(err).To(BeNil()) Expect(obj).To(Equal(dep)) }) @@ -99,14 +100,14 @@ var _ = Describe("Fake client", func() { obj := &unstructured.Unstructured{} obj.SetAPIVersion("apps/v1") obj.SetKind("Deployment") - err := cl.Get(nil, namespacedName, obj) + err := cl.Get(context.Background(), namespacedName, obj) Expect(err).To(BeNil()) }) It("should be able to List", func() { By("Listing all deployments in a namespace") list := &appsv1.DeploymentList{} - err := cl.List(nil, list, client.InNamespace("ns1")) + err := cl.List(context.Background(), list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(2)) Expect(list.Items).To(ConsistOf(*dep, *dep2)) @@ -117,7 +118,7 @@ var _ = Describe("Fake client", func() { list := &unstructured.UnstructuredList{} list.SetAPIVersion("apps/v1") list.SetKind("DeploymentList") - err := cl.List(nil, list, client.InNamespace("ns1")) + err := cl.List(context.Background(), list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(2)) }) @@ -125,7 +126,7 @@ var _ = Describe("Fake client", func() { It("should support filtering by labels and their values", func() { By("Listing deployments with a particular label and value") list := &appsv1.DeploymentList{} - err := cl.List(nil, list, client.InNamespace("ns1"), + err := cl.List(context.Background(), list, client.InNamespace("ns1"), client.MatchingLabels(map[string]string{ "test-label": "label-value", })) @@ -156,7 +157,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns2", }, } - err := cl.Create(nil, newcm) + err := cl.Create(context.Background(), newcm) Expect(err).To(BeNil()) By("Getting the new configmap") @@ -165,7 +166,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns2", } obj := &corev1.ConfigMap{} - err = cl.Get(nil, namespacedName, obj) + err = cl.Get(context.Background(), namespacedName, obj) Expect(err).To(BeNil()) Expect(obj).To(Equal(newcm)) Expect(obj.ObjectMeta.ResourceVersion).To(Equal("1")) @@ -187,7 +188,7 @@ var _ = Describe("Fake client", func() { "test-key": "new-value", }, } - err := cl.Update(nil, newcm) + err := cl.Update(context.Background(), newcm) Expect(err).To(BeNil()) By("Getting the new configmap") @@ -196,7 +197,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns2", } obj := &corev1.ConfigMap{} - err = cl.Get(nil, namespacedName, obj) + err = cl.Get(context.Background(), namespacedName, obj) Expect(err).To(BeNil()) Expect(obj).To(Equal(newcm)) Expect(obj.ObjectMeta.ResourceVersion).To(Equal("2")) @@ -204,12 +205,12 @@ var _ = Describe("Fake client", func() { It("should be able to Delete", func() { By("Deleting a deployment") - err := cl.Delete(nil, dep) + err := cl.Delete(context.Background(), dep) Expect(err).To(BeNil()) By("Listing all deployments in the namespace") list := &appsv1.DeploymentList{} - err = cl.List(nil, list, client.InNamespace("ns1")) + err = cl.List(context.Background(), list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(HaveLen(1)) Expect(list.Items).To(ConsistOf(*dep2)) @@ -217,12 +218,12 @@ var _ = Describe("Fake client", func() { It("should be able to Delete a Collection", func() { By("Deleting a deploymentList") - err := cl.DeleteAllOf(nil, &appsv1.Deployment{}, client.InNamespace("ns1")) + err := cl.DeleteAllOf(context.Background(), &appsv1.Deployment{}, client.InNamespace("ns1")) Expect(err).To(BeNil()) By("Listing all deployments in the namespace") list := &appsv1.DeploymentList{} - err = cl.List(nil, list, client.InNamespace("ns1")) + err = cl.List(context.Background(), list, client.InNamespace("ns1")) Expect(err).To(BeNil()) Expect(list.Items).To(BeEmpty()) }) @@ -236,7 +237,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns2", }, } - err := cl.Create(nil, newcm, client.CreateDryRunAll) + err := cl.Create(context.Background(), newcm, client.DryRunAll) Expect(err).To(BeNil()) By("Getting the new configmap") @@ -245,7 +246,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns2", } obj := &corev1.ConfigMap{} - err = cl.Get(nil, namespacedName, obj) + err = cl.Get(context.Background(), namespacedName, obj) Expect(err).To(HaveOccurred()) Expect(errors.IsNotFound(err)).To(BeTrue()) Expect(obj).NotTo(Equal(newcm)) @@ -263,7 +264,7 @@ var _ = Describe("Fake client", func() { "test-key": "new-value", }, } - err := cl.Update(nil, newcm, client.UpdateDryRunAll) + err := cl.Update(context.Background(), newcm, client.DryRunAll) Expect(err).To(BeNil()) By("Getting the new configmap") @@ -272,7 +273,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns2", } obj := &corev1.ConfigMap{} - err = cl.Get(nil, namespacedName, obj) + err = cl.Get(context.Background(), namespacedName, obj) Expect(err).To(BeNil()) Expect(obj).To(Equal(cm)) Expect(obj.ObjectMeta.ResourceVersion).To(Equal("")) @@ -289,7 +290,7 @@ var _ = Describe("Fake client", func() { }, }) Expect(err).NotTo(HaveOccurred()) - err = cl.Patch(nil, dep, client.RawPatch(types.StrategicMergePatchType, mergePatch)) + err = cl.Patch(context.Background(), dep, client.RawPatch(types.StrategicMergePatchType, mergePatch)) Expect(err).NotTo(HaveOccurred()) By("Getting the patched deployment") @@ -298,7 +299,7 @@ var _ = Describe("Fake client", func() { Namespace: "ns1", } obj := &appsv1.Deployment{} - err = cl.Get(nil, namespacedName, obj) + err = cl.Get(context.Background(), namespacedName, obj) Expect(err).NotTo(HaveOccurred()) Expect(obj.Annotations["foo"]).To(Equal("bar")) Expect(obj.ObjectMeta.ResourceVersion).To(Equal("1")) diff --git a/pkg/controller/controller_integration_test.go b/pkg/controller/controller_integration_test.go index 3fbf8a199d..cd54686284 100644 --- a/pkg/controller/controller_integration_test.go +++ b/pkg/controller/controller_integration_test.go @@ -122,7 +122,7 @@ var _ = Describe("controller", func() { By("Invoking Reconciling for Update") newDeployment := deployment.DeepCopy() newDeployment.Labels = map[string]string{"foo": "bar"} - newDeployment, err = clientset.AppsV1().Deployments("default").Update(newDeployment) + _, err = clientset.AppsV1().Deployments("default").Update(newDeployment) Expect(err).NotTo(HaveOccurred()) Expect(<-reconciled).To(Equal(expectedReconcileRequest)) @@ -152,7 +152,7 @@ var _ = Describe("controller", func() { By("Invoking Reconciling for an OwnedObject when it is updated") newReplicaset := replicaset.DeepCopy() newReplicaset.Labels = map[string]string{"foo": "bar"} - newReplicaset, err = clientset.AppsV1().ReplicaSets("default").Update(newReplicaset) + _, err = clientset.AppsV1().ReplicaSets("default").Update(newReplicaset) Expect(err).NotTo(HaveOccurred()) Expect(<-reconciled).To(Equal(expectedReconcileRequest)) diff --git a/pkg/handler/eventhandler_test.go b/pkg/handler/eventhandler_test.go index 13901955a4..3587afe32c 100644 --- a/pkg/handler/eventhandler_test.go +++ b/pkg/handler/eventhandler_test.go @@ -42,7 +42,6 @@ var _ = Describe("Eventhandler", func() { t := true BeforeEach(func() { q = controllertest.Queue{Interface: workqueue.New()} - instance = handler.EnqueueRequestForObject{} pod = &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{Namespace: "biz", Name: "baz"}, } diff --git a/pkg/internal/controller/controller.go b/pkg/internal/controller/controller.go index 0938fd4ec7..946ef82852 100644 --- a/pkg/internal/controller/controller.go +++ b/pkg/internal/controller/controller.go @@ -236,7 +236,7 @@ func (c *Controller) reconcileHandler(obj interface{}) bool { // Update metrics after processing each item reconcileStartTS := time.Now() defer func() { - c.updateMetrics(time.Now().Sub(reconcileStartTS)) + c.updateMetrics(time.Since(reconcileStartTS)) }() var req reconcile.Request diff --git a/pkg/leaderelection/fake/leader_election.go b/pkg/leaderelection/fake/leader_election.go index 8f0bd4ba15..7bc88d82af 100644 --- a/pkg/leaderelection/fake/leader_election.go +++ b/pkg/leaderelection/fake/leader_election.go @@ -81,7 +81,7 @@ func (f *ResourceLock) Update(ler resourcelock.LeaderElectionRecord) error { // RecordEvent implements the ResourceLockInterface. func (f *ResourceLock) RecordEvent(s string) { - return + } // Identity implements the ResourceLockInterface. diff --git a/pkg/log/zap/zap_test.go b/pkg/log/zap/zap_test.go index 461cf83d03..72e9034c49 100644 --- a/pkg/log/zap/zap_test.go +++ b/pkg/log/zap/zap_test.go @@ -58,6 +58,8 @@ type fakeLoggerRoot struct { messages []logInfo } +var _ logr.Logger = &fakeLogger{} + // fakeLogger is a fake implementation of logr.Logger that records // messages, tags, and names, // just records the name. diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 5ece5ef7eb..2f969fcd36 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -360,11 +360,9 @@ func (cm *controllerManager) serveMetrics(stop <-chan struct{}) { }() // Shutdown the server when stop is closed - select { - case <-stop: - if err := server.Shutdown(context.Background()); err != nil { - cm.errSignal.SignalError(err) - } + <-stop + if err := server.Shutdown(context.Background()); err != nil { + cm.errSignal.SignalError(err) } } @@ -392,11 +390,9 @@ func (cm *controllerManager) serveHealthProbes(stop <-chan struct{}) { cm.mu.Unlock() // Shutdown the server when stop is closed - select { - case <-stop: - if err := server.Shutdown(context.Background()); err != nil { - cm.errSignal.SignalError(err) - } + <-stop + if err := server.Shutdown(context.Background()); err != nil { + cm.errSignal.SignalError(err) } } diff --git a/pkg/manager/signals/signal_test.go b/pkg/manager/signals/signal_test.go index a2477da69c..8ac66ceaf9 100644 --- a/pkg/manager/signals/signal_test.go +++ b/pkg/manager/signals/signal_test.go @@ -36,7 +36,7 @@ var _ = Describe("runtime signal", func() { task := &Task{ ticker: time.NewTicker(time.Second * 2), } - c := make(chan os.Signal) + c := make(chan os.Signal, 1) signal.Notify(c, os.Interrupt) task.wg.Add(1) go func(c chan os.Signal) { diff --git a/pkg/reconcile/example_test.go b/pkg/reconcile/example_test.go index 55e0e976c9..1e3893c022 100644 --- a/pkg/reconcile/example_test.go +++ b/pkg/reconcile/example_test.go @@ -26,7 +26,6 @@ import ( // This example implements a simple no-op reconcile function that prints the object to be Reconciled. func ExampleFunc() { - type Reconciler struct{} r := reconcile.Func(func(o reconcile.Request) (reconcile.Result, error) { // Create your business logic to create, update, delete objects here. diff --git a/pkg/webhook/server.go b/pkg/webhook/server.go index 61ed862197..7005aa2fa5 100644 --- a/pkg/webhook/server.go +++ b/pkg/webhook/server.go @@ -119,7 +119,7 @@ func (s *Server) Register(path string, hook http.Handler) { func instrumentedHook(path string, hookRaw http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { startTS := time.Now() - defer func() { metrics.RequestLatency.WithLabelValues(path).Observe(time.Now().Sub(startTS).Seconds()) }() + defer func() { metrics.RequestLatency.WithLabelValues(path).Observe(time.Since(startTS).Seconds()) }() hookRaw.ServeHTTP(resp, req) // TODO(directxman12): add back in metric about total requests broken down by result?