From 2ecacc3027fba3c13a25a96e82ac3fe308313637 Mon Sep 17 00:00:00 2001 From: Drew Sirenko <68304519+AndrewSirenko@users.noreply.github.com> Date: Fri, 1 Nov 2024 09:50:59 -0400 Subject: [PATCH] Enable golang-ci linters Additional fixes (to be squashed) --- .golangci.yml | 129 +++++++------------------------- cmd/main.go | 4 +- pkg/driver/controller.go | 4 +- pkg/mounter/mount_linux.go | 34 ++++----- pkg/mounter/mount_linux_test.go | 4 - pkg/util/util_test.go | 5 +- 6 files changed, 49 insertions(+), 131 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index a99c01395d..f77b17e7c5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -35,113 +35,36 @@ linters-settings: - "github.com/onsi/gomega" - "github.com/onsi/ginkgo/v2" linters: - enable: - - asasalint - - asciicheck - - bidichk - - bodyclose - - canonicalheader - - containedctx - - contextcheck - - copyloopvar - - decorder - - dogsled - - dupword - - durationcheck - - errcheck - - errchkjson - - errname - - errorlint - - exhaustive - - fatcontext - - forbidigo - - forcetypeassert - - gci - - ginkgolinter - - gocheckcompilerdirectives - - gochecksumtype - - gocritic - - gofmt - - goheader - - goimports - - gomodguard - - goprintffuncname - - gosimple - - gosmopolitan - - grouper - - importas - - inamedparam - - ineffassign - - intrange - - loggercheck - - makezero - - mirror - - misspell - - musttag - - nakedret - - nilerr - - nilnil - - noctx - - nolintlint - - nosprintfhostport - - prealloc - - predeclared - - promlinter - - protogetter - - reassign - - rowserrcheck - - sloglint - - spancheck - - sqlclosecheck - - staticcheck - - stylecheck - - tagalign - - tagliatelle - - tenv - - testableexamples - - testifylint - - thelper - - tparallel - - unconvert - - unparam - - unused - - usestdlibvars - - wastedassign - - whitespace - - zerologlint - - goconst - - gochecknoinits - - perfsprint - - revive - - gosec - - godot # End all godoc comments with period + enable-all: true disable: - govet # We already run with `make verify/govet` # We do not use - - gomoddirectives # We need `replace` in `go.mod` + - cyclop # Cyclomatic complexity - depguard # We don't guard against dependencies - - testpackage # Require separate test package to catch leaky unexported dependencies + - dupl # Tracks code duplication. Too much duplication in tests. False positives in non-tests + - execinquery # Deprecated but enabled-by-default + - exhaustruct # Explicitly instantiating all structs is painful for K8s structs + - exportloopref # Deprecated but enabled-by-default - funlen # Long func names happen - - varnamelen # Long var names happen - - maintidx # Maintainability index - - cyclop # Cyclomatic complexity - gocognit # Cognitive complexity - - wsl # Too strict of a whitespace linter - # TODO Investigate FIXME - - err113 # TODO FIXME do not create errors dynamically from scratch. Instead wrap static (package-level) error. 2 points - - wrapcheck # TODO ^^ - - gochecknoglobals # TODO FIXME A whole 2 point task on its own - - paralleltest # TODO FIXME (Too many tests that aren't parallelized right now, probably half a day to attempt to mark the ones we can) - - nestif # TODO whole refactoring/readability task, should split up - - godox # TODO FIXME audit our project TODOs - # TODO Q: Consult with team + - gocyclo # Cyclomatic complexity + - gofumpt # We don't rely on gofumpt + - gomnd # Magic Number Detection. Many false positives. + - gomoddirectives # We need `replace` in `go.mod` + - interfacebloat # No more than 10 interface methods + - ireturn # Accept interfaces return concrete types - lll # Limit line length - - gofumpt # Rely on gofumpt's stricter formatting opinions - - ireturn # Always accept interfaces return concrete types (gopherism) - - nlreturn # Always have emptyline before return - - nonamedreturns # No using named returns in functions. Maybe nolint a few of the places it actually leads to cleaner code - - exhaustruct # Forces you to explicitly instantiate all structs. REALLY painful for kubernetes structs... - - interfacebloat # Cloud and Mounter have 15 methods instead of linter's recommended 10... Smell or necessary? We can nolint specific ones - - mnd # Magic Number Detection. Many false positives, still worth with nolint? - - gomnd # Many false positives, still worth with nolint? - - dupl # Tracks code duplication. Brutal amount of duplication in tests. False positives in non-tests. + - maintidx # Maintainability index + - mnd # Magic Number Detection. Many false positives + - nestif # Don't allow too many nested if statements + - nlreturn # Always have empty line before return + - testpackage # Require separate test package to catch leaky unexported dependencies + - varnamelen # Long var names happen + - wsl # Too strict of a whitespace linter + # Consider adding in future + - err113 # Do not create errors dynamically from scratch. Instead, wrap static (package-level) error. + - wrapcheck # Same as err113 + - gochecknoglobals # Do not allow global variables + - godox # Do not allow TODOs + - nonamedreturns # Need to nolint/refactor a few places our code + - paralleltest # There are many tests that aren't parallelized diff --git a/cmd/main.go b/cmd/main.go index f0fe09abbf..3579f23350 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -18,6 +18,7 @@ package main import ( "context" + "fmt" "os" "strings" "time" @@ -107,7 +108,8 @@ func main() { klog.ErrorS(err, "failed to get version") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } - klog.Info(versionInfo) + //nolint:forbidigo // Print version info without klog/timestamp + fmt.Println(versionInfo) os.Exit(0) } diff --git a/pkg/driver/controller.go b/pkg/driver/controller.go index dc4e0ef081..352cc278c2 100644 --- a/pkg/driver/controller.go +++ b/pkg/driver/controller.go @@ -486,11 +486,11 @@ func (d *ControllerService) ControllerGetCapabilities(ctx context.Context, req * klog.V(4).InfoS("ControllerGetCapabilities: called", "args", req) caps := make([]*csi.ControllerServiceCapability, 0, len(controllerCaps)) - for _, cap := range controllerCaps { + for _, capability := range controllerCaps { c := &csi.ControllerServiceCapability{ Type: &csi.ControllerServiceCapability_Rpc{ Rpc: &csi.ControllerServiceCapability_RPC{ - Type: cap, + Type: capability, }, }, } diff --git a/pkg/mounter/mount_linux.go b/pkg/mounter/mount_linux.go index ccb9d0863b..82cce121f1 100644 --- a/pkg/mounter/mount_linux.go +++ b/pkg/mounter/mount_linux.go @@ -54,9 +54,9 @@ func NewSafeMounterV2() (*mountutils.SafeFormatAndMount, error) { // FindDevicePath finds path of device and verifies its existence // if the device is not nvme, return the path directly -// if the device is nvme, finds and returns the nvme device path eg. /dev/nvme1n1 +// if the device is nvme, finds and returns the nvme device path eg. /dev/nvme1n1. func (m *NodeMounter) FindDevicePath(devicePath, volumeID, partition, region string) (string, error) { - strippedVolumeName := strings.Replace(volumeID, "-", "", -1) + strippedVolumeName := strings.ReplaceAll(volumeID, "-", "") canonicalDevicePath := "" // If the given path exists, the device MAY be nvme. Further, it MAY be a @@ -132,7 +132,7 @@ func (m *NodeMounter) FindDevicePath(devicePath, volumeID, partition, region str } // findNvmeVolume looks for the nvme volume with the specified name -// It follows the symlink (if it exists) and returns the absolute path to the device +// It follows the symlink (if it exists) and returns the absolute path to the device. func findNvmeVolume(findName string) (device string, err error) { p := filepath.Join("/dev/disk/by-id/", findName) stat, err := os.Lstat(p) @@ -163,12 +163,12 @@ func findNvmeVolume(findName string) (device string, err error) { } // execRunner is a helper to inject exec.Comamnd().CombinedOutput() for verifyVolumeSerialMatch -// Tests use a mocked version that does not actually execute any binaries +// Tests use a mocked version that does not actually execute any binaries. func execRunner(name string, arg ...string) ([]byte, error) { return exec.Command(name, arg...).CombinedOutput() } -// verifyVolumeSerialMatch checks the volume serial of the device against the expected volume +// verifyVolumeSerialMatch checks the volume serial of the device against the expected volume. func verifyVolumeSerialMatch(canonicalDevicePath string, strippedVolumeName string, execRunner func(string, ...string) ([]byte, error)) error { // In some rare cases, a race condition can lead to the /dev/disk/by-id/ symlink becoming out of date // See https://github.com/kubernetes-sigs/aws-ebs-csi-driver/issues/1224 for more info @@ -194,7 +194,7 @@ func verifyVolumeSerialMatch(canonicalDevicePath string, strippedVolumeName stri return nil } -// PreparePublishTarget creates the target directory for the volume to be mounted +// PreparePublishTarget creates the target directory for the volume to be mounted. func (m *NodeMounter) PreparePublishTarget(target string) error { klog.V(4).InfoS("NodePublishVolume: creating dir", "target", target) if err := m.MakeDir(target); err != nil { @@ -203,7 +203,7 @@ func (m *NodeMounter) PreparePublishTarget(target string) error { return nil } -// IsBlockDevice checks if the given path is a block device +// IsBlockDevice checks if the given path is a block device. func (m *NodeMounter) IsBlockDevice(fullPath string) (bool, error) { var st unix.Stat_t err := unix.Stat(fullPath, &st) @@ -214,7 +214,7 @@ func (m *NodeMounter) IsBlockDevice(fullPath string) (bool, error) { return (st.Mode & unix.S_IFMT) == unix.S_IFBLK, nil } -// GetBlockSizeBytes gets the size of the disk in bytes +// GetBlockSizeBytes gets the size of the disk in bytes. func (m *NodeMounter) GetBlockSizeBytes(devicePath string) (int64, error) { output, err := m.Exec.Command("blockdev", "--getsize64", devicePath).Output() if err != nil { @@ -228,7 +228,7 @@ func (m *NodeMounter) GetBlockSizeBytes(devicePath string) (int64, error) { return gotSizeBytes, nil } -// appendPartition appends the partition to the device path +// appendPartition appends the partition to the device path. func (m *NodeMounter) appendPartition(devicePath, partition string) string { if partition == "" { return devicePath @@ -246,13 +246,13 @@ func (m NodeMounter) GetDeviceNameFromMount(mountPath string) (string, int, erro return mountutils.GetDeviceNameFromMount(m, mountPath) } -// IsCorruptedMnt return true if err is about corrupted mount point +// IsCorruptedMnt return true if err is about corrupted mount point. func (m NodeMounter) IsCorruptedMnt(err error) bool { return mountutils.IsCorruptedMnt(err) } // This function is mirrored in ./sanity_test.go to make sure sanity test covered this block of code -// Please mirror the change to func MakeFile in ./sanity_test.go +// Please mirror the change to func MakeFile in ./sanity_test.go. func (m *NodeMounter) MakeFile(path string) error { f, err := os.OpenFile(path, os.O_CREATE, os.FileMode(0644)) if err != nil { @@ -267,7 +267,7 @@ func (m *NodeMounter) MakeFile(path string) error { } // This function is mirrored in ./sanity_test.go to make sure sanity test covered this block of code -// Please mirror the change to func MakeFile in ./sanity_test.go +// Please mirror the change to func MakeFile in ./sanity_test.go. func (m *NodeMounter) MakeDir(path string) error { err := os.MkdirAll(path, os.FileMode(0755)) if err != nil { @@ -279,28 +279,28 @@ func (m *NodeMounter) MakeDir(path string) error { } // This function is mirrored in ./sanity_test.go to make sure sanity test covered this block of code -// Please mirror the change to func MakeFile in ./sanity_test.go +// Please mirror the change to func MakeFile in ./sanity_test.go. func (m *NodeMounter) PathExists(path string) (bool, error) { return mountutils.PathExists(path) } -// Resize resizes the filesystem of the given devicePath +// Resize resizes the filesystem of the given devicePath. func (m *NodeMounter) Resize(devicePath, deviceMountPath string) (bool, error) { return mountutils.NewResizeFs(m.Exec).Resize(devicePath, deviceMountPath) } -// NeedResize checks if the filesystem of the given devicePath needs to be resized +// NeedResize checks if the filesystem of the given devicePath needs to be resized. func (m *NodeMounter) NeedResize(devicePath string, deviceMountPath string) (bool, error) { return mountutils.NewResizeFs(m.Exec).NeedResize(devicePath, deviceMountPath) } -// Unpublish unmounts the given path +// Unpublish unmounts the given path. func (m *NodeMounter) Unpublish(path string) error { // On linux, unpublish and unstage both perform an unmount return m.Unstage(path) } -// Unstage unmounts the given path +// Unstage unmounts the given path. func (m *NodeMounter) Unstage(path string) error { err := mountutils.CleanupMountPoint(path, m, false) // Ignore the error when it contains "not mounted", because that indicates the diff --git a/pkg/mounter/mount_linux_test.go b/pkg/mounter/mount_linux_test.go index c36826642f..ebb4f70b46 100644 --- a/pkg/mounter/mount_linux_test.go +++ b/pkg/mounter/mount_linux_test.go @@ -27,7 +27,6 @@ import ( "github.com/stretchr/testify/assert" "k8s.io/mount-utils" - utilexec "k8s.io/utils/exec" fakeexec "k8s.io/utils/exec/testing" ) @@ -138,7 +137,6 @@ func TestMakeFile(t *testing.T) { if exists, err := mountObj.PathExists(targetPath); !exists { t.Fatalf("Expect no error but got: %v", err) } - } func TestPathExists(t *testing.T) { @@ -165,7 +163,6 @@ func TestPathExists(t *testing.T) { if exists { t.Fatalf("Expected file %s to not exist", targetPath) } - } func TestGetDeviceName(t *testing.T) { @@ -186,7 +183,6 @@ func TestGetDeviceName(t *testing.T) { if _, _, err := mountObj.GetDeviceNameFromMount(targetPath); err != nil { t.Fatalf("Expect no error but got: %v", err) } - } func TestFindDevicePath(t *testing.T) { diff --git a/pkg/util/util_test.go b/pkg/util/util_test.go index 56aac97efe..2d9f6cf760 100644 --- a/pkg/util/util_test.go +++ b/pkg/util/util_test.go @@ -24,9 +24,8 @@ import ( "reflect" "testing" - "github.com/stretchr/testify/assert" - csi "github.com/container-storage-interface/spec/lib/go/csi" + "github.com/stretchr/testify/assert" ) func TestRoundUpBytes(t *testing.T) { @@ -119,7 +118,6 @@ func TestParseEndpoint(t *testing.T) { if err.Error() != tc.expErr.Error() { t.Fatalf("Expecting err: expected %v, got %v", tc.expErr, err) } - } else { if err != nil { t.Fatalf("err is not nil. got: %v", err) @@ -134,7 +132,6 @@ func TestParseEndpoint(t *testing.T) { } }) } - } func TestGetAccessModes(t *testing.T) {