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.

Sign up for GitHub

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

Enable recommended golang-ci linters #2204

Show file tree
Hide file tree
Changes from all commits
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
414 changes: 51 additions & 363 deletions .golangci.yml
View file
Open in desktop

Large diffs are not rendered by default.

24 changes: 16 additions & 8 deletions cmd/hooks/prestop.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ package hooks

import (
"context"
"errors"
"fmt"
"os"

Expand Down Expand Up @@ -46,7 +47,7 @@ const clusterAutoscalerTaint = "ToBeDeletedByClusterAutoscaler"
const v1KarpenterTaint = "karpenter.sh/disrupted"
const v1beta1KarpenterTaint = "karpenter.sh/disruption"

// drainTaints includes taints used by K8s or autoscalers that signify node draining or pod eviction
// drainTaints includes taints used by K8s or autoscalers that signify node draining or pod eviction.
var drainTaints = map[string]struct{}{
v1.TaintNodeUnschedulable: {}, // Kubernetes common eviction taint (kubectl drain)
clusterAutoscalerTaint: {},
Expand All @@ -59,18 +60,19 @@ func PreStop(clientset kubernetes.Interface) error {

nodeName := os.Getenv("CSI_NODE_NAME")
if nodeName == "" {
return fmt.Errorf("PreStop: CSI_NODE_NAME missing")
return errors.New("PreStop: CSI_NODE_NAME missing")
AndrewSirenko marked this conversation as resolved.
Show resolved Hide resolved
}

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)
}

Expand Down Expand Up @@ -104,7 +106,10 @@ func waitForVolumeAttachments(clientset kubernetes.Interface, nodeName string) e
_, err := informer.AddEventHandler(cache.ResourceEventHandlerFuncs{
DeleteFunc: func(obj interface{}) {
klog.V(5).InfoS("DeleteFunc: VolumeAttachment deleted", "node", nodeName)
va := obj.(*storagev1.VolumeAttachment)
va, ok := obj.(*storagev1.VolumeAttachment)
if !ok {
klog.Error("DeleteFunc: error asserting object as type VolumeAttachment", "obj", va)
}
if va.Spec.NodeName == nodeName {
if err := checkVolumeAttachments(clientset, nodeName, allAttachmentsDeleted); err != nil {
klog.ErrorS(err, "DeleteFunc: error checking VolumeAttachments")
Expand All @@ -113,7 +118,10 @@ func waitForVolumeAttachments(clientset kubernetes.Interface, nodeName string) e
},
UpdateFunc: func(oldObj, newObj interface{}) {
klog.V(5).InfoS("UpdateFunc: VolumeAttachment updated", "node", nodeName)
va := newObj.(*storagev1.VolumeAttachment)
va, ok := newObj.(*storagev1.VolumeAttachment)
if !ok {
klog.Error("UpdateFunc: error asserting object as type VolumeAttachment", "obj", va)
}
if va.Spec.NodeName == nodeName {
if err := checkVolumeAttachments(clientset, nodeName, allAttachmentsDeleted); err != nil {
klog.ErrorS(err, "UpdateFunc: error checking VolumeAttachments")
Expand Down
14 changes: 4 additions & 10 deletions cmd/hooks/prestop_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
package hooks

import (
"fmt"
"errors"
"testing"

"github.com/golang/mock/gomock"
Expand All @@ -37,19 +37,19 @@ func TestPreStopHook(t *testing.T) {
{
name: "TestPreStopHook: CSI_NODE_NAME not set",
nodeName: "",
expErr: fmt.Errorf("PreStop: CSI_NODE_NAME missing"),
expErr: errors.New("PreStop: CSI_NODE_NAME missing"),
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockStorageV1 *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {
return nil
},
},
{
name: "TestPreStopHook: failed to retrieve node information",
nodeName: "test-node",
expErr: fmt.Errorf("fetchNode: failed to retrieve node information: non-existent node"),
expErr: errors.New("fetchNode: failed to retrieve node information: non-existent node"),
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockStorageV1 *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {
mockClient.EXPECT().CoreV1().Return(mockCoreV1).Times(1)
mockCoreV1.EXPECT().Nodes().Return(mockNode).Times(1)
mockNode.EXPECT().Get(gomock.Any(), gomock.Eq(nodeName), gomock.Any()).Return(nil, fmt.Errorf("non-existent node")).Times(1)
mockNode.EXPECT().Get(gomock.Any(), gomock.Eq(nodeName), gomock.Any()).Return(nil, errors.New("non-existent node")).Times(1)

return nil
},
Expand Down Expand Up @@ -77,7 +77,6 @@ func TestPreStopHook(t *testing.T) {
nodeName: "test-node",
expErr: nil,
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {

fakeNode := &v1.Node{
Spec: v1.NodeSpec{
Taints: []v1.Taint{
Expand Down Expand Up @@ -109,7 +108,6 @@ func TestPreStopHook(t *testing.T) {
nodeName: "test-node",
expErr: nil,
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {

fakeNode := &v1.Node{
Spec: v1.NodeSpec{
Taints: []v1.Taint{
Expand Down Expand Up @@ -149,7 +147,6 @@ func TestPreStopHook(t *testing.T) {
nodeName: "test-node",
expErr: nil,
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {

fakeNode := &v1.Node{
Spec: v1.NodeSpec{
Taints: []v1.Taint{
Expand Down Expand Up @@ -206,7 +203,6 @@ func TestPreStopHook(t *testing.T) {
nodeName: "test-karpenter-node",
expErr: nil,
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {

fakeNode := &v1.Node{
Spec: v1.NodeSpec{
Taints: []v1.Taint{
Expand Down Expand Up @@ -238,7 +234,6 @@ func TestPreStopHook(t *testing.T) {
nodeName: "test-karpenter-node",
expErr: nil,
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {

fakeNode := &v1.Node{
Spec: v1.NodeSpec{
Taints: []v1.Taint{
Expand Down Expand Up @@ -278,7 +273,6 @@ func TestPreStopHook(t *testing.T) {
nodeName: "test-karpenter-node",
expErr: nil,
mockFunc: func(nodeName string, mockClient *driver.MockKubernetesClient, mockCoreV1 *driver.MockCoreV1Interface, mockNode *driver.MockNodeInterface, mockVolumeAttachments *driver.MockVolumeAttachmentInterface, mockStorageV1Interface *driver.MockStorageV1Interface) error {

fakeNode := &v1.Node{
Spec: v1.NodeSpec{
Taints: []v1.Taint{
Expand Down
5 changes: 3 additions & 2 deletions cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ func main() {
klog.ErrorS(err, "failed to get version")
klog.FlushAndExit(klog.ExitFlushTimeout, 1)
}
//nolint:forbidigo // Print version info without klog/timestamp
fmt.Println(versionInfo)
os.Exit(0)
}
Expand All @@ -133,9 +134,9 @@ func main() {
}()
}

if options.HttpEndpoint != "" {
if options.HTTPEndpoint != "" {
r := metrics.InitializeRecorder()
r.InitializeMetricsHandler(options.HttpEndpoint, "/metrics", options.MetricsCertFile, options.MetricsKeyFile)
r.InitializeMetricsHandler(options.HTTPEndpoint, "/metrics", options.MetricsCertFile, options.MetricsKeyFile)
}

cfg := metadata.MetadataServiceConfig{
Expand Down
13 changes: 7 additions & 6 deletions pkg/batcher/batcher_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
package batcher

import (
"errors"
"fmt"
"sync"
"testing"
Expand All @@ -39,10 +40,11 @@ func mockExecutionWithError(inputs []string) (map[string]string, error) {
for _, input := range inputs {
results[input] = input
}
return results, fmt.Errorf("mock execution error")
return results, errors.New("mock execution error")
}

func TestBatcher(t *testing.T) {
t.Parallel()
type testCase struct {
name string
mockFunc func(inputs []string) (map[string]string, error)
Expand Down Expand Up @@ -127,7 +129,6 @@ func TestBatcher(t *testing.T) {
}

for _, tc := range tests {
tc := tc
t.Run(tc.name, func(t *testing.T) {
t.Parallel()

Expand All @@ -136,7 +137,7 @@ func TestBatcher(t *testing.T) {

var wg sync.WaitGroup

for i := 0; i < len(tc.tasks); i++ {
for i := range tc.tasks {
wg.Add(1)
go func(taskNum int) {
defer wg.Done()
Expand All @@ -148,7 +149,7 @@ func TestBatcher(t *testing.T) {

wg.Wait()

for i := 0; i < len(tc.tasks); i++ {
for i := range tc.tasks {
select {
case r := <-resultChans[i]:
task := fmt.Sprintf("task%d", i)
Expand All @@ -175,7 +176,7 @@ func TestBatcherConcurrentTaskAdditions(t *testing.T) {
b := New(numTasks, 1*time.Second, mockExecution)
resultChans := make([]chan BatchResult[string], numTasks)

for i := 0; i < numTasks; i++ {
for i := range numTasks {
wg.Add(1)
go func(taskNum int) {
defer wg.Done()
Expand All @@ -187,7 +188,7 @@ func TestBatcherConcurrentTaskAdditions(t *testing.T) {

wg.Wait()

for i := 0; i < numTasks; i++ {
for i := range numTasks {
r := <-resultChans[i]
task := fmt.Sprintf("task%d", i)
if r.Err != nil {
Expand Down
Loading