⚠️ Refactor golangci-lint config to remove false negatives

During some audit of golangci-lint, it was found that one of the exclude directives was matching a lot more than it should have. The removal of the unwanted regex caused a large backlog of linting errors to show up. This changeset brings in a refactor in the golangci-lint configuration to make sure we catch more errors in the linting process and enable more linters over time. Signed-off-by: Vince Prignano <[email protected]>
kubernetes-sigs · May 24, 2021 · beed942 · beed942
1 parent 2b60ccd
commit beed942
Show file tree

Hide file tree

Showing 137 changed files with 499 additions and 438 deletions.
diff --git a/.golangci.yml b/.golangci.yml
@@ -23,11 +23,10 @@ linters:
   - nolintlint
   - prealloc
   - rowserrcheck
-  - scopelint
+  - exportloopref
   - staticcheck
   - structcheck
   - stylecheck
-  - testpackage
   - typecheck
   - unconvert
   - unparam
@@ -43,20 +42,44 @@ issues:
   exclude-use-default: false
   # List of regexps of issue texts to exclude, empty list by default.
   exclude:
-    - Using the variable on range scope `(tc)|(rt)|(tt)|(test)|(testcase)|(testCase)` in function literal
-    - "G108: Profiling endpoint is automatically exposed on /debug/pprof"
-    - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
-    # 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)
-    - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
-    - (G104|G307)
+  - "G108: Profiling endpoint is automatically exposed on /debug/pprof"
+  - Error return value of .((os\.)?std(out|err)\..*|.*Close|.*Flush|os\.Remove(All)?|.*print(f|ln)?|os\.(Un)?Setenv). is not checked
+  - exported method `.*.Reconcile` should have comment or be unexported
+  - exported method `.*.SetupWithManager` should have comment or be unexported
+  # 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)
+  - (Expect directory permissions to be 0750 or less|Expect file permissions to be 0600 or less)
+  - (G104|G307)
+  - exported (method|function|type|const) (.+) should have comment or be unexported
+  exclude-rules:
+  # With Go 1.16, the new embed directive can be used with an un-named import,
+  # 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:
+    - golint
+    source: _ "embed"
+  # 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/framework.*.go
+    text: should not use dot imports
+  - path: test/e2e.*.go
+    text: should not use dot imports
 
 run:
   timeout: 10m
   skip-files:
-    - "zz_generated.*\\.go$"
-    - ".*conversion.*\\.go$"
+  - "zz_generated.*\\.go$"
+  - ".*conversion.*\\.go$"
   skip-dirs:
-    - third_party
+  - third_party
   allow-parallel-runners: true
diff --git a/Makefile b/Makefile
@@ -215,16 +215,7 @@ e2e-framework: ## Builds the CAPI e2e framework
 
 .PHONY: lint
 lint: $(GOLANGCI_LINT) ## Lint codebase
-	$(MAKE) -j8 lint-all
-
-.PHONY: lint-all lint-core lint-e2e lint-capd
-lint-all: lint-core lint-e2e lint-capd
-lint-core:
 	$(GOLANGCI_LINT) run -v $(GOLANGCI_LINT_EXTRA_ARGS)
