Skip to content

Commit 04848c7

Browse files
author
Max Jonas Werner
committed
chore: remove unused parameter from util funcs; add unit test
Added the unit tests from https://github.com/kubernetes/kubernetes/blob/master/pkg/util/slice/slice_test.go (minus modifier tests) so we can be sure to not change behaviour.
1 parent 1f13709 commit 04848c7

File tree

5 files changed

+73
-28
lines changed

5 files changed

+73
-28
lines changed

‎pkg/common-controller/framework_test.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1320,7 +1320,7 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
13201320
if funcName == "testAddPVCFinalizer" {
13211321
for _, pvc := range reactor.claims {
13221322
if test.initialClaims[0].Name == pvc.Name {
1323-
if !utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer, nil) && utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) {
1323+
if !utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer) && utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
13241324
klog.V(4).Infof("test %q succeeded. PVCFinalizer is added to PVC %s", test.name, pvc.Name)
13251325
bHasPVCFinalizer = true
13261326
}
@@ -1335,7 +1335,7 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
13351335
if funcName == "testRemovePVCFinalizer" {
13361336
for _, pvc := range reactor.claims {
13371337
if test.initialClaims[0].Name == pvc.Name {
1338-
if utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer, nil) && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) {
1338+
if utils.ContainsString(test.initialClaims[0].ObjectMeta.Finalizers, utils.PVCFinalizer) && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
13391339
klog.V(4).Infof("test %q succeeded. PVCFinalizer is removed from PVC %s", test.name, pvc.Name)
13401340
bHasPVCFinalizer = false
13411341
}
@@ -1350,8 +1350,8 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
13501350
if funcName == "testAddSnapshotFinalizer" {
13511351
for _, snapshot := range reactor.snapshots {
13521352
if test.initialSnapshots[0].Name == snapshot.Name {
1353-
if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) &&
1354-
!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) {
1353+
if !utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1354+
!utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
13551355
klog.V(4).Infof("test %q succeeded. Finalizers are added to snapshot %s", test.name, snapshot.Name)
13561356
bHasSnapshotFinalizer = true
13571357
}
@@ -1366,8 +1366,8 @@ func evaluateFinalizerTests(ctrl *csiSnapshotCommonController, reactor *snapshot
13661366
if funcName == "testRemoveSnapshotFinalizer" {
13671367
for _, snapshot := range reactor.snapshots {
13681368
if test.initialSnapshots[0].Name == snapshot.Name {
1369-
if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil) &&
1370-
utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil) {
1369+
if utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer) &&
1370+
utils.ContainsString(test.initialSnapshots[0].ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) && !utils.ContainsString(snapshot.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer) {
13711371
klog.V(4).Infof("test %q succeeded. SnapshotFinalizer is removed from Snapshot %s", test.name, snapshot.Name)
13721372
bHasSnapshotFinalizer = false
13731373
}

‎pkg/common-controller/snapshot_controller.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ func (ctrl *csiSnapshotCommonController) isPVCwithFinalizerInUseByCurrentSnapsho
222222
}
223223

224224
// Check if there is a Finalizer on PVC. If not, return false
225-
if !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) {
225+
if !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
226226
return false
227227
}
228228

@@ -820,7 +820,7 @@ func (ctrl *csiSnapshotCommonController) ensurePVCFinalizer(snapshot *crdv1.Volu
820820
}
821821

822822
// If PVC is not being deleted and PVCFinalizer is not added yet, the PVCFinalizer should be added.
823-
if pvc.ObjectMeta.DeletionTimestamp == nil && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) {
823+
if pvc.ObjectMeta.DeletionTimestamp == nil && !utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
824824
// Add the finalizer
825825
pvcClone := pvc.DeepCopy()
826826
pvcClone.ObjectMeta.Finalizers = append(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer)
@@ -841,7 +841,7 @@ func (ctrl *csiSnapshotCommonController) removePVCFinalizer(pvc *v1.PersistentVo
841841
// TODO(xyang): We get PVC from informer but it may be outdated
842842
// Should get it from API server directly before removing finalizer
843843
pvcClone := pvc.DeepCopy()
844-
pvcClone.ObjectMeta.Finalizers = utils.RemoveString(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer, nil)
844+
pvcClone.ObjectMeta.Finalizers = utils.RemoveString(pvcClone.ObjectMeta.Finalizers, utils.PVCFinalizer)
845845

846846
_, err := ctrl.client.CoreV1().PersistentVolumeClaims(pvcClone.Namespace).Update(context.TODO(), pvcClone, metav1.UpdateOptions{})
847847
if err != nil {
@@ -897,7 +897,7 @@ func (ctrl *csiSnapshotCommonController) checkandRemovePVCFinalizer(snapshot *cr
897897
klog.V(5).Infof("checkandRemovePVCFinalizer for snapshot [%s]: snapshot status [%#v]", snapshot.Name, snapshot.Status)
898898

899899
// Check if there is a Finalizer on PVC to be removed
900-
if utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer, nil) {
900+
if utils.ContainsString(pvc.ObjectMeta.Finalizers, utils.PVCFinalizer) {
901901
// There is a Finalizer on PVC. Check if PVC is used
902902
// and remove finalizer if it's not used.
903903
inUse := ctrl.isPVCBeingUsed(pvc, snapshot)
@@ -1294,10 +1294,10 @@ func (ctrl *csiSnapshotCommonController) removeSnapshotFinalizer(snapshot *crdv1
12941294

12951295
snapshotClone := snapshot.DeepCopy()
12961296
if removeSourceFinalizer {
1297-
snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer, nil)
1297+
snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotAsSourceFinalizer)
12981298
}
12991299
if removeBoundFinalizer {
1300-
snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer, nil)
1300+
snapshotClone.ObjectMeta.Finalizers = utils.RemoveString(snapshotClone.ObjectMeta.Finalizers, utils.VolumeSnapshotBoundFinalizer)
13011301
}
13021302
_, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshots(snapshotClone.Namespace).Update(context.TODO(), snapshotClone, metav1.UpdateOptions{})
13031303
if err != nil {

‎pkg/sidecar-controller/snapshot_controller.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -505,12 +505,12 @@ func (ctrl *csiSnapshotSideCarController) GetCredentialsFromAnnotation(content *
505505
// removeContentFinalizer removes the VolumeSnapshotContentFinalizer from a
506506
// content if there exists one.
507507
func (ctrl csiSnapshotSideCarController) removeContentFinalizer(content *crdv1.VolumeSnapshotContent) error {
508-
if !utils.ContainsString(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil) {
508+
if !utils.ContainsString(content.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer) {
509509
// the finalizer does not exit, return directly
510510
return nil
511511
}
512512
contentClone := content.DeepCopy()
513-
contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer, nil)
513+
contentClone.ObjectMeta.Finalizers = utils.RemoveString(contentClone.ObjectMeta.Finalizers, utils.VolumeSnapshotContentFinalizer)
514514

515515
_, err := ctrl.clientset.SnapshotV1beta1().VolumeSnapshotContents().Update(context.TODO(), contentClone, metav1.UpdateOptions{})
516516
if err != nil {

‎pkg/utils/util.go‎

Lines changed: 7 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -109,30 +109,23 @@ var SnapshotterListSecretParams = secretParamsMap{
109109
}
110110

111111
// ContainsString checks if a given slice of strings contains the provided string.
112-
// If a modifier func is provided, it is called with the slice item before the comparation.
113-
func ContainsString(slice []string, s string, modifier func(s string) string) bool {
112+
func ContainsString(slice []string, s string) bool {
114113
for _, item := range slice {
115114
if item == s {
116115
return true
117116
}
118-
if modifier != nil && modifier(item) == s {
119-
return true
120-
}
121117
}
122118
return false
123119
}
124120

125121
// RemoveString returns a newly created []string that contains all items from slice that
126-
// are not equal to s and modifier(s) in case modifier func is provided.
127-
func RemoveString(slice []string, s string, modifier func(s string) string) []string {
122+
// are not equal to s.
123+
func RemoveString(slice []string, s string) []string {
128124
newSlice := make([]string, 0)
129125
for _, item := range slice {
130126
if item == s {
131127
continue
132128
}
133-
if modifier != nil && modifier(item) == s {
134-
continue
135-
}
136129
newSlice = append(newSlice, item)
137130
}
138131
if len(newSlice) == 0 {
@@ -371,23 +364,23 @@ func NoResyncPeriodFunc() time.Duration {
371364

372365
// NeedToAddContentFinalizer checks if a Finalizer needs to be added for the volume snapshot content.
373366
func NeedToAddContentFinalizer(content *crdv1.VolumeSnapshotContent) bool {
374-
return content.ObjectMeta.DeletionTimestamp == nil && !ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer, nil)
367+
return content.ObjectMeta.DeletionTimestamp == nil && !ContainsString(content.ObjectMeta.Finalizers, VolumeSnapshotContentFinalizer)
375368
}
376369

377370
// IsSnapshotDeletionCandidate checks if a volume snapshot deletionTimestamp
378371
// is set and any finalizer is on the snapshot.
379372
func IsSnapshotDeletionCandidate(snapshot *crdv1.VolumeSnapshot) bool {
380-
return snapshot.ObjectMeta.DeletionTimestamp != nil && (ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer, nil) || ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil))
373+
return snapshot.ObjectMeta.DeletionTimestamp != nil && (ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer) || ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer))
381374
}
382375

383376
// NeedToAddSnapshotAsSourceFinalizer checks if a Finalizer needs to be added for the volume snapshot as a source for PVC.
384377
func NeedToAddSnapshotAsSourceFinalizer(snapshot *crdv1.VolumeSnapshot) bool {
385-
return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer, nil)
378+
return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotAsSourceFinalizer)
386379
}
387380

388381
// NeedToAddSnapshotBoundFinalizer checks if a Finalizer needs to be added for the bound volume snapshot.
389382
func NeedToAddSnapshotBoundFinalizer(snapshot *crdv1.VolumeSnapshot) bool {
390-
return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer, nil) && IsBoundVolumeSnapshotContentNameSet(snapshot)
383+
return snapshot.ObjectMeta.DeletionTimestamp == nil && !ContainsString(snapshot.ObjectMeta.Finalizers, VolumeSnapshotBoundFinalizer) && IsBoundVolumeSnapshotContentNameSet(snapshot)
391384
}
392385

393386
func deprecationWarning(deprecatedParam, newParam, removalVersion string) string {

‎pkg/utils/util_test.go‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,58 @@ import (
2525
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2626
)
2727

28+
func TestContainsString(t *testing.T) {
29+
src := []string{"aa", "bb", "cc"}
30+
if !ContainsString(src, "bb") {
31+
t.Errorf("ContainsString didn't find the string as expected")
32+
}
33+
}
34+
35+
func TestRemoveString(t *testing.T) {
36+
tests := []struct {
37+
testName string
38+
input []string
39+
remove string
40+
want []string
41+
}{
42+
{
43+
testName: "Nil input slice",
44+
input: nil,
45+
remove: "",
46+
want: nil,
47+
},
48+
{
49+
testName: "Slice doesn't contain the string",
50+
input: []string{"a", "ab", "cdef"},
51+
remove: "NotPresentInSlice",
52+
want: []string{"a", "ab", "cdef"},
53+
},
54+
{
55+
testName: "All strings removed, result is nil",
56+
input: []string{"a"},
57+
remove: "a",
58+
want: nil,
59+
},
60+
{
61+
testName: "One string removed",
62+
input: []string{"a", "ab", "cdef"},
63+
remove: "ab",
64+
want: []string{"a", "cdef"},
65+
},
66+
{
67+
testName: "All(three) strings removed",
68+
input: []string{"ab", "a", "ab", "cdef", "ab"},
69+
remove: "ab",
70+
want: []string{"a", "cdef"},
71+
},
72+
}
73+
for _, tt := range tests {
74+
if got := RemoveString(tt.input, tt.remove); !reflect.DeepEqual(got, tt.want) {
75+
t.Errorf("%v: RemoveString(%v, %q) = %v WANT %v", tt.testName, tt.input, tt.remove, got, tt.want)
76+
}
77+
}
78+
}
79+
2880
func TestGetSecretReference(t *testing.T) {
2981
testcases := map[string]struct {
3082
secretParams secretParamsMap

0 commit comments

Comments
 (0)