Skip to content

Commit 337937e

Browse files
author
Yuvaraj Kakaraparthi
committed
address review comments
1 parent 4c55b03 commit 337937e

File tree

7 files changed

+48
-45
lines changed

7 files changed

+48
-45
lines changed

‎exp/runtime/api/v1alpha1/extensionconfig_types.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -207,4 +207,7 @@ const (
207207
// object wants injection of CAs. It takes the form of a reference to a Secret
208208
// as namespace/name.
209209
InjectCAFromSecretAnnotation string = "runtime.cluster.x-k8s.io/inject-ca-from-secret"
210+
211+
// PendingHooksAnnotation is the annotation used to keep a track of pending runtime hooks.
212+
PendingHooksAnnotation string = "hooks.x-cluster.k8s.io/pending-hooks"
210213
)

‎exp/runtime/hooks/api/v1alpha1/common_types.go‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,9 +20,6 @@ import (
2020
"k8s.io/apimachinery/pkg/runtime"
2121
)
2222

23-
// PendingHooksAnnotation is the annotation used to keep a track of pending runtime hooks.
24-
const PendingHooksAnnotation = "hooks.x-cluster.k8s.io/pending-hooks"
25-
2623
// ResponseObject is a runtime object extended with methods to handle response-specific fields.
2724
// +kubebuilder:object:generate=false
2825
type ResponseObject interface {

‎internal/controllers/topology/cluster/desired_state.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -315,7 +315,7 @@ func (r *Reconciler) computeControlPlaneVersion(ctx context.Context, s *scope.Sc
315315
// to know if the control plane is being upgraded. This information
316316
// is required when updating the TopologyReconciled condition on the cluster.
317317

318-
// Let's call the AfterControlPlaneUpgrade now that the control plane is upgraded.
318+
// Call the AfterControlPlaneUpgrade now that the control plane is upgraded.
319319
if feature.Gates.Enabled(feature.RuntimeSDK) {
320320
// Call the hook only if it is marked. If it is not marked it means we don't need ot call the
321321
// hook because we didn't go through an upgrade or we already called the hook after the upgrade.

‎internal/controllers/topology/cluster/desired_state_test.go‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import (
3434
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
3535

3636
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
37+
runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1"
3738
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
3839
"sigs.k8s.io/cluster-api/feature"
3940
"sigs.k8s.io/cluster-api/internal/contract"
@@ -955,7 +956,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
955956
Name: "test-cluster",
956957
Namespace: "test-ns",
957958
Annotations: map[string]string{
958-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
959+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
959960
},
960961
},
961962
Spec: clusterv1.ClusterSpec{},
@@ -986,7 +987,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
986987
Name: "test-cluster",
987988
Namespace: "test-ns",
988989
Annotations: map[string]string{
989-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
990+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
990991
},
991992
},
992993
Spec: clusterv1.ClusterSpec{},
@@ -1017,7 +1018,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
10171018
Name: "test-cluster",
10181019
Namespace: "test-ns",
10191020
Annotations: map[string]string{
1020-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
1021+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
10211022
},
10221023
},
10231024
Spec: clusterv1.ClusterSpec{},
@@ -1050,7 +1051,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
10501051
Name: "test-cluster",
10511052
Namespace: "test-ns",
10521053
Annotations: map[string]string{
1053-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
1054+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
10541055
},
10551056
},
10561057
Spec: clusterv1.ClusterSpec{},
@@ -1083,7 +1084,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
10831084
Name: "test-cluster",
10841085
Namespace: "test-ns",
10851086
Annotations: map[string]string{
1086-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
1087+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
10871088
},
10881089
},
10891090
Spec: clusterv1.ClusterSpec{},

