Skip to content

Commit 2b7ed78

Browse files
afritzlerafritzler
committed
Address PR review comments
1 parent 29649ae commit 2b7ed78

File tree

4 files changed

+44
-39
lines changed

4 files changed

+44
-39
lines changed

‎internal/controller/test_helper.go‎

Lines changed: 24 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -16,35 +16,41 @@ func EnsureCleanState() {
1616
GinkgoHelper()
1717

1818
Eventually(func(g Gomega) error {
19-
bmcList := &metalv1alpha1.BMCList{}
20-
g.Eventually(ObjectList(bmcList)).Should(HaveField("Items", HaveLen(0)))
19+
endpoints := &metalv1alpha1.EndpointList{}
20+
g.Eventually(ObjectList(endpoints)).Should(HaveField("Items", HaveLen(0)))
2121

22-
bmcSecretList := &metalv1alpha1.BMCSecretList{}
23-
g.Eventually(ObjectList(bmcSecretList)).Should(HaveField("Items", HaveLen(0)))
22+
bmcs := &metalv1alpha1.BMCList{}
23+
g.Eventually(ObjectList(bmcs)).Should(HaveField("Items", HaveLen(0)))
2424

25-
claimList := &metalv1alpha1.ServerClaimList{}
26-
g.Eventually(ObjectList(claimList)).Should(HaveField("Items", HaveLen(0)))
25+
bmcSecrets := &metalv1alpha1.BMCSecretList{}
26+
g.Eventually(ObjectList(bmcSecrets)).Should(HaveField("Items", HaveLen(0)))
27+
28+
claims := &metalv1alpha1.ServerClaimList{}
29+
g.Eventually(ObjectList(claims)).Should(HaveField("Items", HaveLen(0)))
2730

2831
bmcSettingsList := &metalv1alpha1.BMCSettingsList{}
2932
g.Eventually(ObjectList(bmcSettingsList)).Should(HaveField("Items", HaveLen(0)))
3033

31-
bmcVersionSetList := &metalv1alpha1.BMCVersionSetList{}
32-
g.Eventually(ObjectList(bmcVersionSetList)).Should(HaveField("Items", HaveLen(0)))
34+
bmcVersionSets := &metalv1alpha1.BMCVersionSetList{}
35+
g.Eventually(ObjectList(bmcVersionSets)).Should(HaveField("Items", HaveLen(0)))
36+
37+
bmcVersions := &metalv1alpha1.BMCVersionList{}
38+
g.Eventually(ObjectList(bmcVersions)).Should(HaveField("Items", HaveLen(0)))
3339

34-
bmcVersionList := &metalv1alpha1.BMCVersionList{}
35-
g.Eventually(ObjectList(bmcVersionList)).Should(HaveField("Items", HaveLen(0)))
40+
biosVersions := &metalv1alpha1.BIOSVersionList{}
41+
g.Eventually(ObjectList(biosVersions)).Should(HaveField("Items", HaveLen(0)))
3642

37-
biosSettingsSetList := &metalv1alpha1.BIOSSettingsSetList{}
38-
g.Eventually(ObjectList(biosSettingsSetList)).Should(HaveField("Items", HaveLen(0)))
43+
biosSettingsSets := &metalv1alpha1.BIOSSettingsSetList{}
44+
g.Eventually(ObjectList(biosSettingsSets)).Should(HaveField("Items", HaveLen(0)))
3945

40-
biosSettings := &metalv1alpha1.BIOSSettingsList{}
41-
g.Eventually(ObjectList(biosSettings)).Should(HaveField("Items", HaveLen(0)))
46+
biosSettingsList := &metalv1alpha1.BIOSSettingsList{}
47+
g.Eventually(ObjectList(biosSettingsList)).Should(HaveField("Items", HaveLen(0)))
4248

43-
maintenanceList := &metalv1alpha1.ServerMaintenanceList{}
44-
g.Eventually(ObjectList(maintenanceList)).Should(HaveField("Items", HaveLen(0)))
49+
maintenances := &metalv1alpha1.ServerMaintenanceList{}
50+
g.Eventually(ObjectList(maintenances)).Should(HaveField("Items", HaveLen(0)))
4551

46-
serverList := &metalv1alpha1.ServerList{}
47-
g.Eventually(ObjectList(serverList)).Should(HaveField("Items", HaveLen(0)))
52+
servers := &metalv1alpha1.ServerList{}
53+
g.Eventually(ObjectList(servers)).Should(HaveField("Items", HaveLen(0)))
4854

4955
return nil
5056
}).Should(Succeed())

‎internal/webhook/v1alpha1/biossettings_webhook_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ var _ = Describe("BIOSSettings Webhook", func() {
9898
Expect(k8sClient.Create(ctx, biosSettingsV2)).To(Succeed())
9999
})
100100

101-
It("Should deny update if a Spec.ServerRef field is duplicate", func() {
101+
It("Should deny update if spec.serverRef is duplicate", func() {
102102
By("Creating a BIOSSettings with different ServerRef")
103103
biosSettingsV2 := &metalv1alpha1.BIOSSettings{
104104
ObjectMeta: metav1.ObjectMeta{

‎internal/webhook/v1alpha1/biosversion_webhook.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,7 @@ func (v *BIOSVersionCustomValidator) ValidateDelete(ctx context.Context, obj run
9797
versionLog.Info("Validation for BIOSVersion upon deletion", "name", version.GetName())
9898

9999
if version.Status.State == metalv1alpha1.BIOSVersionStateInProgress && !ShouldAllowForceDeleteInProgress(version) {
100-
return nil, apierrors.NewBadRequest("The bios version in progress, unable to delete")
100+
return nil, apierrors.NewBadRequest("The BIOS version is in progress and cannot be deleted")
101101
}
102102
return nil, nil
103103
}
@@ -115,7 +115,7 @@ func checkForDuplicateBIOSVersionRefToServer(versions *metalv1alpha1.BIOSVersion
115115
continue
116116
}
117117
if version.Spec.ServerRef.Name == bv.Spec.ServerRef.Name {
118-
err := fmt.Errorf("server (%s) referred in %s is duplicate of Server (%s) referred in %s",
118+
err := fmt.Errorf("server (%s) referred in %s is duplicate of server (%s) referred in %s",
119119
version.Spec.ServerRef.Name,
120120
version.Name,
121121
bv.Spec.ServerRef.Name,

‎internal/webhook/v1alpha1/biosversion_webhook_test.go‎

Lines changed: 17 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ var _ = Describe("BIOSVersion Webhook", func() {
2222

2323
BeforeEach(func() {
2424
validator = BIOSVersionCustomValidator{Client: k8sClient}
25-
By("Creating an BIOSVersion")
25+
By("Creating a BIOSVersion")
2626
biosVersionV1 = &metalv1alpha1.BIOSVersion{
2727
ObjectMeta: metav1.ObjectMeta{
2828
GenerateName: "test-",
@@ -40,14 +40,14 @@ var _ = Describe("BIOSVersion Webhook", func() {
4040
})
4141

4242
AfterEach(func() {
43-
By("Deleting the BIOSVersion and Server resources")
43+
By("Deleting the BIOSVersion resources")
4444
Expect(k8sClient.DeleteAllOf(ctx, &metalv1alpha1.BIOSVersion{})).To(Succeed())
4545

4646
By("Ensuring clean state")
4747
controller.EnsureCleanState()
4848
})
4949

50-
It("Should deny creation if a spec.serverRef field is duplicate", func(ctx SpecContext) {
50+
It("Should deny creation if spec.serverRef is duplicate", func(ctx SpecContext) {
5151
By("Creating another BIOSVersion with existing ServerRef")
5252
biosVersionV2 := &metalv1alpha1.BIOSVersion{
5353
ObjectMeta: metav1.ObjectMeta{
@@ -80,11 +80,11 @@ var _ = Describe("BIOSVersion Webhook", func() {
8080
ServerRef: &v1.LocalObjectReference{Name: "bar"},
8181
},
8282
}
83-
Expect(k8sClient.Create(ctx, biosVersionV2)).To(Succeed())
83+
Expect(validator.ValidateCreate(ctx, biosVersionV2)).Error().ToNot(HaveOccurred())
8484
})
8585

86-
It("Should deny update if a spec.serverRef field is a duplicate", func() {
87-
By("Creating an BIOSVersion with different ServerRef")
86+
It("Should deny update if spec.serverRef is duplicate", func() {
87+
By("Creating a BIOSVersion with different ServerRef")
8888
biosVersionV2 := &metalv1alpha1.BIOSVersion{
8989
ObjectMeta: metav1.ObjectMeta{
9090
GenerateName: "test-",
@@ -100,14 +100,14 @@ var _ = Describe("BIOSVersion Webhook", func() {
100100
}
101101
Expect(k8sClient.Create(ctx, biosVersionV2)).To(Succeed())
102102

103-
By("Updating an BIOSVersion V2 to conflicting spec.serverRef")
103+
By("Updating a BIOSVersion V2 to conflicting spec.serverRef")
104104
biosVersionV2Updated := biosVersionV2.DeepCopy()
105105
biosVersionV2Updated.Spec.ServerRef = &v1.LocalObjectReference{Name: "foo"}
106106
Expect(validator.ValidateUpdate(ctx, biosVersionV2, biosVersionV2Updated)).Error().To(HaveOccurred())
107107
})
108108

109109
It("Should allow update if a different field is duplicate", func() {
110-
By("Creating an BIOSVersion with different ServerRef")
110+
By("Creating a BIOSVersion with different ServerRef")
111111
biosVersionV2 := &metalv1alpha1.BIOSVersion{
112112
ObjectMeta: metav1.ObjectMeta{
113113
GenerateName: "test-",
@@ -123,14 +123,14 @@ var _ = Describe("BIOSVersion Webhook", func() {
123123
}
124124
Expect(k8sClient.Create(ctx, biosVersionV2)).To(Succeed())
125125

126-
By("Updating an BIOSVersion V2 to conflicting spec.biosVersionSpec")
126+
By("Updating BIOSVersion V2 to duplicate spec.image")
127127
biosVersionV2Updated := biosVersionV2.DeepCopy()
128128
biosVersionV2Updated.Spec.Image = biosVersionV1.Spec.Image
129129
Expect(validator.ValidateUpdate(ctx, biosVersionV2, biosVersionV2Updated)).Error().ToNot(HaveOccurred())
130130
})
131131

132132
It("Should allow update if a ServerRef field is not a duplicate", func() {
133-
By("Creating an BIOSVersion with different ServerRef")
133+
By("Creating a BIOSVersion with different ServerRef")
134134
biosVersionV2 := &metalv1alpha1.BIOSVersion{
135135
ObjectMeta: metav1.ObjectMeta{
136136
GenerateName: "test-",
@@ -146,19 +146,19 @@ var _ = Describe("BIOSVersion Webhook", func() {
146146
}
147147
Expect(k8sClient.Create(ctx, biosVersionV2)).To(Succeed())
148148

149-
By("Updating an BIOSVersion V2 to a non-conflicting spec.serverRef ")
149+
By("Updating a BIOSVersion V2 to a non-conflicting spec.serverRef")
150150
biosVersionV2Updated := biosVersionV2.DeepCopy()
151151
biosVersionV2Updated.Spec.ServerRef = &v1.LocalObjectReference{Name: "foobar"}
152152
Expect(validator.ValidateUpdate(ctx, biosVersionV2, biosVersionV2Updated)).Error().ToNot(HaveOccurred())
153153
})
154154

155-
It("Should no allow update a BIOSVersion is in progress, but should allow to force update it", func() {
155+
It("Should not allow update when BIOSVersion is in progress, but should allow force update", func() {
156156
By("Patching the BIOSVersion V1 to in-progress state")
157157
Eventually(UpdateStatus(biosVersionV1, func() {
158158
biosVersionV1.Status.State = metalv1alpha1.BIOSVersionStateInProgress
159159
})).Should(Succeed())
160160

161-
By("Add ServerMaintenance reference")
161+
By("Adding ServerMaintenance reference")
162162
Eventually(Update(biosVersionV1, func() {
163163
biosVersionV1.Spec.ServerMaintenanceRef = &metalv1alpha1.ObjectReference{Name: "maintenance"}
164164
})).Should(Succeed())
@@ -168,7 +168,7 @@ var _ = Describe("BIOSVersion Webhook", func() {
168168
biosVersionV1Updated.Spec.Version = "P712"
169169
Expect(validator.ValidateUpdate(ctx, biosVersionV1, biosVersionV1Updated)).Error().To(HaveOccurred())
170170

171-
By("Updating an biosVersion V1 spec, should pass to update when inProgress with ForceUpdateResource finalizer")
171+
By("Updating BIOSVersion V1 spec should succeed when InProgress with ForceUpdateInProgress annotation")
172172
biosVersionV1Updated.Annotations = map[string]string{metalv1alpha1.OperationAnnotation: metalv1alpha1.OperationAnnotationForceUpdateInProgress}
173173
Expect(validator.ValidateUpdate(ctx, biosVersionV1, biosVersionV1Updated)).Error().ToNot(HaveOccurred())
174174

@@ -179,17 +179,16 @@ var _ = Describe("BIOSVersion Webhook", func() {
179179
})
180180

181181
It("Should refuse to delete if InProgress", func() {
182-
By("Patching the BIOSVersionV1 to InProgress state")
182+
By("Patching the BIOSVersion V1 to InProgress state")
183183
Eventually(UpdateStatus(biosVersionV1, func() {
184184
biosVersionV1.Status.State = metalv1alpha1.BIOSVersionStateInProgress
185185
})).Should(Succeed())
186186

187-
By("Deleting the BIOSSettings V1 should fail")
188-
Expect(k8sClient.Delete(ctx, biosVersionV1)).To(Not(Succeed()))
187+
By("Validating deletion of BIOSVersion V1 should fail")
188+
Expect(validator.ValidateDelete(ctx, biosVersionV1)).Error().To(HaveOccurred())
189189

190190
Eventually(UpdateStatus(biosVersionV1, func() {
191191
biosVersionV1.Status.State = metalv1alpha1.BIOSVersionStateCompleted
192192
})).Should(Succeed())
193193
})
194-
195194
})

0 commit comments

Comments
 (0)