Refactor golangci-lint, enable more linters and add github action

.github/workflows/golangci-lint.yml

@@ -0,0 +1,23 @@
+name: golangci-lint
+on:
+  pull_request:
+    types: [opened, edited, synchronize, reopened]
+    branches:
+      - main
+      - master
+jobs:
+  golangci:
+    name: lint
+    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        working-directory:
+          - ""
+          - tools/setup-envtest
+    steps:
+      - uses: actions/checkout@v2
+      - name: golangci-lint
+        uses: golangci/golangci-lint-action@v2
+        with:
+          version: v1.40.1
+          working-directory: ${{matrix.working-directory}}

.golangci.yml

@@ -1,36 +1,130 @@
-run:
-  deadline: 5m
-linters-settings:
-  lll:
-    line-length: 170
-  dupl:
-    threshold: 400
-issues:
-  # don't skip warning about doc comments
-  exclude-use-default: false
-
-  # restore some of the defaults
-  # (fill in the rest as needed)
-  exclude-rules:
-  - linters: [errcheck]
-    text: "Error return value of .((os\\.)?std(out|err)\\..*|.*Close|.*Flush|os\\.Remove(All)?|.*printf?|os\\.(Un)?Setenv). is not checked"
 linters:
   disable-all: true
   enable:
-  - misspell
-  - structcheck
-  - golint
-  - govet
+  - asciicheck
+  - bodyclose
   - deadcode
+  - depguard
+  - dogsled
   - errcheck
-  - varcheck
-  - unparam
-  - ineffassign
-  - nakedret
+  - exportloopref
+  - goconst
+  - gocritic
   - gocyclo
-  - dupl
+  - godot
+  - gofmt
   - goimports
-  - golint
-  # disabled:
-  # - goconst is overly aggressive
-  # - lll generally just complains about flag help & error strings that are human-readable
+  - goprintffuncname
+  - gosec
+  - gosimple
+  - govet
+  - ifshort
+  - importas
+  - ineffassign
+  - misspell
+  - nakedret
+  - nilerr
+  - nolintlint
+  - prealloc
+  - revive
+  - rowserrcheck
+  - staticcheck
+  - structcheck
+  - stylecheck
+  - typecheck
+  - unconvert
+  - unparam
+  - varcheck
+  - whitespace
+
+linters-settings:
+  ifshort:
+    # Maximum length of variable declaration measured in number of characters, after which linter won't suggest using short syntax.
+    max-decl-chars: 50
+  importas:
+    no-unaliased: true
+    alias:
+      # Kubernetes
+      - pkg: k8s.io/api/core/v1
+        alias: corev1
+      - pkg: k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1
+        alias: apiextensionsv1
+      - pkg: k8s.io/apimachinery/pkg/apis/meta/v1
+        alias: metav1
+      - pkg: k8s.io/apimachinery/pkg/api/errors
+        alias: apierrors
+      - pkg: k8s.io/apimachinery/pkg/util/errors
+        alias: kerrors
+      # Controller Runtime
+      - pkg: sigs.k8s.io/controller-runtime
+        alias: ctrl
+  staticcheck:
+    go: "1.16"
+  stylecheck:
+    go: "1.16"
+
+issues:
+  max-same-issues: 0
+  max-issues-per-linter: 0
+  # We are disabling default golangci exclusions because we want to help reviewers to focus on reviewing the most relevant
+  # changes in PRs and avoid nitpicking.
+  exclude-use-default: false
+  # List of regexps of issue texts to exclude, empty list by default.
+  exclude:
+  # The following are being worked on to remove their exclusion. This list should be reduced or go away all together over time.
+  # If it is decided they will not be addressed they should be moved above this comment.
+  - Subprocess launch(ed with variable|ing should be audited)
+  - (G204|G104|G307)
+  - "ST1000: at least one file in a package should have a package comment"
+  exclude-rules:
+  - linters:
+    - gosec
+    text: "G108: Profiling endpoint is automatically exposed on /debug/pprof"
+  - linters:
+    - revive
+    text: "exported: exported method .*\\.(Reconcile|SetupWithManager|SetupWebhookWithManager) should have comment or be unexported"
+  - linters:
+    - errcheck
+    text: Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
+  # With Go 1.16, the new embed directive can be used with an un-named import,
+  # revive (previously, golint) only allows these to be imported in a main.go, which wouldn't work for us.
+  # This directive allows the embed package to be imported with an underscore everywhere.
+  - linters:
+    - revive
+    source: _ "embed"
+  # Exclude some packages or code to require comments, for example test code, or fake clients.
+  - linters:
+    - revive
+    text: exported (method|function|type|const) (.+) should have comment or be unexported
+    source: (func|type).*Fake.*
+  - linters:
+    - revive
+    text: exported (method|function|type|const) (.+) should have comment or be unexported
+    path: fake_\.go
+  # Disable unparam "always receives" which might not be really
+  # useful when building libraries.
+  - linters:
+    - unparam
+    text: always receives
+  # Dot imports for gomega or ginkgo are allowed
+  # within test files.
+  - path: _test\.go
+    text: should not use dot imports
+  - path: _test\.go
+    text: cyclomatic complexity
+  - path: _test\.go
+    text: "G107: Potential HTTP request made with variable url"
+  # Append should be able to assign to a different var/slice.
+  - linters:
+    - gocritic
+    text: "appendAssign: append result not assigned to the same slice"
+  - linters:
+    - gocritic
+    text: "singleCaseSwitch: should rewrite switch statement to if statement"
+
+run:
+  timeout: 10m
+  skip-files:
+  - "zz_generated.*\\.go$"
+  - ".*conversion.*\\.go$"
+  allow-parallel-runners: true

