Skip to content

Commit 4a125b8

Browse files
johnbierenjohnbieren
authored
Merge pull request konflux-ci#380 from davidmogar/fix_webhooks_tests
fix: use loader to avoid using the client
2 parents 3d33348 + 27a38f0 commit 4a125b8

File tree

3 files changed

+95
-74
lines changed

3 files changed

+95
-74
lines changed

‎api/v1alpha1/webhooks/release/suite_test.go‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
"github.com/redhat-appstudio/release-service/api/v1alpha1"
3030
"github.com/redhat-appstudio/release-service/api/v1alpha1/webhooks/author"
3131

32-
releaseplanwebhook "github.com/redhat-appstudio/release-service/api/v1alpha1/webhooks/releaseplan"
3332
"sigs.k8s.io/controller-runtime/pkg/metrics/server"
3433
crwebhook "sigs.k8s.io/controller-runtime/pkg/webhook"
3534

@@ -105,7 +104,7 @@ var _ = BeforeSuite(func() {
105104
})
106105
Expect(err).NotTo(HaveOccurred())
107106

108-
err = toolkit.SetupWebhooks(mgr, &Webhook{}, &author.Webhook{}, &releaseplanwebhook.Webhook{})
107+
err = toolkit.SetupWebhooks(mgr, &Webhook{}, &author.Webhook{})
109108
Expect(err).NotTo(HaveOccurred())
110109

111110
//+kubebuilder:scaffold:webhook

