Skip to content

Commit 98e2300

Browse files
sbueringersbueringer
committed
Fix review findings
1 parent 667d9d0 commit 98e2300

17 files changed

+229
-58
lines changed

‎api/controlplane/kubeadm/v1beta1/conversion.go‎

Lines changed: 12 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -65,11 +65,15 @@ func (src *KubeadmControlPlane) ConvertTo(dstRaw conversion.Hub) error {
6565
bootstrapv1beta1.RestoreKubeadmConfigSpec(&restored.Spec.KubeadmConfigSpec, &dst.Spec.KubeadmConfigSpec)
6666
}
6767

68-
if src.Spec.RemediationStrategy != nil && restored.Spec.RemediationStrategy != nil {
68+
if src.Spec.RemediationStrategy != nil {
6969
if dst.Spec.RemediationStrategy == nil {
7070
dst.Spec.RemediationStrategy = &controlplanev1.RemediationStrategy{}
7171
}
72-
convert_Duration_To_Pointer_int32(src.Spec.RemediationStrategy.RetryPeriod, ok, restored.Spec.RemediationStrategy.RetryPeriodSeconds, &dst.Spec.RemediationStrategy.RetryPeriodSeconds)
72+
var restoredRetryPeriodSeconds *int32
73+
if restored.Spec.RemediationStrategy != nil {
74+
restoredRetryPeriodSeconds = restored.Spec.RemediationStrategy.RetryPeriodSeconds
75+
}
76+
clusterv1.Convert_Duration_To_Pointer_int32(src.Spec.RemediationStrategy.RetryPeriod, ok, restoredRetryPeriodSeconds, &dst.Spec.RemediationStrategy.RetryPeriodSeconds)
7377
}
7478

7579
// Override restored data with timeouts values already existing in v1beta1 but in other structs.
@@ -112,11 +116,15 @@ func (src *KubeadmControlPlaneTemplate) ConvertTo(dstRaw conversion.Hub) error {
112116
bootstrapv1beta1.RestoreKubeadmConfigSpec(&restored.Spec.Template.Spec.KubeadmConfigSpec, &dst.Spec.Template.Spec.KubeadmConfigSpec)
113117
}
114118

115-
if src.Spec.Template.Spec.RemediationStrategy != nil && restored.Spec.Template.Spec.RemediationStrategy != nil {
119+
if src.Spec.Template.Spec.RemediationStrategy != nil {
116120
if dst.Spec.Template.Spec.RemediationStrategy == nil {
117121
dst.Spec.Template.Spec.RemediationStrategy = &controlplanev1.RemediationStrategy{}
118122
}
119-
convert_Duration_To_Pointer_int32(src.Spec.Template.Spec.RemediationStrategy.RetryPeriod, ok, restored.Spec.Template.Spec.RemediationStrategy.RetryPeriodSeconds, &dst.Spec.Template.Spec.RemediationStrategy.RetryPeriodSeconds)
123+
var restoredRetryPeriodSeconds *int32
124+
if restored.Spec.Template.Spec.RemediationStrategy != nil {
125+
restoredRetryPeriodSeconds = restored.Spec.Template.Spec.RemediationStrategy.RetryPeriodSeconds
126+
}
127+
clusterv1.Convert_Duration_To_Pointer_int32(src.Spec.Template.Spec.RemediationStrategy.RetryPeriod, ok, restoredRetryPeriodSeconds, &dst.Spec.Template.Spec.RemediationStrategy.RetryPeriodSeconds)
120128
}
121129

122130
// Override restored data with timeouts values already existing in v1beta1 but in other structs.
@@ -351,19 +359,3 @@ func convertToObjectReference(ref *clusterv1.ContractVersionedObjectReference, n
351359
Name: ref.Name,
352360
}, nil
353361
}
354-
355-
func convert_Duration_To_Pointer_int32(in metav1.Duration, hasRestored bool, restoredIn *int32, out **int32) {
356-
// If the value is 0s, convert to *0 only if the value was *0 before (we know it was intentionally set to 0).
357-
// In all the other cases we do not know if the value was intentionally set to 0, so convert to nil.
358-
if in.Nanoseconds() == 0 {
359-
if hasRestored && restoredIn != nil && *restoredIn == 0 {
360-
*out = ptr.To[int32](0)
361-
return
362-
}
363-
*out = nil
364-
return
365-
}
366-
367-
// Otherwise, if the value is not 0, convert to *value.
368-
*out = clusterv1.ConvertToSeconds(&in)
369-
}

‎api/core/v1beta2/conversion.go‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,3 +72,19 @@ func Convert_int32_To_Pointer_int32(in int32, hasRestored bool, restoredIn *int3
7272
// Otherwise, if the value is not 0, convert to *value.
7373
*out = ptr.To(in)
7474
}
75+
76+
func Convert_Duration_To_Pointer_int32(in metav1.Duration, hasRestored bool, restoredIn *int32, out **int32) {
77+
// If the value is 0s, convert to *0 only if the value was *0 before (we know it was intentionally set to 0).
78+
// In all the other cases we do not know if the value was intentionally set to 0, so convert to nil.
79+
if in.Nanoseconds() == 0 {
80+
if hasRestored && restoredIn != nil && *restoredIn == 0 {
81+
*out = ptr.To[int32](0)
82+
return
83+
}
84+
*out = nil
85+
return
86+
}
87+
88+
// Otherwise, if the value is not 0, convert to *value.
89+
*out = ConvertToSeconds(&in)
90+
}

‎api/core/v1beta2/conversion_test.go‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,3 +113,75 @@ func TestConvert_int32_To_Pointer_int32(t *testing.T) {
113113
})
114114
}
115115
}
116+
117+
func TestConvert_Duration_To_Pointer_int32(t *testing.T) {
118+
testCases := []struct {
119+
name string
120+
in metav1.Duration
121+
hasRestored bool
122+
restored *int32
123+
wantOut *int32
124+
}{
125+
{
126+
name: "when applying v1beta1, 0s should be converted to nil",
127+
in: metav1.Duration{},
128+
wantOut: nil,
129+
},
130+
{
131+
name: "when applying v1beta1, value!=0s should be converted to *value",
132+
in: metav1.Duration{Duration: 1 * time.Second},
133+
wantOut: ptr.To[int32](1),
134+
},
135+
{
136+
name: "when doing round trip, 0s should be converted to nil if not previously explicitly set to 0s (previously set to nil)",
137+
in: metav1.Duration{},
138+
hasRestored: true,
139+
restored: nil,
140+
wantOut: nil,
141+
},
142+
{
143+
name: "when doing round trip, 0s should be converted to nil if not previously explicitly set to 0s (previously set to another value)",
144+
in: metav1.Duration{},
145+
hasRestored: true,
146+
restored: ptr.To[int32](1),
147+
wantOut: nil,
148+
},
149+
{
150+
name: "when doing round trip, 0s should be converted to 0s if previously explicitly set to 0s",
151+
in: metav1.Duration{},
152+
hasRestored: true,
153+
restored: ptr.To[int32](0),
154+
wantOut: ptr.To[int32](0),
155+
},
156+
{
157+
name: "when doing round trip, value should be converted to *value (no matter of restored value is nil)",
158+
in: metav1.Duration{Duration: 1 * time.Second},
159+
hasRestored: true,
160+
restored: nil,
161+
wantOut: ptr.To[int32](1),
162+
},
163+
{
164+
name: "when doing round trip, value should be converted to *value (no matter of restored value is not 0s)",
165+
in: metav1.Duration{Duration: 1 * time.Second},
166+
hasRestored: true,
167+
restored: ptr.To[int32](2),
168+
wantOut: ptr.To[int32](1),
169+
},
170+
{
171+
name: "when doing round trip, value should be converted to *value (no matter of restored value is 0s)",
172+
in: metav1.Duration{Duration: 1 * time.Second},
173+
hasRestored: true,
174+
restored: ptr.To[int32](0),
175+
wantOut: ptr.To[int32](1),
176+
},
177+
}
178+
for _, tt := range testCases {
179+
t.Run(tt.name, func(t *testing.T) {
180+
g := NewWithT(t)
181+
182+
var out *int32
183+
Convert_Duration_To_Pointer_int32(tt.in, tt.hasRestored, tt.restored, &out)
184+
g.Expect(out).To(Equal(tt.wantOut))
185+
})
186+
}
187+
}