Makefile

@@ -65,29 +65,29 @@ test-tools: ## tests the tools codebase (setup-envtest)
 ## Binaries
 ## --------------------------------------
 
-$(GOLANGCI_LINT): $(TOOLS_DIR)/go.mod # Build golangci-lint from tools folder.
-	cd $(TOOLS_DIR) && go build -tags=tools -o bin/golangci-lint github.com/golangci/golangci-lint/cmd/golangci-lint
-
 $(GO_APIDIFF): $(TOOLS_DIR)/go.mod # Build go-apidiff from tools folder.
 	cd $(TOOLS_DIR) && go build -tags=tools -o bin/go-apidiff github.com/joelanford/go-apidiff
 
 $(CONTROLLER_GEN): $(TOOLS_DIR)/go.mod # Build controller-gen from tools folder.
 	cd $(TOOLS_DIR) && go build -tags=tools -o bin/controller-gen sigs.k8s.io/controller-tools/cmd/controller-gen
 
+$(GOLANGCI_LINT): .github/workflows/golangci-lint.yml # Download golanci-lint using hack script into tools folder.
+	hack/ensure-golangci-lint.sh \
+		-b $(TOOLS_BIN_DIR) \
+		$(shell cat .github/workflows/golangci-lint.yml | grep version | sed 's/.*version: //')
+
 ## --------------------------------------
 ## Linting
 ## --------------------------------------
 
-.PHONY: lint-libs
-lint-libs: $(GOLANGCI_LINT) ## Lint library codebase.
-	$(GOLANGCI_LINT) run -v
-
-.PHONY: lint-tools
-lint-tools: $(GOLANGCI_LINT) ## Lint tools codebase.
-	cd tools/setup-envtest && $(GOLANGCI_LINT) run -v
-
 .PHONY: lint
-lint: lint-libs lint-tools
+lint: $(GOLANGCI_LINT) ## Lint codebase
+	$(GOLANGCI_LINT) run -v $(GOLANGCI_LINT_EXTRA_ARGS)
+	cd tools/setup-envtest; $(GOLANGCI_LINT) run -v $(GOLANGCI_LINT_EXTRA_ARGS)
+
+.PHONY: lint-fix
+lint-fix: $(GOLANGCI_LINT) ## Lint the codebase and run auto-fixers if supported by the linter.
+	GOLANGCI_LINT_EXTRA_ARGS=--fix $(MAKE) lint
 
 ## --------------------------------------
 ## Generate

alias.go

@@ -45,7 +45,7 @@ type Result = reconcile.Result
 // A Manager is required to create Controllers.
 type Manager = manager.Manager
 
-// Options are the arguments for creating a new Manager
+// Options are the arguments for creating a new Manager.
 type Options = manager.Options
 
 // SchemeBuilder builds a new Scheme for mapping go types to Kubernetes GroupVersionKinds.
@@ -55,7 +55,7 @@ type SchemeBuilder = scheme.Builder
 type GroupVersion = schema.GroupVersion
 
 // GroupResource specifies a Group and a Resource, but does not force a version.  This is useful for identifying
-// concepts during lookup stages without having partially valid types
+// concepts during lookup stages without having partially valid types.
 type GroupResource = schema.GroupResource
 
 // TypeMeta describes an individual object in an API response or request
