Skip to content

Commit 6ab3af5

Browse files
castlemilkcastlemilk
committed
fix lints
1 parent 8ccbfe2 commit 6ab3af5

8 files changed

Lines changed: 109 additions & 858 deletions

File tree

‎pkg/cloudprovider/aws/aws_retry_test.go‎

Lines changed: 12 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import (
66
"time"
77

88
"github.com/aws/aws-sdk-go/aws/awserr"
9-
"github.com/go-logr/logr"
9+
"github.com/go-logr/logr/testr"
1010
)
1111

1212
// TestRetryOnTransientError_IOTimeout reproduces the original issue
@@ -26,7 +26,7 @@ func TestRetryOnTransientError_IOTimeout(t *testing.T) {
2626
return nil
2727
}
2828

29-
logger := &testLogger{t: t}
29+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
3030
err := retryOnTransientError(fn, logger)
3131

3232
if err != nil {
@@ -56,7 +56,7 @@ func TestRetryOnTransientError_AWSThrottling(t *testing.T) {
5656
return nil
5757
}
5858

59-
logger := &testLogger{t: t}
59+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
6060
err := retryOnTransientError(fn, logger)
6161

6262
if err != nil {
@@ -85,7 +85,7 @@ func TestRetryOnTransientError_AWSServiceUnavailable(t *testing.T) {
8585
return nil
8686
}
8787

88-
logger := &testLogger{t: t}
88+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
8989
err := retryOnTransientError(fn, logger)
9090

9191
if err != nil {
@@ -113,7 +113,7 @@ func TestRetryOnTransientError_PermanentError(t *testing.T) {
113113
return permanentErr
114114
}
115115

116-
logger := &testLogger{t: t}
116+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
117117
err := retryOnTransientError(fn, logger)
118118

119119
if err != permanentErr {
@@ -144,7 +144,7 @@ func TestRetryOnTransientError_BackoffTiming(t *testing.T) {
144144
return nil
145145
}
146146

147-
logger := &testLogger{t: t}
147+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
148148
startTime := time.Now()
149149
err := retryOnTransientError(fn, logger)
150150
totalDuration := time.Since(startTime)
@@ -180,7 +180,7 @@ func TestRetryOnTransientError_ExhaustsRetries(t *testing.T) {
180180
return persistentErr
181181
}
182182

183-
logger := &testLogger{t: t}
183+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
184184
err := retryOnTransientError(fn, logger)
185185

186186
if err != persistentErr {
@@ -210,7 +210,7 @@ func TestRetryOnTransientError_ConnectionReset(t *testing.T) {
210210
return nil
211211
}
212212

213-
logger := &testLogger{t: t}
213+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
214214
err := retryOnTransientError(fn, logger)
215215

216216
if err != nil {
@@ -239,7 +239,7 @@ func TestRetryOnTransientError_TLSHandshakeTimeout(t *testing.T) {
239239
return nil
240240
}
241241

242-
logger := &testLogger{t: t}
242+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
243243
err := retryOnTransientError(fn, logger)
244244

245245
if err != nil {
@@ -272,7 +272,7 @@ func TestRetryOnTransientError_MultipleTransientErrors(t *testing.T) {
272272
return nil
273273
}
274274

275-
logger := &testLogger{t: t}
275+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
276276
err := retryOnTransientError(fn, logger)
277277

278278
if err != nil {
@@ -288,31 +288,5 @@ func TestRetryOnTransientError_MultipleTransientErrors(t *testing.T) {
288288
})
289289
}
290290

291-
// testLogger implements logr.Logger for testing
292-
type testLogger struct {
293-
t *testing.T
294-
}
295-
296-
func (l *testLogger) Info(msg string, keysAndValues ...interface{}) {
297-
l.t.Logf("[INFO] %s %v", msg, keysAndValues)
298-
}
299-
300-
func (l *testLogger) Enabled() bool {
301-
return true
302-
}
303-
304-
func (l *testLogger) Error(err error, msg string, keysAndValues ...interface{}) {
305-
l.t.Logf("[ERROR] %s: %v %v", msg, err, keysAndValues)
306-
}
307-
308-
func (l *testLogger) V(level int) logr.Logger {
309-
return l
310-
}
311-
312-
func (l *testLogger) WithValues(keysAndValues ...interface{}) logr.Logger {
313-
return l
314-
}
315-
316-
func (l *testLogger) WithName(name string) logr.Logger {
317-
return l
318-
}
291+
// Using testr.NewWithOptions from go-logr/logr/testr for testing
292+
// This provides a proper logr.Logger implementation that logs to *testing.T

‎pkg/cloudprovider/aws/errors.go‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,10 +30,9 @@ func isNetworkError(err error) bool {
3030
return false
3131
}
3232

33-
// Check for net.Error (includes timeouts, temporary errors, etc.)
33+
// Check for net.Error (includes timeouts)
3434
if netErr, ok := err.(net.Error); ok {
35-
// Timeouts and temporary errors should be retried
36-
if netErr.Timeout() || netErr.Temporary() {
35+
if netErr.Timeout() {
3736
return true
3837
}
3938
}

‎pkg/cloudprovider/aws/errors_test.go‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,10 @@ package aws
22

33
import (
44
"errors"
5-
"net"
65
"testing"
7-
"time"
86

97
"github.com/aws/aws-sdk-go/aws/awserr"
8+
"github.com/go-logr/logr/testr"
109
)
1110

1211
// mockNetError is a mock implementation of net.Error for testing
@@ -37,9 +36,9 @@ func TestIsTransientError(t *testing.T) {
3736
expected: true,
3837
},
3938
{
40-
name: "temporary error",
39+
name: "temporary error (deprecated, no longer detected)",
4140
err: &mockNetError{temporary: true, msg: "temporary"},
42-
expected: true,
41+
expected: false,
4342
},
4443
{
4544
name: "i/o timeout string",
@@ -96,7 +95,8 @@ func TestRetryOnTransientError(t *testing.T) {
9695
return nil
9796
}
9897

99-
err := retryOnTransientError(fn, nil)
98+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
99+
err := retryOnTransientError(fn, logger)
100100
if err != nil {
101101
t.Errorf("expected no error, got %v", err)
102102
}
@@ -115,7 +115,8 @@ func TestRetryOnTransientError(t *testing.T) {
115115
return nil
116116
}
117117

118-
err := retryOnTransientError(fn, nil)
118+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
119+
err := retryOnTransientError(fn, logger)
119120
if err != nil {
120121
t.Errorf("expected no error, got %v", err)
121122
}
@@ -132,7 +133,8 @@ func TestRetryOnTransientError(t *testing.T) {
132133
return nonTransientErr
133134
}
134135

135-
err := retryOnTransientError(fn, nil)
136+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
137+
err := retryOnTransientError(fn, logger)
136138
if err != nonTransientErr {
137139
t.Errorf("expected non-transient error, got %v", err)
138140
}
@@ -149,7 +151,8 @@ func TestRetryOnTransientError(t *testing.T) {
149151
return transientErr
150152
}
151153

152-
err := retryOnTransientError(fn, nil)
154+
logger := testr.NewWithOptions(t, testr.Options{Verbosity: 10})
155+
err := retryOnTransientError(fn, logger)
153156
if err != transientErr {
154157
t.Errorf("expected transient error after exhausting retries, got %v", err)
155158
}
@@ -176,9 +179,9 @@ func TestIsNetworkError(t *testing.T) {
176179
expected: true,
177180
},
178181
{
179-
name: "temporary error",
182+
name: "temporary error (deprecated, no longer detected)",
180183
err: &mockNetError{temporary: true, msg: "temp"},
181-
expected: true,
184+
expected: false,
182185
},
183186
{
184187
name: "dial tcp error",

‎pkg/controller/cyclenoderequest/transitioner/checks.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ func (t *CycleNodeRequestTransitioner) makeRequest(httpMethod string, httpClient
139139
return 0, nil, err
140140
}
141141

142-
defer resp.Body.Close()
142+
defer func() { _ = resp.Body.Close() }()
143143

144144
bytes, err := io.ReadAll(resp.Body)
145145
if err != nil {
@@ -242,7 +242,7 @@ func (t *CycleNodeRequestTransitioner) performInitialHealthChecks(kubeNodes map[
242242
// performCyclingHealthChecks before terminating an instance selected for termination. Cycling pauses
243243
// until all health checks pass for the new instance before terminating the old one
244244
func (t *CycleNodeRequestTransitioner) performCyclingHealthChecks(kubeNodes map[string]corev1.Node) (bool, error) {
245-
var allHealthChecksPassed bool = true
245+
allHealthChecksPassed := true
246246

247247
// Find new instsances attached to the nodegroup and perform health checks on them
248248
// before terminating the old ones they are replacing
@@ -376,7 +376,7 @@ func (t *CycleNodeRequestTransitioner) sendPreTerminationTrigger(node v1.CycleNo
376376
// It monitors the progress shutdown progress. Cyclops will wait until this endpoint returns the expected response before
377377
// proceeding to terminate the node.
378378
func (t *CycleNodeRequestTransitioner) performPreTerminationHealthChecks(node v1.CycleNodeRequestNode) (bool, error) {
379-
var allHealthChecksPassed bool = true
379+
allHealthChecksPassed := true
380380
nodeHash := getNodeHash(node)
381381

382382
// Check that the trigger has already been send to the node before performing any health checks

‎pkg/controller/cyclenoderequest/transitioner/errors.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ func isRetryableError(err error) bool {
1616

1717
// Check for network errors (timeouts, connection refused, etc.)
1818
if netErr, ok := err.(net.Error); ok {
19-
if netErr.Timeout() || netErr.Temporary() {
19+
if netErr.Timeout() {
2020
return true
2121
}
2222
}

‎pkg/controller/cyclenoderequest/transitioner/integration_test.go‎

Lines changed: 78 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,14 @@ package transitioner
22

33
import (
44
"errors"
5-
"fmt"
65
"testing"
76
"time"
87

98
v1 "github.com/atlassian-labs/cyclops/pkg/apis/atlassian/v1"
109
"github.com/atlassian-labs/cyclops/pkg/cloudprovider"
1110
"github.com/atlassian-labs/cyclops/pkg/controller"
1211
"github.com/aws/aws-sdk-go/aws/awserr"
12+
"github.com/go-logr/logr/testr"
1313
corev1 "k8s.io/api/core/v1"
1414
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1515
"k8s.io/apimachinery/pkg/runtime"
@@ -237,7 +237,7 @@ func TestEndToEnd_EquilibriumTimeout(t *testing.T) {
237237
node := createTestNode("test-node-1", "aws:///us-west-2a/i-1234567890abcdef0")
238238

239239
// Set equilibrium wait to past the timeout
240-
cnr.Status.EquilibriumWaitStarted = metav1.Time{
240+
cnr.Status.EquilibriumWaitStarted = &metav1.Time{
241241
Time: time.Now().Add(-10 * time.Minute), // Past the 5 minute limit
242242
}
243243

@@ -331,7 +331,7 @@ func createTestCycleNodeRequest(name string, phase v1.CycleNodeRequestPhase) *v1
331331
},
332332
Status: v1.CycleNodeRequestStatus{
333333
Phase: phase,
334-
EquilibriumWaitStarted: metav1.Time{
334+
EquilibriumWaitStarted: &metav1.Time{
335335
Time: time.Now(),
336336
},
337337
},
@@ -354,7 +354,7 @@ func createTestNode(name, providerID string) *corev1.Node {
354354

355355
func createTestResourceManager(t *testing.T, cnr *v1.CycleNodeRequest, node *corev1.Node, cp cloudprovider.CloudProvider) *controller.ResourceManager {
356356
scheme := runtime.NewScheme()
357-
_ = v1.AddToScheme(scheme)
357+
_ = v1.SchemeBuilder.AddToScheme(scheme)
358358
_ = corev1.AddToScheme(scheme)
359359
fakeClient := fake.NewClientBuilder().
360360
WithScheme(scheme).
@@ -364,7 +364,7 @@ func createTestResourceManager(t *testing.T, cnr *v1.CycleNodeRequest, node *cor
364364
return &controller.ResourceManager{
365365
Client: fakeClient,
366366
CloudProvider: cp,
367-
Logger: &testLogger{t: t},
367+
Logger: testr.NewWithOptions(t, testr.Options{Verbosity: 10}),
368368
}
369369
}
370370

@@ -385,18 +385,83 @@ func (m *mockCloudProviderAlwaysFails) TerminateInstance(providerID string) erro
385385
return m.error
386386
}
387387

388-
func (m *mockCloudProviderAlwaysFails) DetachInstance(nodeGroupName, providerID string) error {
389-
return m.error
388+
func (m *mockCloudProviderAlwaysFails) Name() string {
389+
return "mock-aws-failing"
390390
}
391391

392-
func (m *mockCloudProviderAlwaysFails) AttachInstance(nodeGroupName, providerID string) error {
393-
return m.error
392+
// mockCloudProviderWithTransientErrors simulates AWS transient failures
393+
type mockCloudProviderWithTransientErrors struct {
394+
callCount int
395+
failuresBeforeSuccess int
396+
errorToReturn error
394397
}
395398

396-
func (m *mockCloudProviderAlwaysFails) AddInstanceToNodeGroup(nodeGroupName string, nodeGroup cloudprovider.NodeGroupOptions) error {
397-
return m.error
399+
func (m *mockCloudProviderWithTransientErrors) GetNodeGroups(names []string) (cloudprovider.NodeGroups, error) {
400+
m.callCount++
401+
if m.callCount <= m.failuresBeforeSuccess {
402+
return nil, m.errorToReturn
403+
}
404+
// Success after N failures
405+
return &mockNodeGroups{}, nil
398406
}
399407

400-
func (m *mockCloudProviderAlwaysFails) Name() string {
401-
return "mock-aws-failing"
408+
func (m *mockCloudProviderWithTransientErrors) InstancesExist(providerIDs []string) (map[string]interface{}, error) {
409+
return make(map[string]interface{}), nil
410+
}
411+
412+
func (m *mockCloudProviderWithTransientErrors) TerminateInstance(providerID string) error {
413+
return nil
414+
}
415+
416+
func (m *mockCloudProviderWithTransientErrors) Name() string {
417+
return "mock-aws"
418+
}
419+
420+
// mockNodeGroups is a simple mock implementation
421+
type mockNodeGroups struct{}
422+
423+
func (m *mockNodeGroups) Instances() map[string]cloudprovider.Instance {
424+
return map[string]cloudprovider.Instance{
425+
"aws:///us-west-2a/i-1234567890abcdef0": &mockInstance{
426+
id: "i-1234567890abcdef0",
427+
providerID: "aws:///us-west-2a/i-1234567890abcdef0",
428+
},
429+
}
430+
}
431+
432+
func (m *mockNodeGroups) DetachInstance(providerID string) (bool, error) {
433+
return false, nil
434+
}
435+
436+
func (m *mockNodeGroups) AttachInstance(providerID, nodeGroup string) (bool, error) {
437+
return false, nil
438+
}
439+
440+
func (m *mockNodeGroups) ReadyInstances() map[string]cloudprovider.Instance {
441+
return m.Instances()
442+
}
443+
444+
func (m *mockNodeGroups) NotReadyInstances() map[string]cloudprovider.Instance {
445+
return make(map[string]cloudprovider.Instance)
446+
}
447+
448+
type mockInstance struct {
449+
id string
450+
providerID string
451+
}
452+
453+
func (m *mockInstance) ID() string {
454+
return m.id
455+
}
456+
457+
func (m *mockInstance) OutOfDate() bool {
458+
return false
459+
}
460+
461+
func (m *mockInstance) MatchesProviderID(providerID string) bool {
462+
return m.providerID == providerID
463+
}
464+
465+
func (m *mockInstance) NodeGroupName() string {
466+
return "test-nodegroup"
402467
}

0 commit comments

Comments
 (0)