Skip to content

Commit 05f4ded

Browse files
nexus49nexus49
committed
refactor: addressing some code smell warnings
1 parent fcc3f7c commit 05f4ded

File tree

7 files changed

+33
-40
lines changed

7 files changed

+33
-40
lines changed

‎controller/lifecycle/controllerruntime/lifecycle_test.go‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -388,7 +388,7 @@ func TestLifecycle(t *testing.T) {
388388

389389
fakeClient := testSupport.CreateFakeClient(t, instance)
390390

391-
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: false}}, fakeClient)
391+
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{RequeAfter: false}}, fakeClient)
392392
mgr.WithSpreadingReconciles()
393393

394394
// Act
@@ -473,13 +473,13 @@ func TestLifecycle(t *testing.T) {
473473

474474
fakeClient := testSupport.CreateFakeClient(t, instance)
475475

476-
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{Retry: true, RequeAfter: false}}, fakeClient)
476+
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{RequeAfter: true}}, fakeClient)
477477
mgr.WithSpreadingReconciles()
478478

479479
// Act
480480
_, err := mgr.Reconcile(ctx, request, instance)
481481

482-
assert.Error(t, err)
482+
assert.NoError(t, err)
483483
assert.Equal(t, int64(0), instance.Status.ObservedGeneration)
484484
})
485485

@@ -500,7 +500,7 @@ func TestLifecycle(t *testing.T) {
500500
}
501501
fakeClient := testSupport.CreateFakeClient(t, instance)
502502

503-
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: true}}, fakeClient)
503+
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{RequeAfter: true}}, fakeClient)
504504
mgr.WithSpreadingReconciles()
505505

506506
// Act
@@ -550,7 +550,7 @@ func TestLifecycle(t *testing.T) {
550550
})
551551
assert.NoError(t, err)
552552

553-
lm, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: true}}, fakeClient)
553+
lm, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{RequeAfter: true}}, fakeClient)
554554
tr := &testReconciler{
555555
lifecycleManager: lm,
556556
}
@@ -573,7 +573,7 @@ func TestLifecycle(t *testing.T) {
573573
})
574574
assert.NoError(t, err)
575575

576-
lm, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: true}}, fakeClient)
576+
lm, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{RequeAfter: true}}, fakeClient)
577577
lm.WithSpreadingReconciles()
578578
tr := &testReconciler{
579579
lifecycleManager: lm,
@@ -594,7 +594,7 @@ func TestLifecycle(t *testing.T) {
594594
Name: name,
595595
Namespace: namespace,
596596
Generation: 1,
597-
Labels: map[string]string{spread.SpreadReconcileRefreshLabel: "true"},
597+
Labels: map[string]string{spread.ReconcileRefreshLabel: "true"},
598598
},
599599
Status: testSupport.TestStatus{
600600
Some: "string",
@@ -622,7 +622,7 @@ func TestLifecycle(t *testing.T) {
622622
err = fakeClient.Get(ctx, client.ObjectKey{Name: name, Namespace: namespace}, serverObject)
623623
assert.NoError(t, err)
624624
assert.Equal(t, serverObject.Status.Some, "other string")
625-
_, ok := serverObject.Labels[spread.SpreadReconcileRefreshLabel]
625+
_, ok := serverObject.Labels[spread.ReconcileRefreshLabel]
626626
assert.False(t, ok)
627627
})
628628

@@ -975,7 +975,7 @@ func TestLifecycle(t *testing.T) {
975975
fakeClient := testSupport.CreateFakeClient(t, instance)
976976

977977
mgr, _ := createLifecycleManager([]subroutine.Subroutine{
978-
pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: true}}, fakeClient)
978+
pmtesting.FailureScenarioSubroutine{RequeAfter: true}}, fakeClient)
979979
mgr.WithConditionManagement()
980980

981981
// Act
@@ -1007,7 +1007,7 @@ func TestLifecycle(t *testing.T) {
10071007
fakeClient := testSupport.CreateFakeClient(t, instance)
10081008

10091009
mgr, _ := createLifecycleManager([]subroutine.Subroutine{
1010-
pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: false}}, fakeClient)
1010+
pmtesting.FailureScenarioSubroutine{RequeAfter: false}}, fakeClient)
10111011
mgr.WithConditionManagement()
10121012