@@ -89,18 +89,18 @@ var (
 	//
 	// * In-cluster config if running in cluster
 	//
-	// * $HOME/.kube/config if exists
+	// * $HOME/.kube/config if exists.
 	GetConfig = config.GetConfig
 
 	// ConfigFile returns the cfg.File function for deferred config file loading,
 	// this is passed into Options{}.From() to populate the Options fields for
 	// the manager.
 	ConfigFile = cfg.File
 
-	// NewControllerManagedBy returns a new controller builder that will be started by the provided Manager
+	// NewControllerManagedBy returns a new controller builder that will be started by the provided Manager.
 	NewControllerManagedBy = builder.ControllerManagedBy
 
-	// NewWebhookManagedBy returns a new webhook builder that will be started by the provided Manager
+	// NewWebhookManagedBy returns a new webhook builder that will be started by the provided Manager.
 	NewWebhookManagedBy = builder.WebhookManagedBy
 
 	// NewManager returns a new Manager for creating Controllers.

doc.go

@@ -28,7 +28,7 @@ limitations under the License.
 // The main entrypoint for controller-runtime is this root package, which
 // contains all of the common types needed to get started building controllers:
 //  import (
-//      controllers "sigs.k8s.io/controller-runtime"
+//      ctrl "sigs.k8s.io/controller-runtime"
 //  )
 //
 // The examples in this package walk through a basic controller setup.  The

example_test.go

@@ -24,7 +24,7 @@ import (
 
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	controllers "sigs.k8s.io/controller-runtime"
+	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -34,17 +34,17 @@ import (
 // ReplicaSetReconciler.
 //
 // * Start the application.
-// TODO(pwittrock): Update this example when we have better dependency injection support
+// TODO(pwittrock): Update this example when we have better dependency injection support.
 func Example() {
-	var log = controllers.Log.WithName("builder-examples")
+	var log = ctrl.Log.WithName("builder-examples")
 
-	manager, err := controllers.NewManager(controllers.GetConfigOrDie(), controllers.Options{})
+	manager, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{})
 	if err != nil {
 		log.Error(err, "could not create manager")
 		os.Exit(1)
 	}
 
-	err = controllers.
+	err = ctrl.
 		NewControllerManagedBy(manager). // Create the Controller
 		For(&appsv1.ReplicaSet{}).       // ReplicaSet is the Application API
 		Owns(&corev1.Pod{}).             // ReplicaSet owns Pods created by it
@@ -54,7 +54,7 @@ func Example() {
 		os.Exit(1)
 	}
 
-	if err := manager.Start(controllers.SetupSignalHandler()); err != nil {
+	if err := manager.Start(ctrl.SetupSignalHandler()); err != nil {
 		log.Error(err, "could not start manager")
 		os.Exit(1)
 	}
@@ -72,15 +72,15 @@ func Example() {
 // ReplicaSetReconciler.
 //
 // * Start the application.
-// TODO(pwittrock): Update this example when we have better dependency injection support
+// TODO(pwittrock): Update this example when we have better dependency injection support.
 func Example_updateLeaderElectionDurations() {
-	var log = controllers.Log.WithName("builder-examples")
+	var log = ctrl.Log.WithName("builder-examples")
 	leaseDuration := 100 * time.Second
 	renewDeadline := 80 * time.Second
 	retryPeriod := 20 * time.Second
-	manager, err := controllers.NewManager(
-		controllers.GetConfigOrDie(),
-		controllers.Options{
+	manager, err := ctrl.NewManager(
+		ctrl.GetConfigOrDie(),
+		ctrl.Options{
 			LeaseDuration: &leaseDuration,
 			RenewDeadline: &renewDeadline,
 			RetryPeriod:   &retryPeriod,
@@ -90,7 +90,7 @@ func Example_updateLeaderElectionDurations() {
 		os.Exit(1)
 	}
 
-	err = controllers.
+	err = ctrl.
 		NewControllerManagedBy(manager). // Create the Controller
 		For(&appsv1.ReplicaSet{}).       // ReplicaSet is the Application API
 		Owns(&corev1.Pod{}).             // ReplicaSet owns Pods created by it
@@ -100,7 +100,7 @@ func Example_updateLeaderElectionDurations() {
 		os.Exit(1)
 	}
 
-	if err := manager.Start(controllers.SetupSignalHandler()); err != nil {
+	if err := manager.Start(ctrl.SetupSignalHandler()); err != nil {
 		log.Error(err, "could not start manager")
 		os.Exit(1)
 	}
@@ -117,28 +117,28 @@ type ReplicaSetReconciler struct {
 //
 // * Read the ReplicaSet
 // * Read the Pods
-// * Set a Label on the ReplicaSet with the Pod count
-func (a *ReplicaSetReconciler) Reconcile(ctx context.Context, req controllers.Request) (controllers.Result, error) {
+// * Set a Label on the ReplicaSet with the Pod count.
+func (a *ReplicaSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
 	// Read the ReplicaSet
 	rs := &appsv1.ReplicaSet{}
 	err := a.Get(ctx, req.NamespacedName, rs)
 	if err != nil {
-		return controllers.Result{}, err
+		return ctrl.Result{}, err
 	}
 
 	// List the Pods matching the PodTemplate Labels
 	pods := &corev1.PodList{}
 	err = a.List(ctx, pods, client.InNamespace(req.Namespace), client.MatchingLabels(rs.Spec.Template.Labels))
 	if err != nil {
-		return controllers.Result{}, err
+		return ctrl.Result{}, err
 	}
 
 	// Update the ReplicaSet
 	rs.Labels["pod-count"] = fmt.Sprintf("%v", len(pods.Items))
 	err = a.Update(context.TODO(), rs)
 	if err != nil {
-		return controllers.Result{}, err
+		return ctrl.Result{}, err
 	}
 
-	return controllers.Result{}, nil
+	return ctrl.Result{}, nil
 }