‎api/v1alpha1/webhooks/release/webhook.go‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,13 @@ package release
1919
import (
2020
"context"
2121
"fmt"
22+
"github.com/redhat-appstudio/release-service/loader"
2223
"reflect"
2324

2425
"github.com/go-logr/logr"
2526
"github.com/redhat-appstudio/release-service/api/v1alpha1"
2627
"k8s.io/apimachinery/pkg/api/errors"
2728
"k8s.io/apimachinery/pkg/runtime"
28-
"k8s.io/apimachinery/pkg/types"
2929
ctrl "sigs.k8s.io/controller-runtime"
3030
"sigs.k8s.io/controller-runtime/pkg/client"
3131
"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
@@ -34,28 +34,30 @@ import (
3434
// Webhook describes the data structure for the release webhook
3535
type Webhook struct {
3636
client client.Client
37+
loader loader.ObjectLoader
3738
log logr.Logger
3839
}
3940

41+
// Default implements webhook.Defaulter so a webhook will be registered for the type.
4042
func (w *Webhook) Default(ctx context.Context, obj runtime.Object) error {
4143
release := obj.(*v1alpha1.Release)
4244

43-
if release.Spec.GracePeriodDays == 0 {
44-
releasePlan := &v1alpha1.ReleasePlan{}
45-
46-
err := w.client.Get(ctx, types.NamespacedName{
47-
Name: release.Spec.ReleasePlan,
48-
Namespace: release.Namespace,
49-
}, releasePlan)
45+
if release.Spec.GracePeriodDays != 0 {
46+
return nil
47+
}
5048

51-
if err != nil && errors.IsNotFound(err) {
49+
releasePlan, err := w.loader.GetReleasePlan(ctx, w.client, release)
50+
if err != nil {
51+
if errors.IsNotFound(err) {
5252
w.log.Info("releasePlan not found. Not setting ReleaseGracePeriodDays")
53-
5453
return nil
54+
} else {
55+
return err
5556
}
56-
release.Spec.GracePeriodDays = releasePlan.Spec.ReleaseGracePeriodDays
5757
}
5858

59+
release.Spec.GracePeriodDays = releasePlan.Spec.ReleaseGracePeriodDays
60+
5961
return nil
6062
}
6163

@@ -64,6 +66,7 @@ func (w *Webhook) Default(ctx context.Context, obj runtime.Object) error {
6466

6567
func (w *Webhook) Register(mgr ctrl.Manager, log *logr.Logger) error {
6668
w.client = mgr.GetClient()
69+
w.loader = loader.NewLoader()
6770
w.log = log.WithName("release")
6871

6972
return ctrl.NewWebhookManagedBy(mgr).

‎api/v1alpha1/webhooks/release/webhook_test.go‎

Lines changed: 80 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,11 @@ package release
1717

1818
import (
1919
"context"
20-
20+
toolkit "github.com/redhat-appstudio/operator-toolkit/loader"
2121
"github.com/redhat-appstudio/release-service/api/v1alpha1"
22+
"github.com/redhat-appstudio/release-service/loader"
23+
"k8s.io/apimachinery/pkg/api/errors"
24+
"k8s.io/apimachinery/pkg/runtime/schema"
2225

2326
. "github.com/onsi/ginkgo/v2"
2427
. "github.com/onsi/gomega"
@@ -28,10 +31,81 @@ import (
2831
)
2932

3033
var _ = Describe("Release validation webhook", func() {
31-
var release *v1alpha1.Release
32-
var releasePlan *v1alpha1.ReleasePlan
34+
var (
35+
createResources func()
36+
37+
release *v1alpha1.Release
38+
releasePlan *v1alpha1.ReleasePlan
39+
)
40+
41+
When("Default method is called", func() {
42+
var mockedWebhook *Webhook
43+
44+
BeforeEach(func() {
45+
createResources()
46+
47+
mockedWebhook = &Webhook{
48+
client: k8sClient,
49+
loader: loader.NewMockLoader(),
50+
}
51+
})
52+
53+
It("should set GracePeriodDays to ReleasePlan's value and return nil", func() {
54+
mockedCtx := toolkit.GetMockedContext(ctx, []toolkit.MockData{
55+
{
56+
ContextKey: loader.ReleasePlanContextKey,
57+
Resource: releasePlan,
58+
},
59+
})
60+
61+
Expect(mockedWebhook.Default(mockedCtx, release)).To(BeNil())
62+
Expect(release.Spec.GracePeriodDays).To(Equal(releasePlan.Spec.ReleaseGracePeriodDays))
63+
})
64+
65+
It("should return nil and keep the default value of a go `int` for GracePeriodDays when the specified ReleasePlan does not exist", func() {
66+
mockedCtx := toolkit.GetMockedContext(ctx, []toolkit.MockData{
67+
{
68+
ContextKey: loader.ReleasePlanContextKey,
69+
Err: errors.NewNotFound(schema.GroupResource{}, ""),
70+
},
71+
})
72+
73+
Expect(mockedWebhook.Default(mockedCtx, release)).To(BeNil())
74+
Expect(release.Spec.GracePeriodDays).To(Equal(0))
75+
})
76+
})
77+
78+
When("When ValidateUpdate is called", func() {
79+
It("should error out when updating the resource", func() {
80+
updatedRelease := release.DeepCopy()
81+
updatedRelease.Spec.Snapshot = "another-snapshot"
82+
83+
_, err := webhook.ValidateUpdate(ctx, release, updatedRelease)
84+
Expect(err).Should(HaveOccurred())
85+
Expect(err.Error()).Should(ContainSubstring("release resources spec cannot be updated"))
86+
})
87+
88+
It("should not error out when updating the resource metadata", func() {
89+
ctx := context.Background()
90+
91+
updatedRelease := release.DeepCopy()
92+
updatedRelease.ObjectMeta.Annotations = map[string]string{
93+
"foo": "bar",
94+
}
95+
96+
_, err := webhook.ValidateUpdate(ctx, release, updatedRelease)
97+
Expect(err).NotTo(HaveOccurred())
98+
})
99+
})
33100

34-
BeforeEach(func() {
101+
When("ValidateDelete method is called", func() {
102+
It("should return nil", func() {
103+
_, err := webhook.ValidateDelete(ctx, &v1alpha1.Release{})
104+
Expect(err).NotTo(HaveOccurred())
105+
})
106+
})
107+
108+
createResources = func() {
35109
release = &v1alpha1.Release{
36110
TypeMeta: metav1.TypeMeta{
37111
APIVersion: "appstudio.redhat.com/v1alpha1",
@@ -46,6 +120,7 @@ var _ = Describe("Release validation webhook", func() {
46120
ReleasePlan: "test-releaseplan",
47121
},
48122
}
123+
49124
releasePlan = &v1alpha1.ReleasePlan{
50125
TypeMeta: metav1.TypeMeta{
51126
APIVersion: "appstudio.redhat.com/v1alpha1",
@@ -61,61 +136,5 @@ var _ = Describe("Release validation webhook", func() {
61136
ReleaseGracePeriodDays: 7,
62137
},
63138
}
64-
})
65-
66-
AfterEach(func() {
67-
_ = k8sClient.Delete(ctx, release)
68-
})
69-
70-
When("release CR fields are updated", func() {
71-
It("Should error out when updating the resource", func() {
72-
ctx := context.Background()
73-
74-
Expect(k8sClient.Create(ctx, release)).Should(Succeed())
75-
76-
// Try to update the Release snapshot
77-
release.Spec.Snapshot = "another-snapshot"
78-
79-
err := k8sClient.Update(ctx, release)
80-
Expect(err).Should(HaveOccurred())
81-
Expect(err.Error()).Should(ContainSubstring("release resources spec cannot be updated"))
82-
})
83-
84-
It("Should not error out when updating the resource metadata", func() {
85-
ctx := context.Background()
86-
87-
Expect(k8sClient.Create(ctx, release)).Should(Succeed())
88-
89-
// Try to update the Release annotations
90-
release.ObjectMeta.Annotations = map[string]string{
91-
"foo": "bar",
92-
}
93-
94-
Expect(k8sClient.Update(ctx, release)).ShouldNot(HaveOccurred())
95-
})
96-
})
97-
98-
When("ValidateDelete method is called", func() {
99-
It("should return nil", func() {
100-
release := &v1alpha1.Release{}
101-
Expect(webhook.ValidateDelete(ctx, release)).To(BeNil())
102-
})
103-
})
104-
105-
When("a new Release is created", func() {
106-
It("should set GracePeriodDays to ReleasePlan's value and return nil", func() {
107-
ctx := context.Background()
108-
Expect(k8sClient.Create(ctx, releasePlan)).Should(Succeed())
109-
Expect(k8sClient.Create(ctx, release)).Should(Succeed())
110-
Expect(release.Spec.GracePeriodDays).To(Equal(releasePlan.Spec.ReleaseGracePeriodDays))
111-
112-
Expect(k8sClient.Delete(ctx, releasePlan)).Should(Succeed())
113-
})
114-
115-
It("should return nil and keep the default value of a go `int` for GracePeriodDays when the specified ReleasePlan does not exist", func() {
116-
ctx := context.Background()
117-
Expect(k8sClient.Create(ctx, release)).Should(Succeed())
118-
Expect(release.Spec.GracePeriodDays).To(Equal(0))
119-
})
120-
})
139+
}
121140
})

0 commit comments

Comments
 (0)