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 recommended golang-ci linters #2204

Merged
merged 24 commits into from
Nov 4, 2024
Merged
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
bc0d1c0
Enable default golangci linters
AndrewSirenko Oct 29, 2024
a8e7848
Enable whitespace linter
AndrewSirenko Oct 29, 2024
3d3840b
cleanup: Delete copying of loop variables (Go 1.22+)
AndrewSirenko Oct 29, 2024
3d3f9a0
Enable dupword linter
AndrewSirenko Oct 29, 2024
e239fd8
cleanup: Ensure all variable names conform to 'ErrXxx' format
AndrewSirenko Oct 29, 2024
1dd1c11
cleanup: Do not log with fmt.Println (forbidigo)
AndrewSirenko Oct 29, 2024
9dd6e71
Enable forcetypeassert linter
AndrewSirenko Oct 29, 2024
43d3f17
cleanup: enable gci linter to make package import order deterministic
AndrewSirenko Oct 30, 2024
49aae91
cleanup: enable gocritic linter and simplify elseIf chains
AndrewSirenko Oct 30, 2024
39efb4e
cleanup: enable goimports linter
AndrewSirenko Oct 30, 2024
2ffd949
cleanup: Enable unconvert linter and remove unecessary type conversions
AndrewSirenko Oct 30, 2024
ef3bba5
cleanup: change for loops to use integer range (Go 1.22+)
AndrewSirenko Oct 30, 2024
42f7b9d
cleanup: pre-allocate slices to increase performance
AndrewSirenko Oct 30, 2024
0a6d0c0
cleanup: Use t.Setenv() in unit tests instead of os.Setenv
AndrewSirenko Oct 30, 2024
735a034
cleanup: Add t.Parallel() to more unit tests
AndrewSirenko Oct 30, 2024
2673e24
cleanup: Rename vars with same name as a predeclared identifier
AndrewSirenko Oct 30, 2024
c18bef1
Enable stylecheck linter; Fix ST1003 & ST1005
AndrewSirenko Oct 30, 2024
4b723c6
Ensure test helper functions start from t.Helper()
AndrewSirenko Oct 30, 2024
1267d0a
Enable goconst linter
AndrewSirenko Oct 30, 2024
46c49c4
Enable gochecknoinits linter
AndrewSirenko Oct 30, 2024
af016de
Enable perfsprint linter
AndrewSirenko Oct 30, 2024
a2fc4a4
Enable gosec; Fix potential integer overflows
AndrewSirenko Oct 30, 2024
aa668bd
Ensure comments end with period as godoc suggests
AndrewSirenko Oct 30, 2024
c0949b3
Enable golang-ci linters
AndrewSirenko Nov 1, 2024
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
Prev Previous commit
Next Next commit
cleanup: enable gocritic linter and simplify elseIf chains
AndrewSirenko committed Nov 1, 2024
commit 49aae916fdeb80e004877309cf077a64018f32ce
13 changes: 5 additions & 8 deletions .golangci.yml
Original file line number Diff line number Diff line change
@@ -39,14 +39,8 @@ linters:
- gci
- ginkgolinter
- gocheckcompilerdirectives
# - gochecknoglobals
# - gochecknoinits
# - gochecksumtype
# - gocognit
# - goconst
# - gocritic
# - gocyclo
# - godot
- gochecksumtype
- gocritic
# - godox
# - gofmt
# - gofumpt
@@ -137,3 +131,6 @@ linters:
- nestif # Many false positives
- gocognit # Many false positives
- cyclop # Many false positives
- godot # TODO Ask team
- goconst # TODO FIXME
- gochecknoinits # TODO FIXME
9 changes: 5 additions & 4 deletions cmd/hooks/prestop.go
Original file line number Diff line number Diff line change
@@ -63,14 +63,15 @@ func PreStop(clientset kubernetes.Interface) error {
}

