Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 Enable all errorlint checks #839

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,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)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,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" {
Expand Down
4 changes: 2 additions & 2 deletions pkg/providers/vsphere/virtualmachine/backup.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,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))
Expand Down Expand Up @@ -364,7 +364,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 {
Expand Down
13 changes: 2 additions & 11 deletions pkg/providers/vsphere/vmlifecycle/bootstrap.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,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"
Expand Down Expand Up @@ -273,7 +273,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
}
}
Expand All @@ -292,15 +292,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",
Expand Down
4 changes: 3 additions & 1 deletion pkg/util/kube/spq/spq.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package spq

import (
"context"
"errors"
"fmt"
"strings"

Expand Down Expand Up @@ -67,7 +68,8 @@ func GetStorageClassesForPolicy(
namespace,
o.Name)
if err != nil {
if _, ok := err.(NotFoundInNamespace); !ok {
var notFoundInNamespace NotFoundInNamespace
if !errors.As(err, &notFoundInNamespace) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, can we not use https://pkg.go.dev/errors#Is here?

And another place, you have used fault.As for a similar check.

return nil, err
}
}
Expand Down
38 changes: 12 additions & 26 deletions pkg/util/vsphere/vm/hardware_version_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,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"

Expand Down Expand Up @@ -74,36 +73,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) {
Expand Down
18 changes: 8 additions & 10 deletions pkg/util/vsphere/vm/power_state.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,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"
Expand Down Expand Up @@ -331,15 +331,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)
Expand Down
7 changes: 4 additions & 3 deletions pkg/util/vsphere/watcher/watcher_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,16 +214,17 @@ 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())
var sysErr *os.SyscallError
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())
}
}

Expand Down
5 changes: 4 additions & 1 deletion test/testutil/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,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:
Expand All @@ -103,6 +104,8 @@ func ContainsError(err error, message string) bool {
return true
}
}
default:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why did you have to add the nolint and why this default block?

return false
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1337,7 +1337,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
Expand Down
Loading