Merge pull request #2315 from MrDXY/pr-golangci-enable-gocritic

k8s-ci-robot · web-flow · commit 69a97b93733f · 2023-09-06T05:40:51.000-07:00
🌱 lint: enable `gocritic` configuration and fix findings
diff --git a/.golangci.yml b/.golangci.yml
@@ -68,27 +68,27 @@ linters-settings:
     exclude:
       - '^ \+.*'
       - '^ ANCHOR.*'
-  # gocritic:
-  #   enabled-tags:
-  #     - diagnostic
-  #     - experimental
-  #     - performance
-  #   disabled-checks:
-  #     - appendAssign
-  #     - dupImport # https://github.com/go-critic/go-critic/issues/845
-  #     - evalOrder
-  #     - ifElseChain
-  #     - octalLiteral
-  #     - regexpSimplify
-  #     - sloppyReassign
-  #     - truncateCmp
-  #     - typeDefFirst
-  #     - unnamedResult
-  #     - unnecessaryDefer
-  #     - whyNoLint
-  #     - wrapperFunc
-  #     - rangeValCopy
-  #     - hugeParam
+  gocritic:
+    enabled-tags:
+      - diagnostic
+      - experimental
+      - performance
+    disabled-checks:
+      - appendAssign
+      - dupImport # https://github.com/go-critic/go-critic/issues/845
+      - evalOrder
+      - ifElseChain
+      - octalLiteral
+      - regexpSimplify
+      - sloppyReassign
+      - truncateCmp
+      - typeDefFirst
+      - unnamedResult
+      - unnecessaryDefer
+      - whyNoLint
+      - wrapperFunc
+      - rangeValCopy
+      - hugeParam
   importas:
     no-unaliased: true
     alias:
diff --git a/apis/v1alpha3/cloudprovider_encoding_test.go b/apis/v1alpha3/cloudprovider_encoding_test.go
@@ -610,7 +610,6 @@ func TestUnmarshalINI(t *testing.T) {
 		},
 	}
 