node, err := fetchNode(clientset, nodeName)
if k8serrors.IsNotFound(err) {
switch {
case k8serrors.IsNotFound(err):
klog.InfoS("PreStop: node does not exist - assuming this is a termination event, checking for remaining VolumeAttachments", "node", nodeName)
} else if err != nil {
case err != nil:
return err
} else if !isNodeBeingDrained(node) {
case !isNodeBeingDrained(node):
klog.InfoS("PreStop: node is not being drained, skipping VolumeAttachments check", "node", nodeName)
return nil
} else {
default:
klog.InfoS("PreStop: node is being drained, checking for remaining VolumeAttachments", "node", nodeName)
}

11 changes: 6 additions & 5 deletions pkg/cloud/cloud.go
Original file line number Diff line number Diff line change
@@ -127,7 +127,7 @@ const (
KubernetesTagKeyPrefix = "kubernetes.io"
// AWSTagKeyPrefix is the prefix of the key value that is reserved for AWS.
AWSTagKeyPrefix = "aws:"
//AwsEbsDriverTagKey is the tag to identify if a volume/snapshot is managed by ebs csi driver
// AwsEbsDriverTagKey is the tag to identify if a volume/snapshot is managed by ebs csi driver
AwsEbsDriverTagKey = "ebs.csi.aws.com/cluster"
)

@@ -1700,13 +1700,14 @@ func (c *cloud) checkDesiredState(ctx context.Context, volumeID string, desiredS

// Check if there is a mismatch between the requested modification and the current volume
// If there is, the volume is still modifying and we should not return a success
if realSizeGiB < desiredSizeGiB {
switch {
case realSizeGiB < desiredSizeGiB:
return realSizeGiB, fmt.Errorf("volume %q is still being expanded to %d size", volumeID, desiredSizeGiB)
} else if options.IOPS != 0 && (volume.Iops == nil || *volume.Iops != options.IOPS) {
case options.IOPS != 0 && (volume.Iops == nil || *volume.Iops != options.IOPS):
return realSizeGiB, fmt.Errorf("volume %q is still being modified to iops %d", volumeID, options.IOPS)
} else if options.VolumeType != "" && !strings.EqualFold(string(volume.VolumeType), options.VolumeType) {
case options.VolumeType != "" && !strings.EqualFold(string(volume.VolumeType), options.VolumeType):
return realSizeGiB, fmt.Errorf("volume %q is still being modified to type %q", volumeID, options.VolumeType)
} else if options.Throughput != 0 && (volume.Throughput == nil || *volume.Throughput != options.Throughput) {
case options.Throughput != 0 && (volume.Throughput == nil || *volume.Throughput != options.Throughput):
return realSizeGiB, fmt.Errorf("volume %q is still being modified to throughput %d", volumeID, options.Throughput)
}

38 changes: 12 additions & 26 deletions pkg/cloud/cloud_test.go
Original file line number Diff line number Diff line change
@@ -2667,24 +2667,14 @@ func TestResizeOrModifyDisk(t *testing.T) {
}

newSize, err := c.ResizeOrModifyDisk(ctx, tc.volumeID, util.GiBToBytes(tc.reqSizeGiB), tc.modifyDiskOptions)
if err != nil {
if tc.expErr == nil {
t.Fatalf("ResizeOrModifyDisk() failed: expected no error, got: %v", err)
} else {
if errors.Is(tc.expErr, ErrInvalidArgument) {
if !errors.Is(err, ErrInvalidArgument) {
t.Fatalf("ResizeOrModifyDisk() failed: expected ErrInvalidArgument, got: %v", err)
}
}
}
} else {
if tc.expErr != nil {
t.Fatal("ResizeOrModifyDisk() failed: expected error, got nothing")
} else {
if tc.reqSizeGiB != newSize {
t.Fatalf("ResizeOrModifyDisk() failed: expected capacity %d, got %d", tc.reqSizeGiB, newSize)
}
}
switch {
case errors.Is(tc.expErr, ErrInvalidArgument):
require.ErrorIs(t, err, ErrInvalidArgument, "ResizeOrModifyDisk() should return ErrInvalidArgument")
case tc.expErr != nil:
require.Error(t, err, "ResizeOrModifyDisk() should return error")
default:
require.NoError(t, err, "ResizeOrModifyDisk() should not return error")
assert.Equal(t, tc.reqSizeGiB, newSize, "ResizeOrModifyDisk() returned unexpected capacity")
}

mockCtrl.Finish()
@@ -2788,15 +2778,11 @@ func TestModifyTags(t *testing.T) {
if err != nil {
if tc.expErr == nil {
t.Fatalf("ModifyTags() failed: expected no error, got: %v", err)
} else {
if !strings.Contains(err.Error(), tc.expErr.Error()) {
t.Fatalf("ModifyTags() failed: expected error %v, got: %v", tc.expErr, err)
}
}
} else {
if tc.expErr != nil {
t.Fatal("ModifyTags() failed: expected error, got nothing")
} else if !strings.Contains(err.Error(), tc.expErr.Error()) {
t.Fatalf("ModifyTags() failed: expected error %v, got: %v", tc.expErr, err)
}
} else if tc.expErr != nil {
t.Fatal("ModifyTags() failed: expected error, got nothing")
}

mockCtrl.Finish()
7 changes: 4 additions & 3 deletions pkg/coalescer/coalescer_test.go
Original file line number Diff line number Diff line change
@@ -95,11 +95,12 @@ func TestCoalescer(t *testing.T) {
for range tc.inputs {
err := <-testChannel
if err != nil {
if errors.Is(err, errFailedToMerge) {
switch {
case errors.Is(err, errFailedToMerge):
mergeFailure = true
} else if errors.Is(err, errFailedToExecute) {
case errors.Is(err, errFailedToExecute):
executeFailure = true
} else {
default:
t.Fatalf("Unexpected error %v", err)
}
}
15 changes: 7 additions & 8 deletions pkg/driver/controller.go
Original file line number Diff line number Diff line change
@@ -645,8 +645,8 @@ func isBlock(cap *csi.VolumeCapability) bool {
}

func isValidVolumeContext(volContext map[string]string) bool {
//There could be multiple volume attributes in the volumeContext map
//Validate here case by case
// There could be multiple volume attributes in the volumeContext map
// Validate here case by case
if partition, ok := volContext[VolumeAttributePartition]; ok {
partitionInt, err := strconv.ParseInt(partition, 10, 64)
if err != nil {
@@ -1017,17 +1017,17 @@ func getVolSizeBytes(req *csi.CreateVolumeRequest) (int64, error) {

// BuildOutpostArn returns the string representation of the outpost ARN from the given csi.TopologyRequirement.segments
func BuildOutpostArn(segments map[string]string) string {
if len(segments[AwsPartitionKey]) <= 0 {
if len(segments[AwsPartitionKey]) == 0 {
return ""
}

if len(segments[AwsRegionKey]) <= 0 {
if len(segments[AwsRegionKey]) == 0 {
return ""
}
if len(segments[AwsOutpostIDKey]) <= 0 {
if len(segments[AwsOutpostIDKey]) == 0 {
return ""
}
if len(segments[AwsAccountIDKey]) <= 0 {
if len(segments[AwsAccountIDKey]) == 0 {
return ""
}

@@ -1041,8 +1041,7 @@ func BuildOutpostArn(segments map[string]string) string {

func validateFormattingOption(volumeCapabilities []*csi.VolumeCapability, paramName string, fsConfigs map[string]fileSystemConfig) error {
for _, volCap := range volumeCapabilities {
switch volCap.GetAccessType().(type) {
case *csi.VolumeCapability_Block:
if _, isBlockVolCap := volCap.GetAccessType().(*csi.VolumeCapability_Block); isBlockVolCap {
return status.Error(codes.InvalidArgument, fmt.Sprintf("Cannot use %s with block volume", paramName))
}

7 changes: 4 additions & 3 deletions pkg/driver/controller_modify_volume.go
Original file line number Diff line number Diff line change
@@ -192,15 +192,16 @@ func parseModifyVolumeParameters(params map[string]string) (*modifyVolumeRequest
case ModificationKeyVolumeType:
options.modifyDiskOptions.VolumeType = value
default:
if strings.HasPrefix(key, ModificationAddTag) {
switch {
case strings.HasPrefix(key, ModificationAddTag):
st := strings.SplitN(value, "=", 2)
if len(st) < 2 {
return nil, status.Errorf(codes.InvalidArgument, "Invalid tag specification: %v", st)
}
options.modifyTagsOptions.TagsToAdd[st[0]] = st[1]
} else if strings.HasPrefix(key, ModificationDeleteTag) {
case strings.HasPrefix(key, ModificationDeleteTag):
options.modifyTagsOptions.TagsToDelete = append(options.modifyTagsOptions.TagsToDelete, value)
} else {
default:
return nil, status.Errorf(codes.InvalidArgument, "Invalid mutable parameter key: %s", key)
}
}
6 changes: 2 additions & 4 deletions pkg/driver/controller_test.go
Original file line number Diff line number Diff line change
@@ -3666,10 +3666,8 @@ func TestControllerExpandVolume(t *testing.T) {
if !tc.expError {
t.Fatalf("Unexpected error: %v", err)
}
} else {
if tc.expError {
t.Fatalf("Expected error from ControllerExpandVolume, got nothing")
}
} else if tc.expError {
t.Fatalf("Expected error from ControllerExpandVolume, got nothing")
}

sizeGiB := util.BytesToGiB(resp.GetCapacityBytes())
2 changes: 1 addition & 1 deletion pkg/driver/driver.go
Original file line number Diff line number Diff line change
@@ -52,7 +52,7 @@ const (
AwsRegionKey = "topology." + DriverName + "/region"
AwsOutpostIDKey = "topology." + DriverName + "/outpost-id"
WellKnownZoneTopologyKey = "topology.kubernetes.io/zone"
// DEPRECATED Use the WellKnownZoneTopologyKey instead
// Deprecated: Use the WellKnownZoneTopologyKey instead
ZoneTopologyKey = "topology." + DriverName + "/zone"
OSTopologyKey = "kubernetes.io/os"
)
13 changes: 6 additions & 7 deletions pkg/driver/node.go
Original file line number Diff line number Diff line change
@@ -136,8 +136,7 @@ func (d *NodeService) NodeStageVolume(ctx context.Context, req *csi.NodeStageVol
}

// If the access type is block, do nothing for stage
switch volCap.GetAccessType().(type) {
case *csi.VolumeCapability_Block:
if _, isAccessTypeBlock := volCap.GetAccessType().(*csi.VolumeCapability_Block); isAccessTypeBlock {
return &csi.NodeStageVolumeResponse{}, nil
}

@@ -657,7 +656,7 @@ func (d *NodeService) nodePublishVolumeForBlock(req *csi.NodePublishVolumeReques
return status.Errorf(codes.Internal, "Could not create file %q: %v", target, err)
}

//Checking if the target file is already mounted with a device.
// Checking if the target file is already mounted with a device.
mounted, err := d.isMounted(source, target)
if err != nil {
return status.Errorf(codes.Internal, "Could not check if %q is mounted: %v", target, err)
@@ -689,14 +688,14 @@ func (d *NodeService) isMounted(_ string, target string) (bool, error) {
*/
notMnt, err := d.mounter.IsLikelyNotMountPoint(target)
if err != nil && !os.IsNotExist(err) {
//Checking if the path exists and error is related to Corrupted Mount, in that case, the system could unmount and mount.
// Checking if the path exists and error is related to Corrupted Mount, in that case, the system could unmount and mount.
_, pathErr := d.mounter.PathExists(target)
if pathErr != nil && d.mounter.IsCorruptedMnt(pathErr) {
klog.V(4).InfoS("NodePublishVolume: Target path is a corrupted mount. Trying to unmount.", "target", target)
if mntErr := d.mounter.Unpublish(target); mntErr != nil {
return false, status.Errorf(codes.Internal, "Unable to unmount the target %q : %v", target, mntErr)
}
//After successful unmount, the device is ready to be mounted.
// After successful unmount, the device is ready to be mounted.
return false, nil
}
return false, status.Errorf(codes.Internal, "Could not check if %q is a mount point: %v, %v", target, err, pathErr)
@@ -735,7 +734,7 @@ func (d *NodeService) nodePublishVolumeForFileSystem(req *csi.NodePublishVolumeR
return status.Errorf(codes.Internal, "%s", err.Error())
}

//Checking if the target directory is already mounted with a device.
// Checking if the target directory is already mounted with a device.
mounted, err := d.isMounted(source, target)
if err != nil {
return status.Errorf(codes.Internal, "Could not check if %q is mounted: %v", target, err)
@@ -799,7 +798,7 @@ func (d *NodeService) getVolumesLimit() int64 {
availableAttachments = availableAttachments - enis - reservedSlots
}
}
availableAttachments = availableAttachments - reservedVolumeAttachments
availableAttachments -= reservedVolumeAttachments
if availableAttachments <= 0 {
availableAttachments = 1
}
11 changes: 6 additions & 5 deletions pkg/driver/options.go
Original file line number Diff line number Diff line change
@@ -34,7 +34,7 @@ type Options struct {

// #### Server options ####

//Endpoint is the endpoint for the CSI driver server
// Endpoint is the endpoint for the CSI driver server
Endpoint string
// HttpEndpoint is the TCP network address where the HTTP server for metrics will listen
HttpEndpoint string
@@ -52,7 +52,7 @@ type Options struct {
ExtraTags map[string]string
// ExtraVolumeTags is a map of tags that will be attached to each dynamically provisioned
// volume.
// DEPRECATED: Use ExtraTags instead.
// Deprecated: Use ExtraTags instead.
ExtraVolumeTags map[string]string
// ID of the kubernetes cluster.
KubernetesClusterID string
@@ -129,11 +129,12 @@ func (o *Options) Validate() error {
}

if o.MetricsCertFile != "" || o.MetricsKeyFile != "" {
if o.HttpEndpoint == "" {
switch {
case o.HttpEndpoint == "":
return fmt.Errorf("--http-endpoint MUST be specififed when using the metrics server with HTTPS")
} else if o.MetricsCertFile == "" {
case o.MetricsCertFile == "":
return fmt.Errorf("--metrics-cert-file MUST be specififed when using the metrics server with HTTPS")
} else if o.MetricsKeyFile == "" {
case o.MetricsKeyFile == "":
return fmt.Errorf("--metrics-key-file MUST be specififed when using the metrics server with HTTPS")
}
}
7 changes: 4 additions & 3 deletions pkg/driver/request_coalescing_test.go
Original file line number Diff line number Diff line change
@@ -287,21 +287,22 @@ func testPartialFail(t *testing.T, executor modifyVolumeExecutor) {

wg.Wait()

if volumeTypeChosen == NewVolumeType1 {
switch volumeTypeChosen {
case NewVolumeType1:
if volumeType1Err {
t.Error("Controller chose", NewVolumeType1, "but errored request")
}
if !volumeType2Error {
t.Error("Controller chose", NewVolumeType1, "but returned success to", NewVolumeType2, "request")
}
} else if volumeTypeChosen == NewVolumeType2 {
case NewVolumeType2:
if volumeType2Error {
t.Error("Controller chose", NewVolumeType2, "but errored request")
}
if !volumeType1Err {
t.Error("Controller chose", NewVolumeType2, "but returned success to", NewVolumeType1, "request")
}
} else {
default:
t.Error("No volume type chosen")
}
}
2 changes: 1 addition & 1 deletion pkg/util/util.go
Original file line number Diff line number Diff line change
@@ -147,7 +147,7 @@ func CountMACAddresses(s string) int {

// NormalizeWindowsPath normalizes a Windows path
func NormalizeWindowsPath(path string) string {
normalizedPath := strings.Replace(path, "/", "\\", -1)
normalizedPath := strings.ReplaceAll(path, "/", "\\")
if strings.HasPrefix(normalizedPath, "\\") {
normalizedPath = "c:" + normalizedPath
}
6 changes: 3 additions & 3 deletions tests/e2e/testsuites/testsuites.go
Original file line number Diff line number Diff line change
@@ -601,16 +601,16 @@ func (t *TestDeployment) Logs() ([]byte, error) {
}

// waitForPersistentVolumeClaimDeleted waits for a PersistentVolumeClaim to be removed from the system until timeout occurs, whichever comes first.
func waitForPersistentVolumeClaimDeleted(c clientset.Interface, ns string, pvcName string, Poll, timeout time.Duration) error {
func waitForPersistentVolumeClaimDeleted(c clientset.Interface, ns string, pvcName string, poll, timeout time.Duration) error {
framework.Logf("Waiting up to %v for PersistentVolumeClaim %s to be removed", timeout, pvcName)
for start := time.Now(); time.Since(start) < timeout; time.Sleep(Poll) {
for start := time.Now(); time.Since(start) < timeout; time.Sleep(poll) {
_, err := c.CoreV1().PersistentVolumeClaims(ns).Get(context.Background(), pvcName, metav1.GetOptions{})
if err != nil {
if apierrs.IsNotFound(err) {
framework.Logf("Claim %q in namespace %q doesn't exist in the system", pvcName, ns)
return nil
}
framework.Logf("Failed to get claim %q in namespace %q, retrying in %v. Error: %v", pvcName, ns, Poll, err)
framework.Logf("Failed to get claim %q in namespace %q, retrying in %v. Error: %v", pvcName, ns, poll, err)
}
}
return fmt.Errorf("PersistentVolumeClaim %s is not removed from the system within %v", pvcName, timeout)