Skip to content

Commit 60ab6fd

Browse files
rashmigottipatirashmigottipati
committed
Split needsUpdate into two separate variables and update unit tests
Signed-off-by: Rashmi Gottipati <[email protected]>
1 parent ac093b0 commit 60ab6fd

File tree

3 files changed

+62
-42
lines changed

3 files changed

+62
-42
lines changed

‎pkg/finalizer/finalizer.go

Lines changed: 18 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@ import (
2424

2525
type finalizers map[string]Finalizer
2626

27+
// Result struct holds Updated and StatusUpdated fields
28+
type Result struct {
29+
Updated bool
30+
StatusUpdated bool
31+
}
32+
2733
// NewFinalizers returns the Finalizers interface
2834
func NewFinalizers() Finalizers {
2935
return finalizers{}
@@ -37,27 +43,31 @@ func (f finalizers) Register(key string, finalizer Finalizer) error {
3743
return nil
3844
}
3945

40-
func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) {
41-
var errList []error
42-
needsUpdate := false
46+
func (f finalizers) Finalize(ctx context.Context, obj client.Object) (Result, error) {
47+
var (
48+
res Result
49+
errList []error
50+
)
51+
res.Updated = false
4352
for key, finalizer := range f {
4453
if dt := obj.GetDeletionTimestamp(); dt.IsZero() && !controllerutil.ContainsFinalizer(obj, key) {
4554
controllerutil.AddFinalizer(obj, key)
46-
needsUpdate = true
55+
res.Updated = true
4756
} else if !dt.IsZero() && controllerutil.ContainsFinalizer(obj, key) {
48-
finalizerNeedsUpdate, err := finalizer.Finalize(ctx, obj)
57+
finalizerRes, err := finalizer.Finalize(ctx, obj)
4958
if err != nil {
5059
// Even when the finalizer fails, it may need to signal to update the primary
5160
// object (e.g. it may set a condition and need a status update).
52-
needsUpdate = needsUpdate || finalizerNeedsUpdate
61+
res.Updated = res.Updated || finalizerRes.Updated
62+
res.StatusUpdated = res.StatusUpdated || finalizerRes.StatusUpdated
5363
errList = append(errList, fmt.Errorf("finalizer %q failed: %v", key, err))
5464
} else {
5565
// If the finalizer succeeds, we remove the finalizer from the primary
5666
// object's metadata, so we know it will need an update.
57-
needsUpdate = true
67+
res.Updated = true
5868
controllerutil.RemoveFinalizer(obj, key)
5969
}
6070
}
6171
}
62-
return needsUpdate, utilerrors.NewAggregate(errList)
72+
return res, utilerrors.NewAggregate(errList)
6373
}

‎pkg/finalizer/finalizer_test.go

Lines changed: 43 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,12 @@ import (
1414
)
1515

1616
type mockFinalizer struct {
17-
needsUpdate bool
18-
err error
17+
result Result
18+
err error
1919
}
2020

21-
func (f mockFinalizer) Finalize(context.Context, client.Object) (needsUpdate bool, err error) {
22-
return f.needsUpdate, f.err
21+
func (f mockFinalizer) Finalize(context.Context, client.Object) (Result, error) {
22+
return f.result, f.err
2323
}
2424
func TestFinalizer(t *testing.T) {
2525
RegisterFailHandler(Fail)
@@ -56,24 +56,25 @@ var _ = Describe("TestFinalizer", func() {
5656

5757
})
5858
})
59+
5960
Describe("Finalize", func() {
60-
It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() {
61+
It("successfully finalizes and returns true for Updated when deletion timestamp is nil and finalizer does not exist", func() {
6162
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
6263
Expect(err).To(BeNil())
6364

6465
pod.DeletionTimestamp = nil
6566
pod.Finalizers = []string{}
6667

67-
needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
68+
result, err := finalizers.Finalize(context.TODO(), pod)
6869
Expect(err).To(BeNil())
69-
Expect(needsUpdate).To(BeTrue())
70+
Expect(result.Updated).To(BeTrue())
7071
// when deletion timestamp is nil and finalizer is not present, the registered finalizer would be added to the obj
7172
Expect(len(pod.Finalizers)).To(Equal(1))
7273
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer"))
7374

7475
})
7576

76-
It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() {
77+
It("successfully finalizes and returns true for Updated when deletion timestamp is not nil and the finalizer exists", func() {
7778
now := metav1.Now()
7879
pod.DeletionTimestamp = &now
7980

@@ -82,37 +83,37 @@ var _ = Describe("TestFinalizer", func() {
8283

8384
pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"}
8485

85-
needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
86+
result, err := finalizers.Finalize(context.TODO(), pod)
8687
Expect(err).To(BeNil())
87-
Expect(needsUpdate).To(BeTrue())
88+
Expect(result.Updated).To(BeTrue())
8889
// finalizer will be removed from the obj upon successful finalization
8990
Expect(len(pod.Finalizers)).To(Equal(0))
9091
})
9192

92-
It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() {
93+
It("should return no error and return false for Updated when deletion timestamp is nil and finalizer doesn't exist", func() {
9394
pod.DeletionTimestamp = nil
9495
pod.Finalizers = []string{}
9596

96-
needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
97+
result, err := finalizers.Finalize(context.TODO(), pod)
9798
Expect(err).To(BeNil())
98-
Expect(needsUpdate).To(BeFalse())
99+
Expect(result.Updated).To(BeFalse())
99100
Expect(len(pod.Finalizers)).To(Equal(0))
100101

101102
})
102103

103-
It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() {
104+
It("should return no error and return false for Updated when deletion timestamp is not nil and the finalizer doesn't exist", func() {
104105
now := metav1.Now()
105106
pod.DeletionTimestamp = &now
106107
pod.Finalizers = []string{}
107108

108-
needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
109+
result, err := finalizers.Finalize(context.TODO(), pod)
109110
Expect(err).To(BeNil())
110-
Expect(needsUpdate).To(BeFalse())
111+
Expect(result.Updated).To(BeFalse())
111112
Expect(len(pod.Finalizers)).To(Equal(0))
112113

113114
})
114115

115-
It("successfully finalizes multiple finalizers and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() {
116+
It("successfully finalizes multiple finalizers and returns true for Updated when deletion timestamp is not nil and the finalizer exists", func() {
116117
now := metav1.Now()
117118
pod.DeletionTimestamp = &now
118119

@@ -124,32 +125,35 @@ var _ = Describe("TestFinalizer", func() {
124125

125126
pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer", "finalizers.sigs.k8s.io/newtestfinalizer"}
126127

127-
needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
128+
result, err := finalizers.Finalize(context.TODO(), pod)
128129
Expect(err).To(BeNil())
129-
Expect(needsUpdate).To(BeTrue())
130+
Expect(result.Updated).To(BeTrue())
131+
Expect(result.StatusUpdated).To(BeFalse())
130132
Expect(len(pod.Finalizers)).To(Equal(0))
131133
})
132134

133-
It("should return needsUpdate as false and a non-nil error", func() {
135+
It("should return result as false and a non-nil error", func() {
134136
now := metav1.Now()
135137
pod.DeletionTimestamp = &now
136138
pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"}
137139

138-
f.needsUpdate = false
140+
f.result.Updated = false
141+
f.result.StatusUpdated = false
139142
f.err = fmt.Errorf("finalizer failed for %q", pod.Finalizers[0])
140143

141144
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
142145
Expect(err).To(BeNil())
143146

144-
needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
147+
result, err := finalizers.Finalize(context.TODO(), pod)
145148
Expect(err).ToNot(BeNil())
146149
Expect(err.Error()).To(ContainSubstring("finalizer failed"))
147-
Expect(needsUpdate).To(BeFalse())
150+
Expect(result.Updated).To(BeFalse())
151+
Expect(result.StatusUpdated).To(BeFalse())
148152
Expect(len(pod.Finalizers)).To(Equal(1))
149153
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer"))
150154
})
151155

152-
It("should return expected needsUpdate and error values when registering multiple finalizers", func() {
156+
It("should return expected result values and error values when registering multiple finalizers", func() {
153157
now := metav1.Now()
154158
pod.DeletionTimestamp = &now
155159
pod.Finalizers = []string{
@@ -159,45 +163,51 @@ var _ = Describe("TestFinalizer", func() {
159163
}
160164

161165
// registering multiple finalizers with different return values
162-
// test for needsUpdate as true, and nil error
163-
f.needsUpdate = true
166+
// test for Updated as true, and nil error
167+
f.result.Updated = true
168+
f.result.StatusUpdated = false
164169
f.err = nil
165170
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer1", f)
166171
Expect(err).To(BeNil())
167172

168173
result, err := finalizers.Finalize(context.TODO(), pod)
169174
Expect(err).To(BeNil())
170-
Expect(result).To(BeTrue())
175+
Expect(result.Updated).To(BeTrue())
176+
Expect(result.StatusUpdated).To(BeFalse())
171177
// `finalizers.sigs.k8s.io/testfinalizer1` will be removed from the list
172178
// of finalizers, so length will be 2.
173179
Expect(len(pod.Finalizers)).To(Equal(2))
174180
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2"))
175181
Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3"))
176182

177-
// test for needsUpdate as false, and non-nil error
178-
f.needsUpdate = false
183+
// test for Updated and StatusUpdated as false, and non-nil error
184+
f.result.Updated = false
185+
f.result.StatusUpdated = false
179186
f.err = fmt.Errorf("finalizer failed")
180187
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer2", f)
181188
Expect(err).To(BeNil())
182189

183190
result, err = finalizers.Finalize(context.TODO(), pod)
184191
Expect(err).ToNot(BeNil())
185192
Expect(err.Error()).To(ContainSubstring("finalizer failed"))
186-
Expect(result).To(BeFalse())
193+
Expect(result.Updated).To(BeFalse())
194+
Expect(result.StatusUpdated).To(BeFalse())
187195
Expect(len(pod.Finalizers)).To(Equal(2))
188196
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2"))
189197
Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3"))
190198

191-
// test for needsUpdate as true, and non-nil error
192-
f.needsUpdate = true
199+
// test for result as true, and non-nil error
200+
f.result.Updated = true
201+
f.result.StatusUpdated = true
193202
f.err = fmt.Errorf("finalizer failed")
194203
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer3", f)
195204
Expect(err).To(BeNil())
196205

197206
result, err = finalizers.Finalize(context.TODO(), pod)
198207
Expect(err).ToNot(BeNil())
199208
Expect(err.Error()).To(ContainSubstring("finalizer failed"))
200-
Expect(result).To(BeTrue())
209+
Expect(result.Updated).To(BeTrue())
210+
Expect(result.StatusUpdated).To(BeTrue())
201211
Expect(len(pod.Finalizers)).To(Equal(2))
202212
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2"))
203213
Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3"))

‎pkg/finalizer/types.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,7 @@ type Registerer interface {
3030
// deletion timestamp being set and return an indication of whether the
3131
// obj needs an update or not
3232
type Finalizer interface {
33-
Finalize(context.Context, client.Object) (needsUpdate bool, err error)
33+
Finalize(context.Context, client.Object) (Result, error)
3434
}
3535

3636
// Finalizers implements Registerer and Finalizer to finalize all registered

0 commit comments

Comments
 (0)