Skip to content

⚠️ Change type of int32 fields with valid zero value to *int32 #12424

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jul 2, 2025
Merged

⚠️ Change type of int32 fields with valid zero value to *int32 #12424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b68c4f6
Change type of int32 fields with valid zero value to *int32
sbueringer Jun 30, 2025
bf8a7e2
Fix review findings
sbueringer Jul 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions api/controlplane/kubeadm/v1beta1/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,17 @@ func (src *KubeadmControlPlane) ConvertTo(dstRaw conversion.Hub) error {
bootstrapv1beta1.RestoreKubeadmConfigSpec(&restored.Spec.KubeadmConfigSpec, &dst.Spec.KubeadmConfigSpec)
}

if src.Spec.RemediationStrategy != nil {
if dst.Spec.RemediationStrategy == nil {
dst.Spec.RemediationStrategy = &controlplanev1.RemediationStrategy{}
}
var restoredRetryPeriodSeconds *int32
if restored.Spec.RemediationStrategy != nil {
restoredRetryPeriodSeconds = restored.Spec.RemediationStrategy.RetryPeriodSeconds
}
clusterv1.Convert_Duration_To_Pointer_int32(src.Spec.RemediationStrategy.RetryPeriod, ok, restoredRetryPeriodSeconds, &dst.Spec.RemediationStrategy.RetryPeriodSeconds)
}

// Override restored data with timeouts values already existing in v1beta1 but in other structs.
src.Spec.KubeadmConfigSpec.ConvertTo(&dst.Spec.KubeadmConfigSpec)
return nil
Expand Down Expand Up @@ -133,6 +144,17 @@ func (src *KubeadmControlPlaneTemplate) ConvertTo(dstRaw conversion.Hub) error {
bootstrapv1beta1.RestoreKubeadmConfigSpec(&restored.Spec.Template.Spec.KubeadmConfigSpec, &dst.Spec.Template.Spec.KubeadmConfigSpec)
}

if src.Spec.Template.Spec.RemediationStrategy != nil {
if dst.Spec.Template.Spec.RemediationStrategy == nil {
dst.Spec.Template.Spec.RemediationStrategy = &controlplanev1.RemediationStrategy{}
}
var restoredRetryPeriodSeconds *int32
if restored.Spec.Template.Spec.RemediationStrategy != nil {
restoredRetryPeriodSeconds = restored.Spec.Template.Spec.RemediationStrategy.RetryPeriodSeconds
}
clusterv1.Convert_Duration_To_Pointer_int32(src.Spec.Template.Spec.RemediationStrategy.RetryPeriod, ok, restoredRetryPeriodSeconds, &dst.Spec.Template.Spec.RemediationStrategy.RetryPeriodSeconds)
}

// Override restored data with timeouts values already existing in v1beta1 but in other structs.
src.Spec.Template.Spec.KubeadmConfigSpec.ConvertTo(&dst.Spec.Template.Spec.KubeadmConfigSpec)
return nil
Expand Down Expand Up @@ -281,7 +303,7 @@ func Convert_v1beta1_RemediationStrategy_To_v1beta2_RemediationStrategy(in *Reme
return err
}
out.MinHealthyPeriodSeconds = clusterv1.ConvertToSeconds(in.MinHealthyPeriod)
out.RetryPeriodSeconds = ptr.Deref(clusterv1.ConvertToSeconds(&in.RetryPeriod), 0)
out.RetryPeriodSeconds = clusterv1.ConvertToSeconds(&in.RetryPeriod)
return nil
}

Expand All @@ -290,7 +312,7 @@ func Convert_v1beta2_RemediationStrategy_To_v1beta1_RemediationStrategy(in *cont
return err
}
out.MinHealthyPeriod = clusterv1.ConvertFromSeconds(in.MinHealthyPeriodSeconds)
out.RetryPeriod = ptr.Deref(clusterv1.ConvertFromSeconds(&in.RetryPeriodSeconds), metav1.Duration{})
out.RetryPeriod = ptr.Deref(clusterv1.ConvertFromSeconds(in.RetryPeriodSeconds), metav1.Duration{})
return nil
}