-	//nolint:gocritic
 	testCases := append(
 		testcases,
 		deprecatedTestCases...,
diff --git a/apis/v1alpha3/cloudprovider_types.go b/apis/v1alpha3/cloudprovider_types.go
@@ -23,7 +23,7 @@ limitations under the License.
 //
 // The "gopkg.in/go-ini/ini.v1" package was investigated, but it does not
 // support reflecting a struct with a field of type "map[string]TYPE" to INI.
-//nolint:gocritic,godot
+
 package v1alpha3
 
 // CPIConfig is the vSphere cloud provider's configuration.
diff --git a/apis/v1alpha3/haproxyloadbalancer_types.go b/apis/v1alpha3/haproxyloadbalancer_types.go
@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-//nolint:gocritic,godot
 package v1alpha3
 
 import (
diff --git a/apis/v1alpha3/vspherecluster_types.go b/apis/v1alpha3/vspherecluster_types.go
@@ -14,7 +14,6 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-//nolint:gocritic,godot
 package v1alpha3
 
 import (
diff --git a/apis/v1alpha4/vspheremachinetemplate_conversion.go b/apis/v1alpha4/vspheremachinetemplate_conversion.go
@@ -26,7 +26,7 @@ import (
 	infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"
 )
 
-// ConvertTo.
+// ConvertTo converts this VSphereMachineTemplate to the Hub version (v1beta1).
 func (src *VSphereMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {
 	dst := dstRaw.(*infrav1.VSphereMachineTemplate)
 	if err := Convert_v1alpha4_VSphereMachineTemplate_To_v1beta1_VSphereMachineTemplate(src, dst, nil); err != nil {
@@ -51,6 +51,7 @@ func (src *VSphereMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {
 	return nil
 }
 
+// ConvertFrom converts from the Hub version (v1beta1) to this VSphereMachineTemplate.
 func (dst *VSphereMachineTemplate) ConvertFrom(srcRaw conversion.Hub) error {
 	src := srcRaw.(*infrav1.VSphereMachineTemplate)
 	if err := Convert_v1beta1_VSphereMachineTemplate_To_v1alpha4_VSphereMachineTemplate(src, dst, nil); err != nil {
diff --git a/controllers/controllers_suite_test.go b/controllers/controllers_suite_test.go
@@ -51,9 +51,7 @@ var (
 
 func TestMain(m *testing.M) {
 	setup()
-	defer func() {
-		teardown()
-	}()
+	defer teardown()
 	code := m.Run()
 	os.Exit(code) //nolint:gocritic
 }
diff --git a/controllers/servicediscovery_controller.go b/controllers/servicediscovery_controller.go
@@ -429,7 +429,7 @@ func getSupervisorAPIServerURLWithFIP(client client.Client) (string, error) {
 // tryParseClusterInfoFromConfigMap tries to parse a kubeconfig file from a ConfigMap key.
 func tryParseClusterInfoFromConfigMap(cm *corev1.ConfigMap) (*clientcmdapi.Config, error) {
 	kubeConfigString, ok := cm.Data[bootstrapapi.KubeConfigKey]
-	if !ok || len(kubeConfigString) == 0 {
+	if !ok || kubeConfigString == "" {
 		return nil, errors.Errorf("no %s key in ConfigMap %s/%s", bootstrapapi.KubeConfigKey, cm.Namespace, cm.Name)
 	}
 	parsedKubeConfig, err := clientcmd.Load([]byte(kubeConfigString))
diff --git a/controllers/vspherecluster_reconciler.go b/controllers/vspherecluster_reconciler.go
@@ -153,18 +153,19 @@ func (r clusterReconciler) reconcileDelete(clusterCtx *capvcontext.ClusterContex
 		// If the VSphereMachine is not owned by the CAPI Machine object because the machine object was deleted
 		// before setting the owner references, then proceed with the deletion of the VSphereMachine object.
 		// This is required until CAPI has a solution for https://github.com/kubernetes-sigs/cluster-api/issues/5483
-		if clusterutilv1.IsOwnedByObject(vsphereMachine, clusterCtx.VSphereCluster) && len(vsphereMachine.OwnerReferences) == 1 {
-			machineDeletionCount++
-			// Remove the finalizer since VM creation wouldn't proceed
-			r.Logger.Info("Removing finalizer from VSphereMachine", "namespace", vsphereMachine.Namespace, "name", vsphereMachine.Name)
-			ctrlutil.RemoveFinalizer(vsphereMachine, infrav1.MachineFinalizer)
-			if err := r.Client.Update(clusterCtx, vsphereMachine); err != nil {
-				return reconcile.Result{}, err
-			}
-			if err := r.Client.Delete(clusterCtx, vsphereMachine); err != nil && !apierrors.IsNotFound(err) {
-				clusterCtx.Logger.Error(err, "Failed to delete for VSphereMachine", "namespace", vsphereMachine.Namespace, "name", vsphereMachine.Name)
-				deletionErrors = append(deletionErrors, err)
-			}
+		if !clusterutilv1.IsOwnedByObject(vsphereMachine, clusterCtx.VSphereCluster) || len(vsphereMachine.OwnerReferences) != 1 {
+			continue
+		}
+		machineDeletionCount++
+		// Remove the finalizer since VM creation wouldn't proceed
+		r.Logger.Info("Removing finalizer from VSphereMachine", "namespace", vsphereMachine.Namespace, "name", vsphereMachine.Name)
+		ctrlutil.RemoveFinalizer(vsphereMachine, infrav1.MachineFinalizer)
+		if err := r.Client.Update(clusterCtx, vsphereMachine); err != nil {
+			return reconcile.Result{}, err
+		}
+		if err := r.Client.Delete(clusterCtx, vsphereMachine); err != nil && !apierrors.IsNotFound(err) {
+			clusterCtx.Logger.Error(err, "Failed to delete for VSphereMachine", "namespace", vsphereMachine.Namespace, "name", vsphereMachine.Name)
+			deletionErrors = append(deletionErrors, err)
 		}
 	}
 	if len(deletionErrors) > 0 {
diff --git a/controllers/vspheredeploymentzone_controller.go b/controllers/vspheredeploymentzone_controller.go
@@ -243,20 +243,21 @@ func (r vsphereDeploymentZoneReconciler) getVCenterSession(deploymentZoneCtx *ca
 	}
 
 	for _, vsphereCluster := range clusterList.Items {
-		if deploymentZoneCtx.VSphereDeploymentZone.Spec.Server == vsphereCluster.Spec.Server && vsphereCluster.Spec.IdentityRef != nil {
-			logger := deploymentZoneCtx.Logger.WithValues("cluster", vsphereCluster.Name)
-			params = params.WithThumbprint(vsphereCluster.Spec.Thumbprint)
-			clust := vsphereCluster
-			creds, err := identity.GetCredentials(deploymentZoneCtx, r.Client, &clust, r.Namespace)
-			if err != nil {
-				logger.Error(err, "error retrieving credentials from IdentityRef")
-				continue
-			}
-			logger.Info("using server credentials to create the authenticated session")
-			params = params.WithUserInfo(creds.Username, creds.Password)
-			return session.GetOrCreate(r.Context,
-				params)
+		if deploymentZoneCtx.VSphereDeploymentZone.Spec.Server != vsphereCluster.Spec.Server || vsphereCluster.Spec.IdentityRef == nil {
+			continue
+		}
+		logger := deploymentZoneCtx.Logger.WithValues("cluster", vsphereCluster.Name)
+		params = params.WithThumbprint(vsphereCluster.Spec.Thumbprint)
+		clust := vsphereCluster
+		creds, err := identity.GetCredentials(deploymentZoneCtx, r.Client, &clust, r.Namespace)
+		if err != nil {
+			logger.Error(err, "error retrieving credentials from IdentityRef")
+			continue
 		}
+		logger.Info("using server credentials to create the authenticated session")
+		params = params.WithUserInfo(creds.Username, creds.Password)
+		return session.GetOrCreate(r.Context,
+			params)
 	}
 
 	// Fallback to using credentials provided to the manager
diff --git a/controllers/vspheredeploymentzone_controller_domain_test.go b/controllers/vspheredeploymentzone_controller_domain_test.go
@@ -32,13 +32,7 @@ import (
 	"sigs.k8s.io/cluster-api-provider-vsphere/test/helpers/vcsim"
 )
 
-//nolint:paralleltest
-func TestVsphereDeploymentZoneReconciler_Reconcile_VerifyFailureDomain(t *testing.T) {
-	t.Run("for Compute Cluster Zone Failure Domain", ForComputeClusterZone)
-	t.Run("for Host Group Zone Failure Domain", ForHostGroupZone)
-}
-
-func ForComputeClusterZone(t *testing.T) {
+func TestVsphereDeploymentZoneReconciler_Reconcile_VerifyFailureDomain_ComputeClusterZone(t *testing.T) {
 	g := NewWithT(t)
 
 	model := simulator.VPX()
@@ -120,7 +114,7 @@ func ForComputeClusterZone(t *testing.T) {
 	g.Expect(reconciler.verifyFailureDomain(deploymentZoneCtx, vsphereFailureDomain.Spec.Zone)).To(HaveOccurred())
 }
 
-func ForHostGroupZone(t *testing.T) {
+func TestVsphereDeploymentZoneReconciler_Reconcile_VerifyFailureDomain_HostGroupZone(t *testing.T) {
 	g := NewWithT(t)
 
 	model := simulator.VPX()
diff --git a/controllers/vspherevm_controller_test.go b/controllers/vspherevm_controller_test.go
@@ -710,31 +710,32 @@ func createMachineOwnerHierarchy(machine *clusterv1.Machine) []client.Object {
 		clusterName, _ = machine.Labels[clusterv1.ClusterNameLabel]
 	)
 
-	objs = append(objs, &clusterv1.MachineSet{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf("%s-ms", machine.Name),
-			Namespace: machine.Namespace,
-			Labels: map[string]string{
-				clusterv1.ClusterNameLabel: clusterName,
-			},
-			OwnerReferences: []metav1.OwnerReference{
-				{
-					APIVersion: clusterv1.GroupVersion.String(),
-					Kind:       "MachineDeployment",
-					Name:       fmt.Sprintf("%s-md", machine.Name),
+	objs = append(
+		objs,
+		&clusterv1.MachineSet{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("%s-ms", machine.Name),
+				Namespace: machine.Namespace,
+				Labels: map[string]string{
+					clusterv1.ClusterNameLabel: clusterName,
+				},
+				OwnerReferences: []metav1.OwnerReference{
+					{
+						APIVersion: clusterv1.GroupVersion.String(),
+						Kind:       "MachineDeployment",
+						Name:       fmt.Sprintf("%s-md", machine.Name),
+					},
 				},
 			},
 		},
-	})
-
-	objs = append(objs, &clusterv1.MachineDeployment{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      fmt.Sprintf("%s-md", machine.Name),
-			Namespace: machine.Namespace,
-			Labels: map[string]string{
-				clusterv1.ClusterNameLabel: clusterName,
+		&clusterv1.MachineDeployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("%s-md", machine.Name),
+				Namespace: machine.Namespace,
+				Labels: map[string]string{
+					clusterv1.ClusterNameLabel: clusterName,
+				},
 			},
-		},
-	})
+		})
 	return objs
 }
diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go
@@ -63,7 +63,7 @@ password: '%s'
 
 		// Update the file and wait for watch to detect the change
 		content := fmt.Sprintf(contentFmt, updatedUsername, updatedPassword)
-		_, err = tmpFile.Write([]byte(content))
+		_, err = tmpFile.WriteString(content)
 		g.Expect(err).ToNot(HaveOccurred())
 
 		g.Eventually(func() bool {
@@ -98,7 +98,7 @@ password: '%s'
 
 		// Update the file and wait for watch to detect the change
 		content := fmt.Sprintf(contentFmt, updatedUsername, updatedPassword)
-		if _, err := tmpFile.Write([]byte(content)); err != nil {
+		if _, err := tmpFile.WriteString(content); err != nil {
 			fmt.Printf("failed to update credentials in the file err:%s", err.Error())
 		}
 
diff --git a/pkg/manager/options_test.go b/pkg/manager/options_test.go
@@ -62,7 +62,7 @@ password: '%s'
 		}
 		t.Cleanup(func() { os.Remove(tmpFile.Name()) })
 
-		if _, err := tmpFile.Write([]byte(content)); err != nil {
+		if _, err := tmpFile.WriteString(content); err != nil {
 			t.Fatal(err)
 		}
 		if err := tmpFile.Close(); err != nil {
diff --git a/pkg/services/govmomi/util.go b/pkg/services/govmomi/util.go
@@ -366,7 +366,7 @@ func waitForMacAddresses(ctx *virtualMachineContext) error {
 func getMacAddresses(ctx *virtualMachineContext) ([]string, map[string]int, map[int]string, error) {
 	var (
 		vm                   mo.VirtualMachine
-		macAddresses         []string
+		macAddresses         = make([]string, 0)
 		macToDeviceSpecIndex = map[string]int{}
 		deviceSpecIndexToMac = map[int]string{}
 	)
@@ -375,13 +375,15 @@ func getMacAddresses(ctx *virtualMachineContext) ([]string, map[string]int, map[
 	}
 	i := 0
 	for _, device := range vm.Config.Hardware.Device {
-		if nic, ok := device.(types.BaseVirtualEthernetCard); ok {
-			mac := nic.GetVirtualEthernetCard().MacAddress
-			macAddresses = append(macAddresses, mac)
-			macToDeviceSpecIndex[mac] = i
-			deviceSpecIndexToMac[i] = mac
-			i++
+		nic, ok := device.(types.BaseVirtualEthernetCard)
+		if !ok {
+			continue
 		}
+		mac := nic.GetVirtualEthernetCard().MacAddress
+		macAddresses = append(macAddresses, mac)
+		macToDeviceSpecIndex[mac] = i
+		deviceSpecIndexToMac[i] = mac
+		i++
 	}
 	return macAddresses, macToDeviceSpecIndex, deviceSpecIndexToMac, nil
 }
diff --git a/pkg/services/govmomi/vcenter/clone_test.go b/pkg/services/govmomi/vcenter/clone_test.go
@@ -122,12 +122,7 @@ func TestGetDiskSpec(t *testing.T) {
 			}
 			vmContext := &context.VMContext{VSphereVM: vsphereVM}
 			devices, err := getDiskSpec(vmContext, tc.disks)
-			switch {
-			case tc.err != "" && err == nil:
-				fallthrough
-			case tc.err == "" && err != nil:
-				fallthrough
-			case err != nil && tc.err != err.Error():
+			if (tc.err != "" && err == nil) || (tc.err == "" && err != nil) || (err != nil && tc.err != err.Error()) {
 				t.Fatalf("Expected to get '%v' error from getDiskSpec, got: '%v'", tc.err, err)
 			}
 			if deviceFound := len(devices) != 0; tc.expectDevice != deviceFound {
diff --git a/pkg/util/label_test.go b/pkg/util/label_test.go
@@ -22,12 +22,7 @@ import (
 	"github.com/onsi/gomega"
 )
 
-func TestSanitizeHostInfoLabel(t *testing.T) {
-	t.Run("for IP addresses", testIPAddressLogic)
-	t.Run("for DNS entries", testDNSLogic)
-}
-
-func testIPAddressLogic(t *testing.T) {
+func TestSanitizeIPHostInfoLabel(t *testing.T) {
 	tests := []struct {
 		name, input, expected string
 	}{
@@ -61,7 +56,7 @@ func testIPAddressLogic(t *testing.T) {
 	}
 }
 
-func testDNSLogic(t *testing.T) {
+func TestSanitizeDNSHostInfoLabel(t *testing.T) {
 	tests := []struct {
 		name, input, expected string
 	}{
diff --git a/test/e2e/gpu_pci_passthrough_test.go b/test/e2e/gpu_pci_passthrough_test.go
@@ -75,18 +75,19 @@ var _ = Describe("Cluster creation with GPU devices as PCI passthrough [speciali
 func verifyPCIDeviceOnWorkerNodes(clusterName, namespace string) {
 	list := getVSphereVMsForCluster(clusterName, namespace)
 	for _, vm := range list.Items {
-		if _, ok := vm.GetLabels()[clusterv1.MachineControlPlaneLabel]; !ok {
-			finder := find.NewFinder(vsphereClient.Client, false)
-			dc, err := finder.Datacenter(ctx, vm.Spec.Datacenter)
-			Expect(err).NotTo(HaveOccurred())
-			finder.SetDatacenter(dc)
-
-			vmObj, err := finder.VirtualMachine(ctx, fmt.Sprintf("/%s/vm/%s/%s", vm.Spec.Datacenter, vm.Spec.Folder, vm.Name))
-			Expect(err).NotTo(HaveOccurred())
-			devices, err := vmObj.Device(ctx)
-			Expect(err).NotTo(HaveOccurred())
-			defaultPciDevices := devices.SelectByType((*types.VirtualPCIPassthrough)(nil))
-			Expect(defaultPciDevices).To(HaveLen(1))
+		if _, ok := vm.GetLabels()[clusterv1.MachineControlPlaneLabel]; ok {
+			continue
 		}
+		finder := find.NewFinder(vsphereClient.Client, false)
+		dc, err := finder.Datacenter(ctx, vm.Spec.Datacenter)
+		Expect(err).NotTo(HaveOccurred())
+		finder.SetDatacenter(dc)
+
+		vmObj, err := finder.VirtualMachine(ctx, fmt.Sprintf("/%s/vm/%s/%s", vm.Spec.Datacenter, vm.Spec.Folder, vm.Name))
+		Expect(err).NotTo(HaveOccurred())
+		devices, err := vmObj.Device(ctx)
+		Expect(err).NotTo(HaveOccurred())
+		defaultPciDevices := devices.SelectByType((*types.VirtualPCIPassthrough)(nil))
+		Expect(defaultPciDevices).To(HaveLen(1))
 	}
 }
diff --git a/test/e2e/log_collector.go b/test/e2e/log_collector.go
diff --git a/test/helpers/vmware/intg_test_context.go b/test/helpers/vmware/intg_test_context.go
diff --git a/test/helpers/webhook.go b/test/helpers/webhook.go
diff --git a/test/integration/integration_suite_test.go b/test/integration/integration_suite_test.go

Original file line number	Diff line number	Diff line change
`@@ -610,7 +610,6 @@ func TestUnmarshalINI(t *testing.T) {`
`610`	`610`	`},`
`611`	`611`	`}`
`612`	`612`
`613`		`- //nolint:gocritic`
`614`	`613`	`testCases := append(`
`615`	`614`	`testcases,`
`616`	`615`	`deprecatedTestCases...,`
Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ import (`
`26`	`26`	`infrav1 "sigs.k8s.io/cluster-api-provider-vsphere/apis/v1beta1"`
`27`	`27`	`)`
`28`	`28`
`29`		`-// ConvertTo.`
	`29`	`+// ConvertTo converts this VSphereMachineTemplate to the Hub version (v1beta1).`
`30`	`30`	`func (src *VSphereMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {`
`31`	`31`	`dst := dstRaw.(*infrav1.VSphereMachineTemplate)`
`32`	`32`	`if err := Convert_v1alpha4_VSphereMachineTemplate_To_v1beta1_VSphereMachineTemplate(src, dst, nil); err != nil {`
`@@ -51,6 +51,7 @@ func (src *VSphereMachineTemplate) ConvertTo(dstRaw conversion.Hub) error {`
`51`	`51`	`return nil`
`52`	`52`	`}`
`53`	`53`
	`54`	`+// ConvertFrom converts from the Hub version (v1beta1) to this VSphereMachineTemplate.`
`54`	`55`	`func (dst *VSphereMachineTemplate) ConvertFrom(srcRaw conversion.Hub) error {`
`55`	`56`	`src := srcRaw.(*infrav1.VSphereMachineTemplate)`
`56`	`57`	`if err := Convert_v1beta1_VSphereMachineTemplate_To_v1alpha4_VSphereMachineTemplate(src, dst, nil); err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -429,7 +429,7 @@ func getSupervisorAPIServerURLWithFIP(client client.Client) (string, error) {`
`429`	`429`	`// tryParseClusterInfoFromConfigMap tries to parse a kubeconfig file from a ConfigMap key.`
`430`	`430`	`func tryParseClusterInfoFromConfigMap(cm corev1.ConfigMap) (clientcmdapi.Config, error) {`
`431`	`431`	`kubeConfigString, ok := cm.Data[bootstrapapi.KubeConfigKey]`
`432`		`- if !ok \|\| len(kubeConfigString) == 0 {`
	`432`	`+ if !ok \|\| kubeConfigString == "" {`
`433`	`433`	`return nil, errors.Errorf("no %s key in ConfigMap %s/%s", bootstrapapi.KubeConfigKey, cm.Namespace, cm.Name)`
`434`	`434`	`}`
`435`	`435`	`parsedKubeConfig, err := clientcmd.Load([]byte(kubeConfigString))`