‎internal/topology/upgrade/clusterctl_upgrade_test.go‎

Lines changed: 58 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,8 @@ const (
6565
omittableFieldsMustNotBeSet = false
6666
zeroMustBeSet = true
6767
zeroMustNotBeSet = false
68+
zeroSecondsMustBeSet = true
69+
zeroSecondsMustNotBeSet = false
6870
)
6971

7072
// allObjs can be used to look at all the objects / how they change across generation while debugging.
@@ -127,9 +129,10 @@ func TestAPIAndWebhookChanges(t *testing.T) {
127129
g.Eventually(func() error {
128130
cluster1Refs, err = getClusterTopologyReferences(cluster1, "v1beta1",
129131
checkOmittableFromPatchesField(omittableFieldsMustNotBeSet), // The defaulting webhook drops the omittable field when using v1beta1.
130-
checkPtrTypeToType(), // "" should never show up in the yaml ("" is not a valid value).
131-
checkTypeToPtrType(), // zero or false should never show up due to omitempty (mutating webhook is dropping omitempty values).
132-
checkTypeToOmitZeroType(zeroMustBeSet), // zero value should show up (no omitzero).
132+
checkPtrTypeToType(), // "" should never show up in the yaml ("" is not a valid value).
133+
checkTypeToPtrType(), // zero or false should never show up due to omitempty (mutating webhook is dropping omitempty values).
134+
checkDurationToPtrInt32(zeroSecondsMustBeSet), // metav1.Duration is marshalled as 0s.
135+
checkTypeToOmitZeroType(zeroMustBeSet), // zero value should show up (no omitzero).
133136
)
134137
if err != nil {
135138
return err
@@ -184,9 +187,10 @@ func TestAPIAndWebhookChanges(t *testing.T) {
184187
g.Eventually(func() error {
185188
cluster1RefsNew, err = getClusterTopologyReferences(cluster1, "v1beta2",
186189
checkOmittableFromPatchesField(omittableFieldsMustBeSet), // The defaulting webhook does not drop the omittable field anymore when using v1beta2.
187-
checkPtrTypeToType(), // "" should never show up in the yaml due to omitempty (we set to "" in conversion, but omitempty drops it from yaml).
188-
checkTypeToPtrType(), // zero or false should never show up (we drop zero or false on conversion, we assume implicitly set)
189-
checkTypeToOmitZeroType(zeroMustNotBeSet), // zero value must not show up (omitzero through conversion).
190+
checkPtrTypeToType(), // "" should never show up in the yaml due to omitempty (we set to "" in conversion, but omitempty drops it from yaml).
191+
checkTypeToPtrType(), // zero or false should never show up (we drop zero or false on conversion, we assume implicitly set)
192+
checkDurationToPtrInt32(zeroSecondsMustNotBeSet), // 0 should never show up (we drop 0 on conversion, we assume implicitly set)
193+
checkTypeToOmitZeroType(zeroMustNotBeSet), // zero value must not show up (omitzero through conversion).
190194
)
191195
if err != nil {
192196
return err
@@ -205,9 +209,10 @@ func TestAPIAndWebhookChanges(t *testing.T) {
205209
g.Eventually(func() error {
206210
cluster2Refs, err = getClusterTopologyReferences(cluster2, "v1beta2",
207211
checkOmittableFromPatchesField(omittableFieldsMustNotBeSet), // The conversion webhook drops the omittable field when using v1beta1.
208-
checkPtrTypeToType(), // "" should never show up in the yaml ("" is not a valid value).
209-
checkTypeToPtrType(), // zero or false should never show up (we drop zero or false on conversion, we assume implicitly set)
210-
checkTypeToOmitZeroType(zeroMustNotBeSet), // zero value must not show up (omitzero through conversion, also not a valid value).
212+
checkPtrTypeToType(), // "" should never show up in the yaml ("" is not a valid value).
213+
checkTypeToPtrType(), // zero or false should never show up (we drop zero or false on conversion, we assume implicitly set)
214+
checkDurationToPtrInt32(zeroSecondsMustNotBeSet), // 0 should never show up (we drop 0 on conversion, we assume implicitly set)
215+
checkTypeToOmitZeroType(zeroMustNotBeSet), // zero value must not show up (omitzero through conversion, also not a valid value).
211216
)
212217
if err != nil {
213218
return err
@@ -231,9 +236,10 @@ func TestAPIAndWebhookChanges(t *testing.T) {
231236
g.Eventually(func() error {
232237
cluster2RefsNew, err = getClusterTopologyReferences(cluster2, "v1beta2",
233238
checkOmittableFromPatchesField(omittableFieldsMustBeSet), // The defaulting webhook do not drop anymore the omittable field when using v1beta2.
234-
checkPtrTypeToType(), // "" should never show up in the yaml due to omitempty (also not a valid value).
235-
checkTypeToPtrType(), // zero or false should never show up (it will show up if someone explicitly set zero or false)
236-
checkTypeToOmitZeroType(zeroMustNotBeSet), // zero value must not show up (omitzero, also not a valid value).
239+
checkPtrTypeToType(), // "" should never show up in the yaml due to omitempty (also not a valid value).
240+
checkTypeToPtrType(), // zero or false should never show up (it will show up if someone explicitly set zero or false)
241+
checkDurationToPtrInt32(zeroSecondsMustNotBeSet), // 0 should never show up (it will show up if someone explicitly sets 0)
242+
checkTypeToOmitZeroType(zeroMustNotBeSet), // zero value must not show up (omitzero, also not a valid value).
237243
)
238244
if err != nil {
239245
return err
@@ -284,9 +290,10 @@ func createT1ClusterClass(g *WithT, ns *corev1.Namespace, ct1 client.Client) *cl
284290
Spec: testt1v1beta1.TestResourceTemplateSpec{
285291
Template: testt1v1beta1.TestResourceTemplateResource{
286292
Spec: testt1v1beta1.TestResourceSpec{
287-
BoolToPtrBool: true,
288-
PtrStringToString: ptr.To("Something"),
289-
Int32ToPtrInt32: int32(4),
293+
BoolToPtrBool: true,
294+
PtrStringToString: ptr.To("Something"),
295+
Int32ToPtrInt32: int32(4),
296+
DurationToPtrInt32: metav1.Duration{Duration: 5 * time.Second},
290297
StructWithOnlyOptionalFields: testt1v1beta1.StructWithOnlyOptionalFields{
291298
A: "Something",
292299
},
@@ -307,6 +314,7 @@ func createT1ClusterClass(g *WithT, ns *corev1.Namespace, ct1 client.Client) *cl
307314
BoolToPtrBool: false,
308315
PtrStringToString: nil,
309316
Int32ToPtrInt32: 0,
317+
DurationToPtrInt32: metav1.Duration{},
310318
StructWithOnlyOptionalFields: testt1v1beta1.StructWithOnlyOptionalFields{},
311319
},
312320
},
@@ -481,9 +489,10 @@ func createT2ClusterClass(g *WithT, ns *corev1.Namespace, ct2 client.Client) *cl
481489
Spec: testt2v1beta2.TestResourceTemplateSpec{
482490
Template: testt2v1beta2.TestResourceTemplateResource{
483491
Spec: testt2v1beta2.TestResourceSpec{
484-
BoolToPtrBool: ptr.To(true),
485-
PtrStringToString: "Something",
486-
Int32ToPtrInt32: ptr.To[int32](4),
492+
BoolToPtrBool: ptr.To(true),
493+
PtrStringToString: "Something",
494+
Int32ToPtrInt32: ptr.To[int32](4),
495+
DurationToPtrInt32: ptr.To[int32](5),
487496
StructWithOnlyOptionalFields: testt2v1beta2.StructWithOnlyOptionalFields{
488497
A: "Something",
489498
},
@@ -504,6 +513,7 @@ func createT2ClusterClass(g *WithT, ns *corev1.Namespace, ct2 client.Client) *cl
504513
Spec: testt2v1beta2.TestResourceSpec{
505514
BoolToPtrBool: nil,
506515
Int32ToPtrInt32: nil,
516+
DurationToPtrInt32: nil,
507517
StructWithOnlyOptionalFields: testt2v1beta2.StructWithOnlyOptionalFields{},
508518
},
509519
},
@@ -863,6 +873,36 @@ func checkTypeToPtrType() func(obj *unstructured.Unstructured) error {
863873
}
864874
}
865875

876+
func checkDurationToPtrInt32(mustBeSet bool) func(obj *unstructured.Unstructured) error {
877+
return func(obj *unstructured.Unstructured) error {
878+
switch obj.GetKind() {
879+
case "TestResource":
880+
value, exists, err := unstructured.NestedFieldCopy(obj.Object, "spec", "durationToPtrInt32")
881+
if err != nil {
882+
return err
883+
}
884+
if !mustBeSet && exists && (reflect.DeepEqual(value, "0s") || reflect.DeepEqual(value, int64(0))) {
885+
return errors.New("expected to not contain a 0s durationToPtrInt32 field")
886+
}
887+
if mustBeSet && !exists {
888+
return errors.New("expected to contain a durationToPtrInt32 field")
889+
}
890+
case "TestResourceTemplate":
891+
value, exists, err := unstructured.NestedFieldCopy(obj.Object, "spec", "template", "spec", "durationToPtrInt32")
892+
if err != nil {
893+
return err
894+
}
895+
if !mustBeSet && exists && (reflect.DeepEqual(value, "0s") || reflect.DeepEqual(value, int64(0))) {
896+
return errors.New("expected to not contain a 0s durationToPtrInt32 field")
897+
}
898+
if mustBeSet && !exists {
899+
return errors.New("expected to contain a durationToPtrInt32 field")
900+
}
901+
}
902+
return nil
903+
}
904+
}
905+
866906
func checkTypeToOmitZeroType(mustBeSet bool) func(obj *unstructured.Unstructured) error {
867907
return func(obj *unstructured.Unstructured) error {
868908
switch obj.GetKind() {

‎internal/topology/upgrade/test/t1/crd/test.cluster.x-k8s.io_testresources.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ spec:
4141
properties:
4242
boolToPtrBool:
4343
type: boolean
44+
durationToPtrInt32:
45+
type: string
4446
int32ToPtrInt32:
4547
format: int32
4648
minimum: 0

‎internal/topology/upgrade/test/t1/crd/test.cluster.x-k8s.io_testresourcetemplates.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ spec:
4848
properties:
4949
boolToPtrBool:
5050
type: boolean
51+
durationToPtrInt32:
52+
type: string
5153
int32ToPtrInt32:
5254
format: int32
5355
minimum: 0

‎internal/topology/upgrade/test/t1/v1beta1/types.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,9 @@ type TestResourceSpec struct {
105105
// +kubebuilder:validation:Minimum=0
106106
Int32ToPtrInt32 int32 `json:"int32ToPtrInt32,omitempty"`
107107

108+
// +optional
109+
DurationToPtrInt32 metav1.Duration `json:"durationToPtrInt32,omitempty"`
110+
108111
// +optional
109112
StructWithOnlyOptionalFields StructWithOnlyOptionalFields `json:"structWithOnlyOptionalFields,omitempty"`
110113
}

‎internal/topology/upgrade/test/t1/v1beta1/zz_generated.deepcopy.go‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎internal/topology/upgrade/test/t2/crd/test.cluster.x-k8s.io_testresources.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ spec:
4141
properties:
4242
boolToPtrBool:
4343
type: boolean
44+
durationToPtrInt32:
45+
type: string
4446
int32ToPtrInt32:
4547
format: int32
4648
minimum: 0
@@ -163,6 +165,10 @@ spec:
163165
properties:
164166
boolToPtrBool:
165167
type: boolean
168+
durationToPtrInt32:
169+
format: int32
170+
minimum: 0
171+
type: integer
166172
int32ToPtrInt32:
167173
format: int32
168174
minimum: 0

‎internal/topology/upgrade/test/t2/crd/test.cluster.x-k8s.io_testresourcetemplates.yaml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ spec:
4848
properties:
4949
boolToPtrBool:
5050
type: boolean
51+
durationToPtrInt32:
52+
type: string
5153
int32ToPtrInt32:
5254
format: int32
5355
minimum: 0
@@ -184,6 +186,10 @@ spec:
184186
properties:
185187
boolToPtrBool:
186188
type: boolean
189+
durationToPtrInt32:
190+
format: int32
191+
minimum: 0
192+
type: integer
187193
int32ToPtrInt32:
188194
format: int32
189195
minimum: 0

0 commit comments

Comments
 (0)