Expand Down
8 changes: 4 additions & 4 deletions api/controlplane/kubeadm/v1beta2/kubeadm_control_plane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,7 +583,7 @@ type RemediationStrategy struct {
// If not set, a retry will happen immediately.
// +optional
// +kubebuilder:validation:Minimum=0
RetryPeriodSeconds int32 `json:"retryPeriodSeconds,omitempty"`
RetryPeriodSeconds *int32 `json:"retryPeriodSeconds,omitempty"`

// minHealthyPeriodSeconds defines the duration after which KCP will consider any failure to a machine unrelated
// from the previous one. In this case the remediation is not considered a retry anymore, and thus the retry
Expand Down Expand Up @@ -740,14 +740,14 @@ type KubeadmControlPlaneV1Beta1DeprecatedStatus struct {
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
UpdatedReplicas int32 `json:"updatedReplicas"`
UpdatedReplicas int32 `json:"updatedReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed

// readyReplicas is the total number of fully running and ready control plane machines.
//
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
ReadyReplicas int32 `json:"readyReplicas"`
ReadyReplicas int32 `json:"readyReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed

// unavailableReplicas is the total number of unavailable machines targeted by this control plane.
// This is the total number of machines that are still required for
Expand All @@ -758,7 +758,7 @@ type KubeadmControlPlaneV1Beta1DeprecatedStatus struct {
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
UnavailableReplicas int32 `json:"unavailableReplicas"`
UnavailableReplicas int32 `json:"unavailableReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note, when we are doing noline:kubeapilinter in the future, it's better to use the regex based excludes so that we can exclude single messages rather than entire fields from being checked across all rules.

In this case since it's going away, this is fine though

Copy link
Member Author

@sbueringer sbueringer Jul 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but I thought there is less harm in a local nolint vs an exclude which might match other fields accidentally in this case (we have some of these fields with the same name elsewhere)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can scope at least to package level and field name, so provided there aren't fields with duplicated names (which there probably are for us) then that would work. Would have to double check if we can scope more succinctly

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in some cases it's the same CRD

}

// LastRemediationStatus stores info about last remediation performed.
Expand Down
5 changes: 5 additions & 0 deletions api/controlplane/kubeadm/v1beta2/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 18 additions & 2 deletions api/core/v1beta1/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -499,7 +499,22 @@ func (dst *MachineDeployment) ConvertFrom(srcRaw conversion.Hub) error {
func (src *MachineHealthCheck) ConvertTo(dstRaw conversion.Hub) error {
dst := dstRaw.(*clusterv1.MachineHealthCheck)

return Convert_v1beta1_MachineHealthCheck_To_v1beta2_MachineHealthCheck(src, dst, nil)
if err := Convert_v1beta1_MachineHealthCheck_To_v1beta2_MachineHealthCheck(src, dst, nil); err != nil {
return err
}

// Manually restore data.
restored := &clusterv1.MachineHealthCheck{}
ok, err := utilconversion.UnmarshalData(src, restored)
if err != nil {
return err
}

clusterv1.Convert_int32_To_Pointer_int32(src.Status.ExpectedMachines, ok, restored.Status.ExpectedMachines, &dst.Status.ExpectedMachines)
clusterv1.Convert_int32_To_Pointer_int32(src.Status.CurrentHealthy, ok, restored.Status.CurrentHealthy, &dst.Status.CurrentHealthy)
clusterv1.Convert_int32_To_Pointer_int32(src.Status.RemediationsAllowed, ok, restored.Status.RemediationsAllowed, &dst.Status.RemediationsAllowed)

return nil
}

func (dst *MachineHealthCheck) ConvertFrom(srcRaw conversion.Hub) error {
Expand All @@ -513,7 +528,8 @@ func (dst *MachineHealthCheck) ConvertFrom(srcRaw conversion.Hub) error {
}

dropEmptyStringsMachineHealthCheck(dst)
return nil

return utilconversion.MarshalData(src, dst)
}

func (src *MachinePool) ConvertTo(dstRaw conversion.Hub) error {
Expand Down
24 changes: 18 additions & 6 deletions api/core/v1beta1/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

32 changes: 32 additions & 0 deletions api/core/v1beta2/conversion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,3 +72,35 @@ func Convert_bool_To_Pointer_bool(in bool, hasRestored bool, restoredIn *bool, o
// Otherwise, if the value is true, convert to *true.
*out = ptr.To(true)
}

func Convert_int32_To_Pointer_int32(in int32, hasRestored bool, restoredIn *int32, out **int32) {
// If the value is 0, convert to *0 only if the value was *0 before (we know it was intentionally set to 0).
// In all the other cases we do not know if the value was intentionally set to 0, so convert to nil.
if in == 0 {
if hasRestored && restoredIn != nil && *restoredIn == 0 {
*out = ptr.To[int32](0)
return
}
*out = nil
return
}

// Otherwise, if the value is not 0, convert to *value.
*out = ptr.To(in)
}

func Convert_Duration_To_Pointer_int32(in metav1.Duration, hasRestored bool, restoredIn *int32, out **int32) {
// If the value is 0s, convert to *0 only if the value was *0 before (we know it was intentionally set to 0).
// In all the other cases we do not know if the value was intentionally set to 0, so convert to nil.
if in.Nanoseconds() == 0 {
if hasRestored && restoredIn != nil && *restoredIn == 0 {
*out = ptr.To[int32](0)
return
}
*out = nil
return
}

// Otherwise, if the value is not 0, convert to *value.
*out = ConvertToSeconds(&in)
}
144 changes: 144 additions & 0 deletions api/core/v1beta2/conversion_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,147 @@ func TestConvert_bool_To_Pointer_bool(t *testing.T) {
})
}
}

func TestConvert_int32_To_Pointer_int32(t *testing.T) {
testCases := []struct {
name string
in int32
hasRestored bool
restored *int32
wantOut *int32
}{
{
name: "when applying v1beta1, 0 should be converted to nil",
in: 0,
wantOut: nil,
},
{
name: "when applying v1beta1, value!=0 should be converted to *value",
in: 1,
wantOut: ptr.To[int32](1),
},
{
name: "when doing round trip, 0 should be converted to nil if not previously explicitly set to 0 (previously set to nil)",
in: 0,
hasRestored: true,
restored: nil,
wantOut: nil,
},
{
name: "when doing round trip, 0 should be converted to nil if not previously explicitly set to 0 (previously set to another value)",
in: 0,
hasRestored: true,
restored: ptr.To[int32](1),
wantOut: nil,
},
{
name: "when doing round trip, 0 should be converted to 0 if previously explicitly set to 0",
in: 0,
hasRestored: true,
restored: ptr.To[int32](0),
wantOut: ptr.To[int32](0),
},
{
name: "when doing round trip, value should be converted to *value (no matter of restored value is nil)",
in: 1,
hasRestored: true,
restored: nil,
wantOut: ptr.To[int32](1),
},
{
name: "when doing round trip, value should be converted to *value (no matter of restored value is not 0)",
in: 1,
hasRestored: true,
restored: ptr.To[int32](2),
wantOut: ptr.To[int32](1),
},
{
name: "when doing round trip, value should be converted to *value (no matter of restored value is 0)",
in: 1,
hasRestored: true,
restored: ptr.To[int32](0),
wantOut: ptr.To[int32](1),
},
}
for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

var out *int32
Convert_int32_To_Pointer_int32(tt.in, tt.hasRestored, tt.restored, &out)
g.Expect(out).To(Equal(tt.wantOut))
})
}
}

func TestConvert_Duration_To_Pointer_int32(t *testing.T) {
testCases := []struct {
name string
in metav1.Duration
hasRestored bool
restored *int32
wantOut *int32
}{
{
name: "when applying v1beta1, 0s should be converted to nil",
in: metav1.Duration{},
wantOut: nil,
},
{
name: "when applying v1beta1, value!=0s should be converted to *value",
in: metav1.Duration{Duration: 1 * time.Second},
wantOut: ptr.To[int32](1),
},
{
name: "when doing round trip, 0s should be converted to nil if not previously explicitly set to 0s (previously set to nil)",
in: metav1.Duration{},
hasRestored: true,
restored: nil,
wantOut: nil,
},
{
name: "when doing round trip, 0s should be converted to nil if not previously explicitly set to 0s (previously set to another value)",
in: metav1.Duration{},
hasRestored: true,
restored: ptr.To[int32](1),
wantOut: nil,
},
{
name: "when doing round trip, 0s should be converted to 0s if previously explicitly set to 0s",
in: metav1.Duration{},
hasRestored: true,
restored: ptr.To[int32](0),
wantOut: ptr.To[int32](0),
},
{
name: "when doing round trip, value should be converted to *value (no matter of restored value is nil)",
in: metav1.Duration{Duration: 1 * time.Second},
hasRestored: true,
restored: nil,
wantOut: ptr.To[int32](1),
},
{
name: "when doing round trip, value should be converted to *value (no matter of restored value is not 0s)",
in: metav1.Duration{Duration: 1 * time.Second},
hasRestored: true,
restored: ptr.To[int32](2),
wantOut: ptr.To[int32](1),
},
{
name: "when doing round trip, value should be converted to *value (no matter of restored value is 0s)",
in: metav1.Duration{Duration: 1 * time.Second},
hasRestored: true,
restored: ptr.To[int32](0),
wantOut: ptr.To[int32](1),
},
}
for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

var out *int32
Convert_Duration_To_Pointer_int32(tt.in, tt.hasRestored, tt.restored, &out)
g.Expect(out).To(Equal(tt.wantOut))
})
}
}
8 changes: 4 additions & 4 deletions api/core/v1beta2/machinedeployment_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,22 +492,22 @@ type MachineDeploymentV1Beta1DeprecatedStatus struct {
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
UpdatedReplicas int32 `json:"updatedReplicas"`
UpdatedReplicas int32 `json:"updatedReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed

// readyReplicas is the total number of ready machines targeted by this deployment.
//
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
ReadyReplicas int32 `json:"readyReplicas"`
ReadyReplicas int32 `json:"readyReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed

// availableReplicas is the total number of available machines (ready for at least minReadySeconds)
// targeted by this deployment.
//
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
AvailableReplicas int32 `json:"availableReplicas"`
AvailableReplicas int32 `json:"availableReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed

// unavailableReplicas is the total number of unavailable machines targeted by this deployment.
// This is the total number of machines that are still required for
Expand All @@ -518,7 +518,7 @@ type MachineDeploymentV1Beta1DeprecatedStatus struct {
// Deprecated: This field is deprecated and is going to be removed when support for v1beta1 will be dropped. Please see https://github.com/kubernetes-sigs/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md for more details.
//
// +optional
UnavailableReplicas int32 `json:"unavailableReplicas"`
UnavailableReplicas int32 `json:"unavailableReplicas"` //nolint:kubeapilinter // field will be removed when v1beta1 is removed
}

// ANCHOR_END: MachineDeploymentStatus
Expand Down
Loading