🌱 Update golangci-lint / enable more linters (kcp-dev#1907)

* update golangci-lint to 1.49 * fix nolintlint errors * fix unconvert errors * fix gocritic errors * fix nolintlint errors * enable more linters by default * fix errorlint exceptions, remove dead code * do not swallow errors from running external process * post-rebase fixes * fix tab in comment * do not use ErrorS * fix typo in embeddedetcd config
ramramu3433 · Sep 13, 2022 · f197112 · f197112
1 parent ea31363
commit f197112
Show file tree

Hide file tree

Showing 51 changed files with 119 additions and 158 deletions.
diff --git a/.golangci.yaml b/.golangci.yaml
@@ -1,17 +1,24 @@
 linters-settings:
-  errorlint:
-    errorf: true
-    asserts: true
-    comparison: true
   misspell:
     ignore-words:
     - creater
 
 linters:
   enable:
+  - asciicheck
+  - bidichk
+  - durationcheck
+  - errname
   - errorlint
   - exportloopref
   - gofmt
+  - gosimple
+  - govet
   - importas
+  - ineffassign
   - misspell
   - nilerr
+  - unconvert
+  - unused
+  - errcheck
+  - nolintlint
diff --git a/Makefile b/Makefile
@@ -38,7 +38,7 @@ OPENSHIFT_GOIMPORTS_BIN := openshift-goimports
 OPENSHIFT_GOIMPORTS := $(TOOLS_DIR)/$(OPENSHIFT_GOIMPORTS_BIN)-$(OPENSHIFT_GOIMPORTS_VER)
 export OPENSHIFT_GOIMPORTS # so hack scripts can use it
 
-GOLANGCI_LINT_VER := v1.47.2
+GOLANGCI_LINT_VER := v1.49.0
 GOLANGCI_LINT_BIN := golangci-lint
 GOLANGCI_LINT := $(TOOLS_GOBIN_DIR)/$(GOLANGCI_LINT_BIN)-$(GOLANGCI_LINT_VER)
 

diff --git a/cmd/crd-puller/pull-crds.go b/cmd/crd-puller/pull-crds.go
@@ -62,7 +62,7 @@ func main() {
 				if err != nil {
 					return err
 				}
-				if err := ioutil.WriteFile(name.String()+".yaml", []byte(yamlBytes), os.ModePerm); err != nil {
+				if err := ioutil.WriteFile(name.String()+".yaml", yamlBytes, os.ModePerm); err != nil {
 					return err
 				}
 			}

diff --git a/cmd/sharded-test-server/frontproxy.go b/cmd/sharded-test-server/frontproxy.go
@@ -18,6 +18,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"net/http"
@@ -133,7 +134,7 @@ func startFrontProxy(
 		"--secure-port=6443",
 	)
 	commandLine = append(commandLine, args...)
-	fmt.Fprintf(out, "running: %v\n", strings.Join(commandLine, " ")) // nolint: errcheck
+	fmt.Fprintf(out, "running: %v\n", strings.Join(commandLine, " "))
 
 	cmd := exec.CommandContext(ctx, commandLine[0], commandLine[1:]...)
 
@@ -158,13 +159,16 @@ func startFrontProxy(
 
 	go func() {
 		<-ctx.Done()
-		cmd.Process.Kill() // nolint: errcheck
+		if err := cmd.Process.Kill(); err != nil {
+			klog.FromContext(ctx).Error(err, "failed to kill process")
+		}
 	}()
 
 	terminatedCh := make(chan int, 1)
 	go func() {
 		if err := cmd.Wait(); err != nil {
-			if exitErr, ok := err.(*exec.ExitError); ok { // nolint: errorlint
+			var exitErr *exec.ExitError
+			if errors.As(err, &exitErr) {
 				terminatedCh <- exitErr.ExitCode()
 			}
 		} else {
@@ -218,7 +222,7 @@ func startFrontProxy(
 	if !klog.V(3).Enabled() {
 		writer.StopOut()
 	}
-	fmt.Fprintf(successOut, "kcp-front-proxy is ready\n") // nolint: errcheck
+	fmt.Fprintf(successOut, "kcp-front-proxy is ready\n")
 
 	return nil
 }

diff --git a/cmd/sharded-test-server/main.go b/cmd/sharded-test-server/main.go
@@ -50,7 +50,7 @@ func main() {
 			genericFlags = append(genericFlags, arg)
 		}
 	}
-	flag.CommandLine.Parse(genericFlags) // nolint: errcheck
+	flag.CommandLine.Parse(genericFlags) //nolint:errcheck
 
 	if err := start(proxyFlags, shardFlags, *logDirPath, *workDirPath, *numberOfShards); err != nil {
 		fmt.Println(err.Error())

diff --git a/cmd/sharded-test-server/virtual.go b/cmd/sharded-test-server/virtual.go
@@ -54,7 +54,7 @@ func startVirtual(ctx context.Context, index int, logDirPath string) (<-chan err
 		"--requestheader-group-headers=X-Remote-Group",
 		fmt.Sprintf("--secure-port=%d", 7444+index),
 	)
-	fmt.Fprintf(out, "running: %v\n", strings.Join(commandLine, " ")) // nolint: errcheck
+	fmt.Fprintf(out, "running: %v\n", strings.Join(commandLine, " "))
 
 	cmd := exec.CommandContext(ctx, commandLine[0], commandLine[1:]...)
 

diff --git a/cmd/test-server/helpers/writer.go b/cmd/test-server/helpers/writer.go
@@ -49,13 +49,13 @@ func (hw *headWriter) Write(p []byte) (n int, err error) {
 			return hw.file.Write(p)
 		}
 		if pos := strings.Index(string(p), "\n"); pos == -1 {
-			hw.out.Write(p) // nolint: errcheck
+			hw.out.Write(p) //nolint:errcheck
 		} else {
 			hw.linePending = false
-			hw.out.Write(p[:pos+1]) // nolint: errcheck
+			hw.out.Write(p[:pos+1]) //nolint:errcheck
 		}
 	default:
-		hw.out.Write(p) // nolint: errcheck
+		hw.out.Write(p) //nolint:errcheck
 	}
 	return hw.file.Write(p)
 }
diff --git a/cmd/test-server/kcp/shard.go b/cmd/test-server/kcp/shard.go
@@ -19,6 +19,7 @@ package shard
 import (
 	"context"
 	"embed"
+	"errors"
 	"fmt"
 	"net/http"
 	"os"
@@ -83,7 +84,7 @@ func Start(ctx context.Context, name, runtimeDir, logFilePath string, args []str
 		"--audit-log-batch-throttle-qps=10",
 		"--audit-policy-file", filepath.Join(runtimeDir, "audit-policy.yaml"),
 	)
-	fmt.Fprintf(out, "running: %v\n", strings.Join(commandLine, " ")) // nolint: errcheck
+	fmt.Fprintf(out, "running: %v\n", strings.Join(commandLine, " "))
 
 	cmd := exec.CommandContext(ctx, commandLine[0], commandLine[1:]...)
 	if err := os.MkdirAll(filepath.Dir(logFilePath), 0755); err != nil {
@@ -105,7 +106,9 @@ func Start(ctx context.Context, name, runtimeDir, logFilePath string, args []str
 
 	go func() {
 		<-ctx.Done()
-		cmd.Process.Kill() // nolint: errcheck
+		if err := cmd.Process.Kill(); err != nil {
+			klog.FromContext(ctx).Error(err, "failed to kill process")
+		}
 	}()
 
 	terminatedCh := make(chan error, 1)
@@ -121,9 +124,10 @@ func Start(ctx context.Context, name, runtimeDir, logFilePath string, args []str
 		case <-ctx.Done():
 			return nil, fmt.Errorf("context canceled")
 		case err := <-terminatedCh:
+			var exitErr *exec.ExitError
 			if err == nil {
 				return nil, fmt.Errorf("kcp shard %s terminated unexpectedly with exit code 0", name)
-			} else if exitErr, ok := err.(*exec.ExitError); ok { // nolint: errorlint
+			} else if errors.As(err, &exitErr) {
 				return nil, fmt.Errorf("kcp shard %s terminated with exit code %d", name, exitErr.ExitCode())
 			}
 			return nil, fmt.Errorf("kcp shard %s terminated with unknown error: %w", name, err)
@@ -145,9 +149,10 @@ func Start(ctx context.Context, name, runtimeDir, logFilePath string, args []str
 		case <-ctx.Done():
 			return nil, fmt.Errorf("context canceled")
 		case err := <-terminatedCh:
+			var exitErr *exec.ExitError
 			if err == nil {
 				return nil, fmt.Errorf("kcp shard %s terminated unexpectedly with exit code 0", name)
-			} else if exitErr, ok := err.(*exec.ExitError); ok { // nolint: errorlint
+			} else if errors.As(err, &exitErr) {
 				return nil, fmt.Errorf("kcp shard %s terminated with exit code %d", name, exitErr.ExitCode())
 			}
 			return nil, fmt.Errorf("kcp shard %s terminated with unknown error: %w", name, err)
@@ -187,7 +192,7 @@ func Start(ctx context.Context, name, runtimeDir, logFilePath string, args []str
 	if !klog.V(3).Enabled() {
 		writer.StopOut()
 	}
-	fmt.Fprintf(successOut, "shard is ready\n") // nolint: errcheck
+	fmt.Fprintf(successOut, "shard is ready\n")
 
 	return terminatedCh, nil
 }
diff --git a/cmd/test-server/main.go b/cmd/test-server/main.go
@@ -18,6 +18,7 @@ package main
 
 import (
 	"context"
+	"errors"
 	"flag"
 	"fmt"
 	"os"
@@ -35,16 +36,15 @@ import (
 //
 // Repeatably start a persistent test server:
 //
-//   $ rm -rf .kcp/ && make build && ./bin/test-server 2>&1 | tee kcp.log
+//	$ rm -rf .kcp/ && make build && ./bin/test-server 2>&1 | tee kcp.log
 //
 // Run the e2e suite against a persistent server:
 //
-//   $ TEST_ARGS='-args --use-default-kcp-server' E2E_PARALLELISM=6 make test-e2e
+//	$ TEST_ARGS='-args --use-default-kcp-server' E2E_PARALLELISM=6 make test-e2e
 //
 // Run individual tests against a persistent server:
 //
-//   $ go test -v --use-default-kcp-server
-//
+//	$ go test -v --use-default-kcp-server
 func main() {
 	flag.String("log-file-path", ".kcp/kcp.log", "Path to the log file")
 
@@ -62,14 +62,14 @@ func main() {
 			genericFlags = append(genericFlags, arg)
 		}
 	}
-	flag.CommandLine.Parse(genericFlags) // nolint: errcheck
+	flag.CommandLine.Parse(genericFlags) //nolint:errcheck
 
 	if err := start(shardFlags); err != nil {
-		// nolint: errorlint
-		if err, ok := err.(*exec.ExitError); ok {
-			os.Exit(err.ExitCode())
+		var exitErr *exec.ExitError
+		if errors.As(err, &exitErr) {
+			os.Exit(exitErr.ExitCode())
 		}
-		fmt.Fprintf(os.Stderr, "error: %v\n", err) // nolint:errcheck
+		fmt.Fprintf(os.Stderr, "error: %v\n", err)
 		os.Exit(1)
 	}
 }

diff --git a/pkg/admission/apiresourceschema/admission_test.go b/pkg/admission/apiresourceschema/admission_test.go
@@ -31,7 +31,6 @@ import (
 
 	"github.com/kcp-dev/kcp/pkg/admission/helpers"
 	apisv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/apis/v1alpha1"
-	tenancyv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/tenancy/v1alpha1"
 )
 
 func createAttr(s *apisv1alpha1.APIResourceSchema) admission.Attributes {
@@ -50,23 +49,6 @@ func createAttr(s *apisv1alpha1.APIResourceSchema) admission.Attributes {
 	)
 }
 
-// nolint:deadcode,unused
-func updateAttr(s, old *apisv1alpha1.APIResourceSchema) admission.Attributes {
-	return admission.NewAttributesRecord(
-		helpers.ToUnstructuredOrDie(s),
-		helpers.ToUnstructuredOrDie(old),
-		tenancyv1alpha1.Kind("APIResourceSchema").WithVersion("v1alpha1"),
-		"",
-		s.Name,
-		tenancyv1alpha1.Resource("apiresourceschemas").WithVersion("v1alpha1"),
-		"",
-		admission.Update,
-		&metav1.CreateOptions{},
-		false,
-		&user.DefaultInfo{},
-	)
-}
-
 func TestValidate(t *testing.T) {
 	tests := []struct {
 		name           string

diff --git a/pkg/admission/clusterworkspacetypeexists/admission_test.go b/pkg/admission/clusterworkspacetypeexists/admission_test.go
@@ -806,7 +806,7 @@ func TestValidateAllowedParents(t *testing.T) {
 			}
 
 			// inverse child/parent inputs for validateAllowedChildren, fully symmetric
-			wantErr := strings.Replace(tt.wantErr, "parent", "child", -1)
+			wantErr := strings.ReplaceAll(tt.wantErr, "parent", "child")
 			childAliases := make([]*tenancyv1alpha1.ClusterWorkspaceType, len(tt.parentAliases))
 			for i, parent := range tt.parentAliases {
 				parent = parent.DeepCopy()

diff --git a/pkg/admission/mutatingwebhook/plugin.go b/pkg/admission/mutatingwebhook/plugin.go
@@ -84,7 +84,7 @@ func NewValidatingAdmissionWebhook(configfile io.Reader) (*Plugin, error) {
 		return nil, err
 	}
 
-	//Override the ready func
+	// Override the ready func
 
 	p.SetReadyFunc(func() bool {
 		if p.WebhookDispatcher.HasSynced() && p.Plugin.WaitForReady() {

diff --git a/pkg/admission/reservedmetadata/admission.go b/pkg/admission/reservedmetadata/admission.go
@@ -76,7 +76,7 @@ var _ = admission.ValidationInterface(&reservedMetadata{})
 // If the user is member of the "system:masters" group, all mutations are allowed.
 func (o *reservedMetadata) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) (err error) {
 	newMeta, err := meta.Accessor(a.GetObject())
-	// nolint: nilerr
+	//nolint:nilerr
 	if err != nil {
 		// The object we are dealing with doesn't have object metadata defined
 		// hence it doesn't have annotations to be checked.

diff --git a/pkg/admission/validatingwebhook/plugin.go b/pkg/admission/validatingwebhook/plugin.go
@@ -82,7 +82,7 @@ func NewValidatingAdmissionWebhook(configfile io.Reader) (*Plugin, error) {
 		return nil, err
 	}
 
-	//Override the ready func
+	// Override the ready func
 
 	p.SetReadyFunc(func() bool {
 		if p.WebhookDispatcher.HasSynced() && p.Plugin.WaitForReady() {

diff --git a/pkg/authorization/apibinding_authorizer.go b/pkg/authorization/apibinding_authorizer.go
@@ -215,7 +215,7 @@ func (a *apiBindingAccessAuthorizer) Authorize(ctx context.Context, attr authori
 	return authorizer.DecisionNoOpinion, reason, nil
 }
 
-//TODO [shawn-hurley]: this should be a helper shared.
+// TODO [shawn-hurley]: this should be a helper shared.
 func (a *apiBindingAccessAuthorizer) getAPIBindingReference(attr authorizer.Attributes, clusterName logicalcluster.Name) (*apisv1alpha1.ExportReference, bool, error) {
 	objs, err := a.apiBindingIndexer.ByIndex(byWorkspaceIndex, clusterName.String())
 	if err != nil {

diff --git a/pkg/cache/server/server.go b/pkg/cache/server/server.go
@@ -56,7 +56,6 @@ func (s *Server) PrepareRun(ctx context.Context) (preparedServer, error) {
 		logger = logger.WithValues("postStartHook", "bootstrap-cache-server")
 		if err := bootstrap.Bootstrap(klog.NewContext(goContext(hookContext), logger), s.ApiExtensionsClusterClient); err != nil {
 			logger.Error(err, "failed creating the static CustomResourcesDefinitions")
-			// nolint:nilerr
 			return nil // don't klog.Fatal. This only happens when context is cancelled.
 		}
 		return nil

diff --git a/pkg/cliplugins/workload/plugin/sync.go b/pkg/cliplugins/workload/plugin/sync.go
@@ -192,7 +192,7 @@ func (o *SyncOptions) Run(ctx context.Context) error {
 		if err != nil {
 			return err
 		}
-		defer outputFile.Close() // nolint: errcheck
+		defer outputFile.Close()
 	}
 
 	token, syncerID, syncTargetUID, err := o.enableSyncerForWorkspace(ctx, config, o.SyncTargetName, o.KCPNamespace)
@@ -240,7 +240,6 @@ func (o *SyncOptions) Run(ctx context.Context) error {
 
 	_, err = outputFile.Write(resources)
 	if o.OutputFile != "-" {
-		// nolint: errcheck
 		fmt.Fprintf(o.ErrOut, "\nWrote physical cluster manifest to %s for namespace %q. Use\n\n  KUBECONFIG=<pcluster-config> kubectl apply -f %q\n\nto apply it. "+
 			"Use\n\n  KUBECONFIG=<pcluster-config> kubectl get deployment -n %q %s\n\nto verify the syncer pod is running.\n", o.OutputFile, o.DownstreamNamespace, o.OutputFile, o.DownstreamNamespace, syncerID)
 	}
@@ -274,7 +273,6 @@ func (o *SyncOptions) enableSyncerForWorkspace(ctx context.Context, config *rest
 		// Create the sync target that will serve as a point of coordination between
 		// kcp and the syncer (e.g. heartbeating from the syncer and virtual cluster urls
 		// to the syncer).
-		// nolint: errcheck
 		fmt.Fprintf(o.ErrOut, "Creating synctarget %q\n", syncTargetName)
 		syncTarget, err = kcpClient.WorkloadV1alpha1().SyncTargets().Create(ctx,
 			&workloadv1alpha1.SyncTarget{
@@ -288,7 +286,6 @@ func (o *SyncOptions) enableSyncerForWorkspace(ctx context.Context, config *rest
 			return "", "", "", fmt.Errorf("failed to create synctarget %q: %w", syncTargetName, err)
 		}
 	} else if err == nil {
-		// nolint: errcheck
 		fmt.Fprintf(o.ErrOut, "Synctarget %q already exists.\n", syncTargetName)
 	}
 

diff --git a/pkg/crdpuller/discovery.go b/pkg/crdpuller/discovery.go
@@ -167,9 +167,9 @@ func (sp *schemaPuller) PullCRDs(context context.Context, resourceNames ...strin
 
 			crdName := apiResource.Name
 			if gv.Group == "" {
-				crdName = crdName + ".core"
+				crdName += ".core"
 			} else {
-				crdName = crdName + "." + gv.Group
+				crdName += "." + gv.Group
 			}
 
 			var resourceScope apiextensionsv1.ResourceScope

diff --git a/pkg/embeddedetcd/config.go b/pkg/embeddedetcd/config.go
@@ -98,7 +98,7 @@ func NewConfig(o options.CompletedOptions, enableWatchCache bool) (*Config, erro
 		if err != nil {
 			return nil, err
 		}
-		cfg.ListenMetricsUrls = append(cfg.LPUrls, *u)
+		cfg.ListenMetricsUrls = append(cfg.ListenMetricsUrls, *u)
 	}
 
 	if enableUnsafeEtcdDisableFsyncHack, _ := strconv.ParseBool(os.Getenv("UNSAFE_E2E_HACK_DISABLE_ETCD_FSYNC")); enableUnsafeEtcdDisableFsyncHack {

diff --git a/pkg/localenvoy/controllers/ingress/envoyingress.go b/pkg/localenvoy/controllers/ingress/envoyingress.go
@@ -70,7 +70,6 @@ func (c *Controller) reconcile(ctx context.Context, ingress *networkingv1.Ingres
 // TODO(jmprusi): Review the hash algorithm.
 func domainHashString(s string) string {
 	h := fnv.New32a()
-	// nolint: errcheck
 	h.Write([]byte(s))
 	return fmt.Sprint(h.Sum32())
 }