10131013
// Act
@@ -1167,7 +1167,7 @@ func TestLifecycle(t *testing.T) {
11671167

11681168
fakeClient := testSupport.CreateFakeClient(t, instance)
11691169

1170-
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{Retry: false, RequeAfter: false}}, fakeClient)
1170+
mgr, _ := createLifecycleManager([]subroutine.Subroutine{pmtesting.FailureScenarioSubroutine{RequeAfter: false}}, fakeClient)
11711171
mgr.WithSpreadingReconciles()
11721172
mgr.WithConditionManagement()
11731173

‎controller/lifecycle/lifecycle.go‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -57,24 +57,24 @@ func Reconcile(ctx context.Context, req ctrl.Request, instance runtimeobject.Run
5757
ctx = sentry.ContextWithSentryTags(ctx, sentryTags)
5858

5959
log.Info().Msg("start reconcile")
60-
generationChanged := true
6160
err := cl.Get(ctx, req.NamespacedName, instance)
6261
if err != nil {
6362
if kerrors.IsNotFound(err) {
6463
log.Info().Msg("instance not found. It was likely deleted")
6564
return ctrl.Result{}, nil
6665
}
67-
return HandleClientError("failed to retrieve instance", log, err, generationChanged, sentryTags)
66+
return HandleClientError("failed to retrieve instance", log, err, true, sentryTags)
6867
}
6968

7069
originalCopy := instance.DeepCopyObject()
7170
inDeletion := instance.GetDeletionTimestamp() != nil
71+
generationChanged := true
7272

7373
if l.Spreader() != nil && instance.GetDeletionTimestamp().IsZero() {
7474
instanceStatusObj := l.Spreader().MustToRuntimeObjectSpreadReconcileStatusInterface(instance, log)
7575
generationChanged = instance.GetGeneration() != instanceStatusObj.GetObservedGeneration()
7676
isAfterNextReconcileTime := v1.Now().UTC().After(instanceStatusObj.GetNextReconcileTime().UTC())
77-
refreshRequested := slices.Contains(maps.Keys(instance.GetLabels()), spread.SpreadReconcileRefreshLabel)
77+
refreshRequested := slices.Contains(maps.Keys(instance.GetLabels()), spread.ReconcileRefreshLabel)
7878

7979
reconcileRequired := generationChanged || isAfterNextReconcileTime || refreshRequested
8080
if !reconcileRequired {
@@ -111,23 +111,23 @@ func Reconcile(ctx context.Context, req ctrl.Request, instance runtimeobject.Run
111111
}
112112

113113
// Continue with reconciliation
114-
for _, subroutine := range subroutines {
114+
for _, s := range subroutines {
115115
if l.ConditionsManager() != nil {
116-
l.ConditionsManager().SetSubroutineConditionToUnknownIfNotSet(&condArr, subroutine, inDeletion, log)
116+
l.ConditionsManager().SetSubroutineConditionToUnknownIfNotSet(&condArr, s, inDeletion, log)
117117
}
118118

119-
// Set current condArr before reconciling the subroutine
119+
// Set current condArr before reconciling the s
120120
if l.ConditionsManager() != nil {
121121
l.ConditionsManager().MustToRuntimeObjectConditionsInterface(instance, log).SetConditions(condArr)
122122
}
123-
subResult, retry, err := reconcileSubroutine(ctx, instance, subroutine, cl, l, log, generationChanged, sentryTags)
124-
// Update condArr with any changes the subroutine did
123+
subResult, retry, err := reconcileSubroutine(ctx, instance, s, cl, l, log, generationChanged, sentryTags)
124+
// Update condArr with any changes the s did
125125
if l.ConditionsManager() != nil {
126126
condArr = l.ConditionsManager().MustToRuntimeObjectConditionsInterface(instance, log).GetConditions()
127127
}
128128
if err != nil {
129129
if l.ConditionsManager() != nil {
130-
l.ConditionsManager().SetSubroutineCondition(&condArr, subroutine, result, err, inDeletion, log)
130+
l.ConditionsManager().SetSubroutineCondition(&condArr, s, result, err, inDeletion, log)
131131
l.ConditionsManager().SetInstanceConditionReady(&condArr, v1.ConditionFalse)
132132
l.ConditionsManager().MustToRuntimeObjectConditionsInterface(instance, log).SetConditions(condArr)
133133
}
@@ -149,7 +149,7 @@ func Reconcile(ctx context.Context, req ctrl.Request, instance runtimeobject.Run
149149
}
150150
if l.ConditionsManager() != nil {
151151
if subResult.RequeueAfter == 0 {
152-
l.ConditionsManager().SetSubroutineCondition(&condArr, subroutine, subResult, err, inDeletion, log)
152+
l.ConditionsManager().SetSubroutineCondition(&condArr, s, subResult, err, inDeletion, log)
153153
}
154154
}
155155
}
@@ -340,9 +340,9 @@ func AddFinalizersIfNeeded(ctx context.Context, cl client.Client, instance runti
340340

341341
update := false
342342
original := instance.DeepCopyObject().(client.Object)
343-
for _, subroutine := range subroutines {
344-
if len(subroutine.Finalizers()) > 0 {
345-
needsUpdate := AddFinalizerIfNeeded(instance, subroutine)
343+
for _, s := range subroutines {
344+
if len(s.Finalizers()) > 0 {
345+
needsUpdate := AddFinalizerIfNeeded(instance, s)
346346
if needsUpdate {
347347
update = true
348348
}

‎controller/lifecycle/spread/spread.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import (
1313
"github.com/platform-mesh/golang-commons/sentry"
1414
)
1515

16-
const SpreadReconcileRefreshLabel = "platform-mesh.io/refresh-reconcile"
16+
const ReconcileRefreshLabel = "platform-mesh.io/refresh-reconcile"
1717

1818
type Spreader struct {
1919
}
@@ -52,7 +52,7 @@ func (s *Spreader) OnNextReconcile(instanceStatusObj RuntimeObjectSpreadReconcil
5252
return ctrl.Result{RequeueAfter: requeueAfter}, nil
5353
}
5454

55-
// setNextReconcileTime calculates and sets the next reconcile time for the instance
55+
// SetNextReconcileTime calculates and sets the next reconcile time for the instance
5656
func (s *Spreader) SetNextReconcileTime(instanceStatusObj RuntimeObjectSpreadReconcileStatus, log *logger.Logger) {
5757

5858
var border = defaultMaxReconcileDuration
@@ -66,14 +66,14 @@ func (s *Spreader) SetNextReconcileTime(instanceStatusObj RuntimeObjectSpreadRec
6666
instanceStatusObj.SetNextReconcileTime(v1.NewTime(time.Now().Add(nextReconcileTime)))
6767
}
6868

69-
// updateObservedGeneration updates the observed generation of the instance struct
69+
// UpdateObservedGeneration updates the observed generation of the instance struct
7070
func (s *Spreader) UpdateObservedGeneration(instanceStatusObj RuntimeObjectSpreadReconcileStatus, log *logger.Logger) {
7171
log.Debug().Int64("observed-generation", instanceStatusObj.GetObservedGeneration()).Int64("generation", instanceStatusObj.GetGeneration()).Msg("Updating observed generation")
7272
instanceStatusObj.SetObservedGeneration(instanceStatusObj.GetGeneration())
7373
}
7474
func (s *Spreader) RemoveRefreshLabelIfExists(instance runtimeobject.RuntimeObject) bool {
7575
keyCount := len(instance.GetLabels())
76-
delete(instance.GetLabels(), SpreadReconcileRefreshLabel)
76+
delete(instance.GetLabels(), ReconcileRefreshLabel)
7777
return keyCount != len(instance.GetLabels())
7878
}
7979

‎controller/lifecycle/spread/spread_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -102,25 +102,25 @@ func TestRemoveRefreshLabel(t *testing.T) {
102102
s := NewSpreader()
103103
apiObject := &testSupport.TestApiObject{
104104
ObjectMeta: v1.ObjectMeta{
105-
Labels: map[string]string{SpreadReconcileRefreshLabel: ""},
105+
Labels: map[string]string{ReconcileRefreshLabel: ""},
106106
},
107107
}
108108
s.RemoveRefreshLabelIfExists(apiObject)
109109

110-
_, ok := apiObject.GetLabels()[SpreadReconcileRefreshLabel]
110+
_, ok := apiObject.GetLabels()[ReconcileRefreshLabel]
111111
assert.False(t, ok)
112112
}
113113

114114
func TestRemoveRefreshLabelFilledWithValue(t *testing.T) {
115115
s := NewSpreader()
116116
apiObject := &testSupport.TestApiObject{
117117
ObjectMeta: v1.ObjectMeta{
118-
Labels: map[string]string{SpreadReconcileRefreshLabel: "true"},
118+
Labels: map[string]string{ReconcileRefreshLabel: "true"},
119119
},
120120
}
121121
s.RemoveRefreshLabelIfExists(apiObject)
122122

123-
_, ok := apiObject.GetLabels()[SpreadReconcileRefreshLabel]
123+
_, ok := apiObject.GetLabels()[ReconcileRefreshLabel]
124124

125125
assert.False(t, ok)
126126
}
@@ -132,7 +132,7 @@ func TestRemoveRefreshLabelNoLabels(t *testing.T) {
132132
}
133133
s.RemoveRefreshLabelIfExists(apiObject)
134134

135-
_, ok := apiObject.GetLabels()[SpreadReconcileRefreshLabel]
135+
_, ok := apiObject.GetLabels()[ReconcileRefreshLabel]
136136

137137
assert.False(t, ok)
138138
}

‎controller/lifecycle/testing/testSupport.go‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,10 +163,6 @@ func (f FailureScenarioSubroutine) Process(_ context.Context, _ runtimeobject.Ru
163163
}
164164

165165
func (f FailureScenarioSubroutine) Finalize(_ context.Context, _ runtimeobject.RuntimeObject) (controllerruntime.Result, errors.OperatorError) {
166-
if f.Retry {
167-
return controllerruntime.Result{Requeue: true}, nil
168-
}
169-
170166
if f.RequeAfter {
171167
return controllerruntime.Result{RequeueAfter: 10 * time.Second}, nil
172168
}

‎go.mod‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@ require (
3838
k8s.io/client-go v0.33.2
3939
k8s.io/utils v0.0.0-20250604170112-4c0f3b243397
4040
sigs.k8s.io/controller-runtime v0.21.0
41-
sigs.k8s.io/multicluster-runtime v0.21.0-alpha.8
4241
)
4342

4443
require (

‎go.sum‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -483,8 +483,6 @@ sigs.k8s.io/controller-runtime v0.21.0 h1:CYfjpEuicjUecRk+KAeyYh+ouUBn4llGyDYytI
483483
sigs.k8s.io/controller-runtime v0.21.0/go.mod h1:OSg14+F65eWqIu4DceX7k/+QRAbTTvxeQSNSOQpukWM=
484484
sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3 h1:/Rv+M11QRah1itp8VhT6HoVx1Ray9eB4DBr+K+/sCJ8=
485485
sigs.k8s.io/json v0.0.0-20241010143419-9aa6b5e7a4b3/go.mod h1:18nIHnGi6636UCz6m8i4DhaJ65T6EruyzmoQqI2BVDo=
486-
sigs.k8s.io/multicluster-runtime v0.21.0-alpha.8 h1:Pq69tTKfN8ADw8m8A3wUtP8wJ9SPQbbOsgapm3BZEPw=
487-
sigs.k8s.io/multicluster-runtime v0.21.0-alpha.8/go.mod h1:CpBzLMLQKdm+UCchd2FiGPiDdCxM5dgCCPKuaQ6Fsv0=
488486
sigs.k8s.io/randfill v0.0.0-20250304075658-069ef1bbf016/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY=
489487
sigs.k8s.io/randfill v1.0.0 h1:JfjMILfT8A6RbawdsK2JXGBR5AQVfd+9TbzrlneTyrU=
490488
sigs.k8s.io/randfill v1.0.0/go.mod h1:XeLlZ/jmk4i1HRopwe7/aU3H5n1zNUcX6TM94b3QxOY=

0 commit comments

Comments
 (0)