‎internal/controllers/topology/cluster/reconcile_state_test.go‎

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import (
3636
"sigs.k8s.io/controller-runtime/pkg/client/fake"
3737

3838
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
39+
runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1"
3940
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
4041
"sigs.k8s.io/cluster-api/internal/contract"
4142
"sigs.k8s.io/cluster-api/internal/controllers/topology/cluster/scope"
@@ -325,7 +326,7 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) {
325326
Name: "test-cluster",
326327
Namespace: "test-ns",
327328
Annotations: map[string]string{
328-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneInitialized",
329+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneInitialized",
329330
},
330331
},
331332
Spec: clusterv1.ClusterSpec{
@@ -353,7 +354,7 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) {
353354
Name: "test-cluster",
354355
Namespace: "test-ns",
355356
Annotations: map[string]string{
356-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneInitialized",
357+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneInitialized",
357358
},
358359
},
359360
Spec: clusterv1.ClusterSpec{
@@ -381,7 +382,7 @@ func TestReconcile_callAfterControlPlaneInitialized(t *testing.T) {
381382
Name: "test-cluster",
382383
Namespace: "test-ns",
383384
Annotations: map[string]string{
384-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneInitialized",
385+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneInitialized",
385386
},
386387
},
387388
Spec: clusterv1.ClusterSpec{
@@ -586,7 +587,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
586587
Name: "test-cluster",
587588
Namespace: "test-ns",
588589
Annotations: map[string]string{
589-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
590+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
590591
},
591592
},
592593
Spec: clusterv1.ClusterSpec{},
@@ -619,7 +620,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
619620
Name: "test-cluster",
620621
Namespace: "test-ns",
621622
Annotations: map[string]string{
622-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
623+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
623624
},
624625
},
625626
Spec: clusterv1.ClusterSpec{},
@@ -652,7 +653,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
652653
Name: "test-cluster",
653654
Namespace: "test-ns",
654655
Annotations: map[string]string{
655-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
656+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
656657
},
657658
},
658659
Spec: clusterv1.ClusterSpec{},
@@ -689,7 +690,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
689690
Name: "test-cluster",
690691
Namespace: "test-ns",
691692
Annotations: map[string]string{
692-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
693+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
693694
},
694695
},
695696
Spec: clusterv1.ClusterSpec{},
@@ -727,7 +728,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
727728
Name: "test-cluster",
728729
Namespace: "test-ns",
729730
Annotations: map[string]string{
730-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
731+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
731732
},
732733
},
733734
Spec: clusterv1.ClusterSpec{},
@@ -765,7 +766,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
765766
Name: "test-cluster",
766767
Namespace: "test-ns",
767768
Annotations: map[string]string{
768-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
769+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
769770
},
770771
},
771772
Spec: clusterv1.ClusterSpec{
@@ -802,7 +803,7 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) {
802803
Name: "test-cluster",
803804
Namespace: "test-ns",
804805
Annotations: map[string]string{
805-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
806+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
806807
},
807808
},
808809
Spec: clusterv1.ClusterSpec{

‎internal/hooks/tracking.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import (
2525
"k8s.io/apimachinery/pkg/util/sets"
2626
"sigs.k8s.io/controller-runtime/pkg/client"
2727

28-
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
28+
runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1"
2929
runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
3030
"sigs.k8s.io/cluster-api/util/patch"
3131
)
@@ -46,7 +46,7 @@ func MarkAsPending(ctx context.Context, c client.Client, obj client.Object, hook
4646
if annotations == nil {
4747
annotations = map[string]string{}
4848
}
49-
annotations[runtimehooksv1.PendingHooksAnnotation] = addToCommaSeparatedList(annotations[runtimehooksv1.PendingHooksAnnotation], hookNames...)
49+
annotations[runtimev1.PendingHooksAnnotation] = addToCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...)
5050
obj.SetAnnotations(annotations)
5151

5252
if err := patchHelper.Patch(ctx, obj); err != nil {
@@ -63,10 +63,10 @@ func IsPending(hook runtimecatalog.Hook, obj client.Object) bool {
6363
if annotations == nil {
6464
return false
6565
}
66-
return isInCommaSeparatedList(annotations[runtimehooksv1.PendingHooksAnnotation], hookName)
66+
return isInCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookName)
6767
}
6868

69-
// MarkAsDone remove the information on the object that represents tha hook is marked.
69+
// MarkAsDone removes the information on the object that represents that the hook is pending.
7070
func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks ...runtimecatalog.Hook) (retErr error) {
7171
patchHelper, err := patch.NewHelper(obj, c)
7272
if err != nil {
@@ -82,9 +82,9 @@ func MarkAsDone(ctx context.Context, c client.Client, obj client.Object, hooks .
8282
if annotations == nil {
8383
annotations = map[string]string{}
8484
}
85-
annotations[runtimehooksv1.PendingHooksAnnotation] = removeFromCommaSeparatedList(annotations[runtimehooksv1.PendingHooksAnnotation], hookNames...)
86-
if annotations[runtimehooksv1.PendingHooksAnnotation] == "" {
87-
delete(annotations, runtimehooksv1.PendingHooksAnnotation)
85+
annotations[runtimev1.PendingHooksAnnotation] = removeFromCommaSeparatedList(annotations[runtimev1.PendingHooksAnnotation], hookNames...)
86+
if annotations[runtimev1.PendingHooksAnnotation] == "" {
87+
delete(annotations, runtimev1.PendingHooksAnnotation)
8888
}
8989
obj.SetAnnotations(annotations)
9090

‎internal/hooks/tracking_test.go‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -26,47 +26,48 @@ import (
2626
"sigs.k8s.io/controller-runtime/pkg/client"
2727
"sigs.k8s.io/controller-runtime/pkg/client/fake"
2828

29+
runtimev1 "sigs.k8s.io/cluster-api/exp/runtime/api/v1alpha1"
2930
runtimehooksv1 "sigs.k8s.io/cluster-api/exp/runtime/hooks/api/v1alpha1"
3031
runtimecatalog "sigs.k8s.io/cluster-api/internal/runtime/catalog"
3132
)
3233

33-
func TestIsMarked(t *testing.T) {
34+
func TestIsPending(t *testing.T) {
3435
tests := []struct {
3536
name string
3637
obj client.Object
3738
hook runtimecatalog.Hook
3839
want bool
3940
}{
4041
{
41-
name: "should return true if the hook is marked",
42+
name: "should return true if the hook is marked as pending",
4243
obj: &corev1.ConfigMap{
4344
ObjectMeta: metav1.ObjectMeta{
4445
Name: "test-cluster",
4546
Namespace: "test-ns",
4647
Annotations: map[string]string{
47-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
48+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
4849
},
4950
},
5051
},
5152
hook: runtimehooksv1.AfterClusterUpgrade,
5253
want: true,
5354
},
5455
{
55-
name: "should return true if the hook is marked - other hooks are marked too",
56+
name: "should return true if the hook is marked - other hooks are marked as pending too",
5657
obj: &corev1.ConfigMap{
5758
ObjectMeta: metav1.ObjectMeta{
5859
Name: "test-cluster",
5960
Namespace: "test-ns",
6061
Annotations: map[string]string{
61-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade,AfterControlPlaneUpgrade",
62+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade,AfterControlPlaneUpgrade",
6263
},
6364
},
6465
},
6566
hook: runtimehooksv1.AfterClusterUpgrade,
6667
want: true,
6768
},
6869
{
69-
name: "should return false if the hook is not marked",
70+
name: "should return false if the hook is not marked as pending",
7071
obj: &corev1.ConfigMap{
7172
ObjectMeta: metav1.ObjectMeta{
7273
Name: "test-cluster",
@@ -77,13 +78,13 @@ func TestIsMarked(t *testing.T) {
7778
want: false,
7879
},
7980
{
80-
name: "should return false if the hook is not marked - other hooks are marked",
81+
name: "should return false if the hook is not marked - other hooks are marked as pending",
8182
obj: &corev1.ConfigMap{
8283
ObjectMeta: metav1.ObjectMeta{
8384
Name: "test-cluster",
8485
Namespace: "test-ns",
8586
Annotations: map[string]string{
86-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
87+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
8788
},
8889
},
8990
},
@@ -100,14 +101,14 @@ func TestIsMarked(t *testing.T) {
100101
}
101102
}
102103

103-
func TestMark(t *testing.T) {
104+
func TestMarkAsPending(t *testing.T) {
104105
tests := []struct {
105106
name string
106107
obj client.Object
107108
hook runtimecatalog.Hook
108109
}{
109110
{
110-
name: "should add the marker if not already present",
111+
name: "should add the marker if not already marked as pending",
111112
obj: &corev1.ConfigMap{
112113
ObjectMeta: metav1.ObjectMeta{
113114
Name: "test-cluster",
@@ -117,26 +118,26 @@ func TestMark(t *testing.T) {
117118
hook: runtimehooksv1.AfterClusterUpgrade,
118119
},
119120
{
120-
name: "should add the marker if not already present - other hooks are present",
121+
name: "should add the marker if not already marked as pending - other hooks are present",
121122
obj: &corev1.ConfigMap{
122123
ObjectMeta: metav1.ObjectMeta{
123124
Name: "test-cluster",
124125
Namespace: "test-ns",
125126
Annotations: map[string]string{
126-
runtimehooksv1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
127+
runtimev1.PendingHooksAnnotation: "AfterControlPlaneUpgrade",
127128
},
128129
},
129130
},
130131
hook: runtimehooksv1.AfterClusterUpgrade,
131132
},
132133
{
133-
name: "should pass if the marker is already present",
134+
name: "should pass if the marker is already marked as pending",
134135
obj: &corev1.ConfigMap{
135136
ObjectMeta: metav1.ObjectMeta{
136137
Name: "test-cluster",
137138
Namespace: "test-ns",
138139
Annotations: map[string]string{
139-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
140+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
140141
},
141142
},
142143
},
@@ -151,12 +152,12 @@ func TestMark(t *testing.T) {
151152
ctx := context.Background()
152153
g.Expect(MarkAsPending(ctx, fakeClient, tt.obj, tt.hook)).To(Succeed())
153154
annotations := tt.obj.GetAnnotations()
154-
g.Expect(annotations[runtimehooksv1.PendingHooksAnnotation]).To(ContainSubstring(runtimecatalog.HookName(tt.hook)))
155+
g.Expect(annotations[runtimev1.PendingHooksAnnotation]).To(ContainSubstring(runtimecatalog.HookName(tt.hook)))
155156
})
156157
}
157158
}
158159

159-
func TestUnmark(t *testing.T) {
160+
func TestMarkAsDone(t *testing.T) {
160161
tests := []struct {
161162
name string
162163
obj client.Object
@@ -179,7 +180,7 @@ func TestUnmark(t *testing.T) {
179180
Name: "test-cluster",
180181
Namespace: "test-ns",
181182
Annotations: map[string]string{
182-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade",
183+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade",
183184
},
184185
},
185186
},
@@ -192,7 +193,7 @@ func TestUnmark(t *testing.T) {
192193
Name: "test-cluster",
193194
Namespace: "test-ns",
194195
Annotations: map[string]string{
195-
runtimehooksv1.PendingHooksAnnotation: "AfterClusterUpgrade,AfterControlPlaneUpgrade",
196+
runtimev1.PendingHooksAnnotation: "AfterClusterUpgrade,AfterControlPlaneUpgrade",
196197
},
197198
},
198199
},
@@ -207,7 +208,7 @@ func TestUnmark(t *testing.T) {
207208
ctx := context.Background()
208209
g.Expect(MarkAsDone(ctx, fakeClient, tt.obj, tt.hook)).To(Succeed())
209210
annotations := tt.obj.GetAnnotations()
210-
g.Expect(annotations[runtimehooksv1.PendingHooksAnnotation]).NotTo(ContainSubstring(runtimecatalog.HookName(tt.hook)))
211+
g.Expect(annotations[runtimev1.PendingHooksAnnotation]).NotTo(ContainSubstring(runtimecatalog.HookName(tt.hook)))
211212
})
212213
}
213214
}

0 commit comments

Comments
 (0)