From 74c81d787ee1e935f91eb1fb3b0892cca0338b4b Mon Sep 17 00:00:00 2001 From: Bryan Venteicher Date: Mon, 23 Dec 2024 17:14:02 -0600 Subject: [PATCH] Enable all errorlint checks Use %w instead of %v to wrap errors Use errors Is() and As() and the govmomi fault equivalents instead of type casting/checking --- .golangci.yml | 7 +--- .../virtualmachinereplicaset_controller.go | 2 +- .../content_library_provider.go | 2 +- .../vsphere/virtualmachine/backup.go | 4 +- .../vsphere/vmlifecycle/bootstrap.go | 13 +------ pkg/util/kube/spq/spq.go | 4 +- pkg/util/vsphere/vm/hardware_version_test.go | 38 ++++++------------- pkg/util/vsphere/vm/power_state.go | 18 ++++----- pkg/util/vsphere/watcher/watcher_test.go | 11 +++--- test/testutil/util.go | 5 ++- .../validation/virtualmachine_validator.go | 2 +- 11 files changed, 41 insertions(+), 65 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index f62c04fba..7aa2ba3f7 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -143,21 +143,16 @@ linters-settings: desc: "replaced by internal packages like pkg/util/ptr" - pkg: github.com/onsi/ginkgo$ desc: "replaced by github.com/onsi/ginkgo/v2" - errorlint: - # Check whether fmt.Errorf uses the %w verb for formatting errors. - errorf: false - # Check for plain type assertions and type switches. - asserts: false linters: disable-all: true enable: - - errorlint - asciicheck - bodyclose - depguard - dogsled - errcheck + - errorlint - exportloopref - goconst - gocritic diff --git a/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller.go b/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller.go index 4173a7d96..3d4ea8534 100644 --- a/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller.go +++ b/controllers/virtualmachinereplicaset/virtualmachinereplicaset_controller.go @@ -307,7 +307,7 @@ func (r *Reconciler) ReconcileNormal(ctx *pkgctx.VirtualMachineReplicaSetContext client.MatchingLabels(selectorMap), ); err != nil { return ctrl.Result{}, - fmt.Errorf("failed to list virtual machines matched by the VirtualMachineReplicaSet. selector: %q, err: %v", + fmt.Errorf("failed to list virtual machines matched by the VirtualMachineReplicaSet. selector: %q, err: %w", selectorMap, err) } diff --git a/pkg/providers/vsphere/contentlibrary/content_library_provider.go b/pkg/providers/vsphere/contentlibrary/content_library_provider.go index 6b48704fd..5c71954bd 100644 --- a/pkg/providers/vsphere/contentlibrary/content_library_provider.go +++ b/pkg/providers/vsphere/contentlibrary/content_library_provider.go @@ -357,7 +357,7 @@ func (cs *provider) generateDownloadURLForLibraryItem( if info.Status == "ERROR" { // Log message used by VMC LINT. Refer to before making changes - return false, fmt.Errorf("error occurred preparing file for download %v", info.ErrorMessage) + return false, fmt.Errorf("error occurred preparing file for download %w", info.ErrorMessage) } if info.Status != "PREPARED" { diff --git a/pkg/providers/vsphere/virtualmachine/backup.go b/pkg/providers/vsphere/virtualmachine/backup.go index d507e3821..171c45059 100644 --- a/pkg/providers/vsphere/virtualmachine/backup.go +++ b/pkg/providers/vsphere/virtualmachine/backup.go @@ -296,7 +296,7 @@ func trimBackupFields(obj client.Object) { func getEncodedVMYaml(vm *vmopv1.VirtualMachine) (string, error) { vmYAML, err := k8syaml.Marshal(vm) if err != nil { - return "", fmt.Errorf("failed to marshal VM into YAML %+v: %v", vm, err) + return "", fmt.Errorf("failed to marshal VM into YAML %+v: %w", vm, err) } return pkgutil.EncodeGzipBase64(string(vmYAML)) @@ -363,7 +363,7 @@ func getDesiredAdditionalResourceYAMLForBackup( trimBackupFields(resource) marshaledYaml, err := k8syaml.Marshal(resource) if err != nil { - return "", fmt.Errorf("failed to marshal object %q: %v", resource.GetName(), err) + return "", fmt.Errorf("failed to marshal object %q: %w", resource.GetName(), err) } sb.Write(marshaledYaml) if i != len(resources)-1 { diff --git a/pkg/providers/vsphere/vmlifecycle/bootstrap.go b/pkg/providers/vsphere/vmlifecycle/bootstrap.go index 93271ed74..7ec0d07ef 100644 --- a/pkg/providers/vsphere/vmlifecycle/bootstrap.go +++ b/pkg/providers/vsphere/vmlifecycle/bootstrap.go @@ -8,8 +8,8 @@ import ( "slices" "strings" + "github.com/vmware/govmomi/fault" "github.com/vmware/govmomi/object" - "github.com/vmware/govmomi/task" vimtypes "github.com/vmware/govmomi/vim25/types" apiEquality "k8s.io/apimachinery/pkg/api/equality" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -272,7 +272,7 @@ func doCustomize( if err := resources.NewVMFromObject(vcVM).Customize(vmCtx, *customSpec); err != nil { // isCustomizationPendingExtraConfig() above is supposed to prevent this error, but // handle it explicitly here just in case so VM reconciliation can proceed. - if !isCustomizationPendingError(err) { + if !fault.Is(err, &vimtypes.CustomizationPending{}) { return err } } @@ -291,15 +291,6 @@ func IsCustomizationPendingExtraConfig(extraConfig []vimtypes.BaseOptionValue) b return false } -func isCustomizationPendingError(err error) bool { - if te, ok := err.(task.Error); ok { - if _, ok := te.Fault().(*vimtypes.CustomizationPending); ok { - return true - } - } - return false -} - // linuxGuestIDPrefixes is derived from the vimtypes.VirtualMachineGuestOsIdentifier values. var linuxGuestIDPrefixes = [...]string{ "redhat", diff --git a/pkg/util/kube/spq/spq.go b/pkg/util/kube/spq/spq.go index ec69555fa..03594ac5f 100644 --- a/pkg/util/kube/spq/spq.go +++ b/pkg/util/kube/spq/spq.go @@ -5,6 +5,7 @@ package spq import ( "context" + "errors" "fmt" "strings" @@ -66,7 +67,8 @@ func GetStorageClassesForPolicy( namespace, o.Name) if err != nil { - if _, ok := err.(NotFoundInNamespace); !ok { + var notFoundInNamespace NotFoundInNamespace + if !errors.As(err, ¬FoundInNamespace) { return nil, err } } diff --git a/pkg/util/vsphere/vm/hardware_version_test.go b/pkg/util/vsphere/vm/hardware_version_test.go index c8aafea08..3b0ebd668 100644 --- a/pkg/util/vsphere/vm/hardware_version_test.go +++ b/pkg/util/vsphere/vm/hardware_version_test.go @@ -4,16 +4,15 @@ package vm_test import ( - "errors" "fmt" "github.com/go-logr/logr" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/vmware/govmomi/fault" "github.com/vmware/govmomi/object" "github.com/vmware/govmomi/simulator" - "github.com/vmware/govmomi/task" "github.com/vmware/govmomi/vim25/mo" vimtypes "github.com/vmware/govmomi/vim25/types" @@ -73,36 +72,23 @@ func hardwareVersionTests() { } assertInvalidPowerStateFaultPoweredOn := func(err error) { - ExpectWithOffset(1, err).To(HaveOccurred()) - err = errors.Unwrap(err) - ExpectWithOffset(1, err).To(HaveOccurred()) - ExpectWithOffset(1, err).To(BeAssignableToTypeOf(task.Error{})) - ExpectWithOffset(1, err.(task.Error).Fault()).ToNot(BeNil()) - ExpectWithOffset(1, err.(task.Error).Fault()).To(BeAssignableToTypeOf(&vimtypes.InvalidPowerStateFault{})) - fault := err.(task.Error).Fault().(*vimtypes.InvalidPowerStateFault) - ExpectWithOffset(1, fault.ExistingState).To(Equal(vimtypes.VirtualMachinePowerStatePoweredOn)) - ExpectWithOffset(1, fault.RequestedState).To(Equal(vimtypes.VirtualMachinePowerStatePoweredOff)) + var psFault *vimtypes.InvalidPowerStateFault + _, ok := fault.As(err, &psFault) + ExpectWithOffset(1, ok).To(BeTrue()) + ExpectWithOffset(1, psFault.ExistingState).To(Equal(vimtypes.VirtualMachinePowerStatePoweredOn)) + ExpectWithOffset(1, psFault.RequestedState).To(Equal(vimtypes.VirtualMachinePowerStatePoweredOff)) } assertInvalidPowerStateFaultSuspended := func(err error) { - ExpectWithOffset(1, err).To(HaveOccurred()) - err = errors.Unwrap(err) - ExpectWithOffset(1, err).To(HaveOccurred()) - ExpectWithOffset(1, err).To(BeAssignableToTypeOf(task.Error{})) - ExpectWithOffset(1, err.(task.Error).Fault()).ToNot(BeNil()) - ExpectWithOffset(1, err.(task.Error).Fault()).To(BeAssignableToTypeOf(&vimtypes.InvalidPowerStateFault{})) - fault := err.(task.Error).Fault().(*vimtypes.InvalidPowerStateFault) - ExpectWithOffset(1, fault.ExistingState).To(Equal(vimtypes.VirtualMachinePowerStateSuspended)) - ExpectWithOffset(1, fault.RequestedState).To(Equal(vimtypes.VirtualMachinePowerStatePoweredOff)) + var psFault *vimtypes.InvalidPowerStateFault + _, ok := fault.As(err, &psFault) + ExpectWithOffset(1, ok).To(BeTrue()) + ExpectWithOffset(1, psFault.ExistingState).To(Equal(vimtypes.VirtualMachinePowerStateSuspended)) + ExpectWithOffset(1, psFault.RequestedState).To(Equal(vimtypes.VirtualMachinePowerStatePoweredOff)) } assertAlreadyUpgradedFault := func(err error) { - ExpectWithOffset(1, err).To(HaveOccurred()) - err = errors.Unwrap(err) - ExpectWithOffset(1, err).To(HaveOccurred()) - ExpectWithOffset(1, err).To(BeAssignableToTypeOf(task.Error{})) - ExpectWithOffset(1, err.(task.Error).Fault()).ToNot(BeNil()) - ExpectWithOffset(1, err.(task.Error).Fault()).To(BeAssignableToTypeOf(&vimtypes.AlreadyUpgradedFault{})) + Expect(fault.Is(err, &vimtypes.AlreadyUpgradedFault{})).To(BeTrue()) } assertFailedToRetrievePropsNotFound := func(err error) { diff --git a/pkg/util/vsphere/vm/power_state.go b/pkg/util/vsphere/vm/power_state.go index 7b52ee15c..f3150b3d4 100644 --- a/pkg/util/vsphere/vm/power_state.go +++ b/pkg/util/vsphere/vm/power_state.go @@ -13,8 +13,8 @@ import ( "github.com/go-logr/logr" + "github.com/vmware/govmomi/fault" "github.com/vmware/govmomi/object" - "github.com/vmware/govmomi/task" "github.com/vmware/govmomi/vim25" "github.com/vmware/govmomi/vim25/mo" vimtypes "github.com/vmware/govmomi/vim25/types" @@ -330,15 +330,13 @@ func doAndWaitOnHardPowerOp( "failed to invoke hard power op for %s %w", desiredPowerState, err) } if ti, err := t.WaitForResult(ctx); err != nil { - if err, ok := err.(task.Error); ok { - // Ignore error if desired power state already set. - if ips, ok := err.Fault().(*vimtypes.InvalidPowerState); ok && ips.ExistingState == ips.RequestedState { - log.Info( - "Power state already set", - "desiredPowerState", - desiredPowerState) - return PowerOpResultNone, nil - } + var ips *vimtypes.InvalidPowerState + if _, ok := fault.As(err, &ips); ok && ips.ExistingState == ips.RequestedState { + log.Info( + "Power state already set", + "desiredPowerState", + desiredPowerState) + return PowerOpResultNone, nil } if ti != nil { log.Error(err, "Change power state task failed", "taskInfo", ti) diff --git a/pkg/util/vsphere/watcher/watcher_test.go b/pkg/util/vsphere/watcher/watcher_test.go index a3cee81a1..46b55fdad 100644 --- a/pkg/util/vsphere/watcher/watcher_test.go +++ b/pkg/util/vsphere/watcher/watcher_test.go @@ -213,8 +213,9 @@ var _ = Describe("Start", func() { ExpectWithOffset(1, errors.As(w.Err(), &urlErr)).To(BeTrue()) ExpectWithOffset(1, urlErr.Op).To(Equal("Post")) - switch tErr := urlErr.Err.(type) { - case *net.OpError: + var tErr *net.OpError + switch { + case errors.As(urlErr.Err, &tErr): ExpectWithOffset(1, tErr.Op).To(Equal("dial")) ExpectWithOffset(1, tErr.Net).To(Equal("tcp")) ExpectWithOffset(1, tErr.Err).ToNot(BeNil()) @@ -222,11 +223,11 @@ var _ = Describe("Start", func() { ExpectWithOffset(1, errors.As(tErr.Err, &sysErr)).To(BeTrue()) ExpectWithOffset(1, sysErr.Syscall).To(Equal("connect")) default: - ExpectWithOffset(1, tErr).To(HaveOccurred()) + ExpectWithOffset(1, urlErr.Err).To(HaveOccurred()) } } - When("the context is cancelled", FlakeAttempts(5), func() { + When("the context is cancelled", FlakeAttempts(1), func() { JustBeforeEach(func() { cancel() }) @@ -247,7 +248,7 @@ var _ = Describe("Start", func() { }) }) - When("the connection to vSphere is interrupted", FlakeAttempts(5), func() { + When("the connection to vSphere is interrupted", FlakeAttempts(1), func() { JustBeforeEach(func() { closeServerOnce.Do(server.Close) }) diff --git a/test/testutil/util.go b/test/testutil/util.go index 2c84b8a57..1c645220c 100644 --- a/test/testutil/util.go +++ b/test/testutil/util.go @@ -93,7 +93,8 @@ func ContainsError(err error, message string) bool { if err.Error() == message { return true } - switch tErr := err.(type) { + + switch tErr := err.(type) { //nolint:errorlint case unwrappableError: err = tErr.Unwrap() case unwrappableErrorSlice: @@ -102,6 +103,8 @@ func ContainsError(err error, message string) bool { return true } } + default: + return false } } } diff --git a/webhooks/virtualmachine/validation/virtualmachine_validator.go b/webhooks/virtualmachine/validation/virtualmachine_validator.go index 87e699ec9..9192c7e37 100644 --- a/webhooks/virtualmachine/validation/virtualmachine_validator.go +++ b/webhooks/virtualmachine/validation/virtualmachine_validator.go @@ -1332,7 +1332,7 @@ func (v validator) isNetworkRestrictedForReadinessProbe(ctx *pkgctx.WebhookReque configMap := &corev1.ConfigMap{} configMapKey := ctrlclient.ObjectKey{Name: config.ProviderConfigMapName, Namespace: ctx.Namespace} if err := v.client.Get(ctx, configMapKey, configMap); err != nil { - return false, fmt.Errorf("error get ConfigMap: %s while validating TCP readiness probe port: %v", configMapKey, err) + return false, fmt.Errorf("error get ConfigMap: %s while validating TCP readiness probe port: %w", configMapKey, err) } return configMap.Data[isRestrictedNetworkKey] == "true", nil