-lint-e2e:
-	cd $(E2E_FRAMEWORK_DIR); $(GOLANGCI_LINT) run -v $(GOLANGCI_LINT_EXTRA_ARGS)
-lint-capd:
-	cd $(CAPD_DIR); $(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.

diff --git a/api/v1alpha3/cluster_types.go b/api/v1alpha3/cluster_types.go
@@ -29,6 +29,8 @@ import (
 )
 
 const (
+	// ClusterFinalizer is the finalizer used by the cluster controller to
+	// cleanup the cluster resources when a Cluster is being deleted.
 	ClusterFinalizer = "cluster.cluster.x-k8s.io"
 )
 
@@ -87,6 +89,7 @@ type ClusterNetwork struct {
 // ANCHOR_END: ClusterNetwork
 
 // ANCHOR: NetworkRanges
+
 // NetworkRanges represents ranges of network addresses.
 type NetworkRanges struct {
 	CIDRBlocks []string `json:"cidrBlocks"`

diff --git a/api/v1alpha3/common_types.go b/api/v1alpha3/common_types.go
@@ -53,13 +53,16 @@ const (
 // MachineAddressType describes a valid MachineAddress type.
 type MachineAddressType string
 
+// Define all the constants related to MachineAddressType.
 const (
 	MachineHostName    MachineAddressType = "Hostname"
 	MachineExternalIP  MachineAddressType = "ExternalIP"
 	MachineInternalIP  MachineAddressType = "InternalIP"
 	MachineExternalDNS MachineAddressType = "ExternalDNS"
 	MachineInternalDNS MachineAddressType = "InternalDNS"
+)
 
+const (
 	// MachineNodeNameIndex is used by the Machine Controller to index Machines by Node name, and add a watch on Nodes.
 	MachineNodeNameIndex = "status.nodeRef.name"
 )

diff --git a/api/v1alpha3/condition_consts.go b/api/v1alpha3/condition_consts.go
@@ -55,7 +55,7 @@ const (
 // Conditions and condition Reasons for the Cluster object
 
 const (
-	// ControlPlaneReady reports the ready condition from the control plane object defined for this cluster.
+	// ControlPlaneReadyCondition reports the ready condition from the control plane object defined for this cluster.
 	// This condition is mirrored from the Ready condition in the control plane ref object, and
 	// the absence of this condition might signal problems in the reconcile external loops or the fact that
 	// the control plane provider does not not implements the Ready condition yet.
@@ -173,7 +173,7 @@ const (
 	// allowed to remediate any Machines or whether it is blocked from remediating any further.
 	RemediationAllowedCondition ConditionType = "RemediationAllowed"
 
-	// TooManyUnhealthy is the reason used when too many Machines are unhealthy and the MachineHealthCheck is blocked
+	// TooManyUnhealthyReason is the reason used when too many Machines are unhealthy and the MachineHealthCheck is blocked
 	// from making any further remediations.
 	TooManyUnhealthyReason = "TooManyUnhealthy"
 )
diff --git a/api/v1alpha3/machinedeployment_types.go b/api/v1alpha3/machinedeployment_types.go
@@ -24,7 +24,7 @@ import (
 type MachineDeploymentStrategyType string
 
 const (
-	// Replace the old MachineSet by new one using rolling update
+	// RollingUpdateMachineDeploymentStrategyType replaces the old MachineSet by new one using rolling update
 	// i.e. gradually scale down the old MachineSet and scale up the new one.
 	RollingUpdateMachineDeploymentStrategyType MachineDeploymentStrategyType = "RollingUpdate"
 

diff --git a/api/v1alpha4/cluster_types.go b/api/v1alpha4/cluster_types.go
@@ -30,6 +30,8 @@ import (
 )
 
 const (
+	// ClusterFinalizer is the finalizer used by the cluster controller to
+	// cleanup the cluster resources when a Cluster is being deleted.
 	ClusterFinalizer = "cluster.cluster.x-k8s.io"
 )
 
@@ -88,6 +90,7 @@ type ClusterNetwork struct {
 // ANCHOR_END: ClusterNetwork
 
 // ANCHOR: NetworkRanges
+
 // NetworkRanges represents ranges of network addresses.
 type NetworkRanges struct {
 	CIDRBlocks []string `json:"cidrBlocks"`
@@ -286,6 +289,7 @@ func ipFamilyForCIDRStrings(cidrs []string) (ClusterIPFamily, error) {
 
 type ClusterIPFamily int
 
+// Define the ClusterIPFamily constants.
 const (
 	InvalidIPFamily ClusterIPFamily = iota
 	IPv4IPFamily

diff --git a/api/v1alpha4/common_types.go b/api/v1alpha4/common_types.go
@@ -102,13 +102,16 @@ var (
 // MachineAddressType describes a valid MachineAddress type.
 type MachineAddressType string
 
+// Define the MachineAddressType constants.
 const (
 	MachineHostName    MachineAddressType = "Hostname"
 	MachineExternalIP  MachineAddressType = "ExternalIP"
 	MachineInternalIP  MachineAddressType = "InternalIP"
 	MachineExternalDNS MachineAddressType = "ExternalDNS"
 	MachineInternalDNS MachineAddressType = "InternalDNS"
+)
 
+const (
 	// MachineNodeNameIndex is used by the Machine Controller to index Machines by Node name, and add a watch on Nodes.
 	MachineNodeNameIndex = "status.nodeRef.name"
 )

diff --git a/api/v1alpha4/condition_consts.go b/api/v1alpha4/condition_consts.go
@@ -72,7 +72,7 @@ const (
 	// provider to report successful control plane initialization.
 	WaitingForControlPlaneProviderInitializedReason = "WaitingForControlPlaneProviderInitialized"
 
-	// ControlPlaneReady reports the ready condition from the control plane object defined for this cluster.
+	// ControlPlaneReadyCondition reports the ready condition from the control plane object defined for this cluster.
 	// This condition is mirrored from the Ready condition in the control plane ref object, and
 	// the absence of this condition might signal problems in the reconcile external loops or the fact that
 	// the control plane provider does not not implements the Ready condition yet.
@@ -197,7 +197,7 @@ const (
 	// allowed to remediate any Machines or whether it is blocked from remediating any further.
 	RemediationAllowedCondition ConditionType = "RemediationAllowed"
 
-	// TooManyUnhealthy is the reason used when too many Machines are unhealthy and the MachineHealthCheck is blocked
+	// TooManyUnhealthyReason is the reason used when too many Machines are unhealthy and the MachineHealthCheck is blocked
 	// from making any further remediations.
 	TooManyUnhealthyReason = "TooManyUnhealthy"
 )
diff --git a/api/v1alpha4/machine_webhook.go b/api/v1alpha4/machine_webhook.go
@@ -18,9 +18,10 @@ package v1alpha4
 
 import (
 	"fmt"
-	"sigs.k8s.io/cluster-api/util/version"
 	"strings"
 
+	"sigs.k8s.io/cluster-api/util/version"
+
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/validation/field"

diff --git a/api/v1alpha4/machinedeployment_types.go b/api/v1alpha4/machinedeployment_types.go
@@ -24,7 +24,7 @@ import (
 type MachineDeploymentStrategyType string
 
 const (
-	// Replace the old MachineSet by new one using rolling update
+	// RollingUpdateMachineDeploymentStrategyType replaces the old MachineSet by new one using rolling update
 	// i.e. gradually scale down the old MachineSet and scale up the new one.
 	RollingUpdateMachineDeploymentStrategyType MachineDeploymentStrategyType = "RollingUpdate"
 
@@ -33,12 +33,15 @@ const (
 
 	// RevisionAnnotation is the revision annotation of a machine deployment's machine sets which records its rollout sequence.
 	RevisionAnnotation = "machinedeployment.clusters.x-k8s.io/revision"
+
 	// RevisionHistoryAnnotation maintains the history of all old revisions that a machine set has served for a machine deployment.
 	RevisionHistoryAnnotation = "machinedeployment.clusters.x-k8s.io/revision-history"
+
 	// DesiredReplicasAnnotation is the desired replicas for a machine deployment recorded as an annotation
 	// in its machine sets. Helps in separating scaling events from the rollout process and for
 	// determining if the new machine set for a deployment is really saturated.
 	DesiredReplicasAnnotation = "machinedeployment.clusters.x-k8s.io/desired-replicas"
+
 	// MaxReplicasAnnotation is the maximum replicas a deployment can have at a given point, which
 	// is machinedeployment.spec.replicas + maxSurge. Used by the underlying machine sets to estimate their
 	// proportions in case the deployment has surge replicas.

diff --git a/api/v1alpha4/machinedeployment_webhook.go b/api/v1alpha4/machinedeployment_webhook.go
@@ -24,7 +24,6 @@ import (
 	"k8s.io/apimachinery/pkg/labels"
 	runtime "k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	intstrutil "k8s.io/apimachinery/pkg/util/intstr"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -100,7 +99,7 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error {
 		}
 
 		if m.Spec.Strategy.RollingUpdate.MaxSurge != nil {
-			if _, err := intstrutil.GetScaledValueFromIntOrPercent(m.Spec.Strategy.RollingUpdate.MaxSurge, total, true); err != nil {
+			if _, err := intstr.GetScaledValueFromIntOrPercent(m.Spec.Strategy.RollingUpdate.MaxSurge, total, true); err != nil {
 				allErrs = append(
 					allErrs,
 					field.Invalid(field.NewPath("spec", "strategy", "rollingUpdate", "maxSurge"),
@@ -110,7 +109,7 @@ func (m *MachineDeployment) validate(old *MachineDeployment) error {
 		}
 
 		if m.Spec.Strategy.RollingUpdate.MaxUnavailable != nil {
-			if _, err := intstrutil.GetScaledValueFromIntOrPercent(m.Spec.Strategy.RollingUpdate.MaxUnavailable, total, true); err != nil {
+			if _, err := intstr.GetScaledValueFromIntOrPercent(m.Spec.Strategy.RollingUpdate.MaxUnavailable, total, true); err != nil {
 				allErrs = append(
 					allErrs,
 					field.Invalid(field.NewPath("spec", "strategy", "rollingUpdate", "maxUnavailable"),

diff --git a/api/v1alpha4/machineset_webhook.go b/api/v1alpha4/machineset_webhook.go
@@ -40,7 +40,7 @@ func (m *MachineSet) SetupWebhookWithManager(mgr ctrl.Manager) error {
 var _ webhook.Defaulter = &MachineSet{}
 var _ webhook.Validator = &MachineSet{}
 
-// DefaultingFunction sets default MachineSet field values.
+// Default sets default MachineSet field values.
 func (m *MachineSet) Default() {
 	if m.Labels == nil {
 		m.Labels = make(map[string]string)

diff --git a/bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_types.go b/bootstrap/kubeadm/api/v1alpha3/kubeadmconfig_types.go
@@ -211,7 +211,7 @@ type FileSource struct {
 	Secret SecretFileSource `json:"secret"`
 }
 
-// Adapts a Secret into a FileSource.
+// SecretFileSource adapts a Secret into a FileSource.
 //
 // The contents of the target Secret's Data field will be presented
 // as files using the keys in the Data field as the file names.

diff --git a/bootstrap/kubeadm/api/v1alpha4/kubeadm_types.go b/bootstrap/kubeadm/api/v1alpha4/kubeadm_types.go
@@ -412,11 +412,12 @@ type HostPathMount struct {
 	PathType corev1.HostPathType `json:"pathType,omitempty"`
 }
 
-// +kubebuilder:validation:Type=string
 // BootstrapTokenString is a token of the format abcdef.abcdef0123456789 that is used
 // for both validation of the practically of the API server from a joining node's point
 // of view and as an authentication method for the node in the bootstrap phase of
 // "kubeadm join". This token is and should be short-lived.
+//
+// +kubebuilder:validation:Type=string
 type BootstrapTokenString struct {
 	ID     string `json:"-"`
 	Secret string `json:"-"`

diff --git a/bootstrap/kubeadm/api/v1alpha4/kubeadmconfig_types.go b/bootstrap/kubeadm/api/v1alpha4/kubeadmconfig_types.go
@@ -204,7 +204,7 @@ type FileSource struct {
 	Secret SecretFileSource `json:"secret"`
 }
 
-// Adapts a Secret into a FileSource.
+// SecretFileSource adapts a Secret into a FileSource.
 //
 // The contents of the target Secret's Data field will be presented
 // as files using the keys in the Data field as the file names.

diff --git a/bootstrap/kubeadm/api/v1alpha4/kubeadmconfig_webhook.go b/bootstrap/kubeadm/api/v1alpha4/kubeadmconfig_webhook.go
@@ -27,10 +27,10 @@ import (
 )
 
 var (
-	ConflictingFileSourceMsg = "only one of content or contentFrom may be specified for a single file"
-	MissingSecretNameMsg     = "secret file source must specify non-empty secret name"
-	MissingSecretKeyMsg      = "secret file source must specify non-empty secret key"
-	PathConflictMsg          = "path property must be unique among all files"
+	conflictingFileSourceMsg = "only one of content or contentFrom may be specified for a single file"
+	missingSecretNameMsg     = "secret file source must specify non-empty secret name"
+	missingSecretKeyMsg      = "secret file source must specify non-empty secret key"
+	pathConflictMsg          = "path property must be unique among all files"
 )
 
 func (c *KubeadmConfig) SetupWebhookWithManager(mgr ctrl.Manager) error {
@@ -71,7 +71,7 @@ func (c *KubeadmConfigSpec) validate(name string) error {
 				field.Invalid(
 					field.NewPath("spec", "files", fmt.Sprintf("%d", i)),
 					file,
-					ConflictingFileSourceMsg,
+					conflictingFileSourceMsg,
 				),
 			)
 		}
@@ -85,7 +85,7 @@ func (c *KubeadmConfigSpec) validate(name string) error {
 					field.Invalid(
 						field.NewPath("spec", "files", fmt.Sprintf("%d", i), "contentFrom", "secret", "name"),
 						file,
-						MissingSecretNameMsg,
+						missingSecretNameMsg,
 					),
 				)
 			}
@@ -95,7 +95,7 @@ func (c *KubeadmConfigSpec) validate(name string) error {
 					field.Invalid(
 						field.NewPath("spec", "files", fmt.Sprintf("%d", i), "contentFrom", "secret", "key"),
 						file,
-						MissingSecretKeyMsg,
+						missingSecretKeyMsg,
 					),
 				)
 			}
@@ -107,7 +107,7 @@ func (c *KubeadmConfigSpec) validate(name string) error {
 				field.Invalid(
 					field.NewPath("spec", "files", fmt.Sprintf("%d", i), "path"),
 					file,
-					PathConflictMsg,
+					pathConflictMsg,
 				),
 			)
 		}

diff --git a/bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go b/bootstrap/kubeadm/controllers/kubeadmconfig_controller_test.go
@@ -190,7 +190,7 @@ func TestKubeadmConfigReconciler_ReturnEarlyIfClusterInfraNotReady(t *testing.T)
 	machine := newMachine(cluster, "machine")
 	config := newKubeadmConfig(machine, "cfg")
 
-	//cluster infra not ready
+	// cluster infra not ready
 	cluster.Status = clusterv1.ClusterStatus{
 		InfrastructureReady: false,
 	}