Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 42 additions & 0 deletions api/v1beta1/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,48 @@ func (dst *MachineHealthCheck) ConvertFrom(srcRaw conversion.Hub) error {
return Convert_v1beta2_MachineHealthCheck_To_v1beta1_MachineHealthCheck(src, dst, nil)
}

func Convert_v1beta2_ClusterClassSpec_To_v1beta1_ClusterClassSpec(in *clusterv1.ClusterClassSpec, out *ClusterClassSpec, s apimachineryconversion.Scope) error {
if err := autoConvert_v1beta2_ClusterClassSpec_To_v1beta1_ClusterClassSpec(in, out, s); err != nil {
return err
}

if in.Infrastructure.NamingStrategy != nil {
out.InfrastructureNamingStrategy = &InfrastructureNamingStrategy{
Template: in.Infrastructure.NamingStrategy.Template,
}
}
return nil
}

func Convert_v1beta2_InfrastructureClass_To_v1beta1_LocalObjectTemplate(in *clusterv1.InfrastructureClass, out *LocalObjectTemplate, s apimachineryconversion.Scope) error {
if in == nil {
return nil
}

return autoConvert_v1beta2_LocalObjectTemplate_To_v1beta1_LocalObjectTemplate(&in.LocalObjectTemplate, out, s)
}

func Convert_v1beta1_ClusterClassSpec_To_v1beta2_ClusterClassSpec(in *ClusterClassSpec, out *clusterv1.ClusterClassSpec, s apimachineryconversion.Scope) error {
if err := autoConvert_v1beta1_ClusterClassSpec_To_v1beta2_ClusterClassSpec(in, out, s); err != nil {
return err
}

if in.InfrastructureNamingStrategy != nil {
out.Infrastructure.NamingStrategy = &clusterv1.InfrastructureClassNamingStrategy{
Template: in.InfrastructureNamingStrategy.Template,
}
}
return nil
}

func Convert_v1beta1_LocalObjectTemplate_To_v1beta2_InfrastructureClass(in *LocalObjectTemplate, out *clusterv1.InfrastructureClass, s apimachineryconversion.Scope) error {
if in == nil {
return nil
}

return autoConvert_v1beta1_LocalObjectTemplate_To_v1beta2_LocalObjectTemplate(in, &out.LocalObjectTemplate, s)
}

func Convert_v1beta2_ClusterClassStatus_To_v1beta1_ClusterClassStatus(in *clusterv1.ClusterClassStatus, out *ClusterClassStatus, s apimachineryconversion.Scope) error {
if err := autoConvert_v1beta2_ClusterClassStatus_To_v1beta1_ClusterClassStatus(in, out, s); err != nil {
return err
Expand Down
77 changes: 23 additions & 54 deletions api/v1beta1/zz_generated.conversion.go

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

29 changes: 16 additions & 13 deletions api/v1beta2/clusterclass_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,10 @@ type ClusterClassSpec struct {
// +kubebuilder:validation:MaxItems=32
AvailabilityGates []ClusterAvailabilityGate `json:"availabilityGates,omitempty"`

// infrastructure is a reference to a provider-specific template that holds
// the details for provisioning infrastructure specific cluster
// for the underlying provider.
// The underlying provider is responsible for the implementation
// of the template to an infrastructure cluster.
// infrastructure is a reference to a local struct that holds the details
// for provisioning the infrastructure cluster for the Cluster.
// +optional
Infrastructure LocalObjectTemplate `json:"infrastructure,omitempty"`

// infrastructureNamingStrategy allows changing the naming pattern used when creating the infrastructure object.
// +optional
InfrastructureNamingStrategy *InfrastructureNamingStrategy `json:"infrastructureNamingStrategy,omitempty"`
Infrastructure InfrastructureClass `json:"infrastructure,omitempty"`
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 know this should be a pointer, but I chose to be consistent with the controlPlane field below for now.

Added a sub-task to the v1beta2 umbrella issue to follow-up

Copy link
Contributor

Choose a reason for hiding this comment

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

We need to make sure that we correct this before we ship.

InfrastructureClass is a struct, and cannot be omitempty'd correctly.

LocalObjectTemplate is inlined and has a required field within.

The required field should not accept "", which means a structured client marshalling this right now creates an invalid object and cannot treat Infrastructure as truly optional

Copy link
Member Author

@sbueringer sbueringer May 15, 2025

Choose a reason for hiding this comment

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

I agree. I just don't want to create a different set of problems between this field and the ControlPlane field

Because even if we make the Infrastructure + ControlPlane fields a pointer it's still not perfect as the required Ref field in there is a pointer, so we have to change more to fix this all up

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, that's fine, is the umbrella issue tracking what's required and what's optional to be completed before the next release?

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently we only flagged must have (P0) and nice to have (P1) for the alpha (in 2-3 weeks)

Copy link
Member Author

@sbueringer sbueringer May 15, 2025

Choose a reason for hiding this comment

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

My personal goal would be to fix all these marshalling problems if somehow possible once the KAL linter is available.

Independent of that. I don't know if this should be a showstopper for the release, given we have the exact same situation for the existing ControPlane field

Copy link
Contributor

Choose a reason for hiding this comment

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

The optionalfields work for KAL I think is pretty close. Might be able to get that here next week, and at least it will highlight where we have marshalling issues. Fixes will still be partially manual/need to be discussed

Copy link
Member Author

Choose a reason for hiding this comment

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

If it helps for the current PR. I think the PR doesn't make it worse as we had the same problem on this field before? (probably? :))


// controlPlane is a reference to a local struct that holds the details
// for provisioning the Control Plane for the Cluster.
Expand All @@ -135,6 +128,16 @@ type ClusterClassSpec struct {
Patches []ClusterClassPatch `json:"patches,omitempty"`
}

// InfrastructureClass defines the class for the infrastructure cluster.
type InfrastructureClass struct {
// LocalObjectTemplate contains the reference to a provider-specific infrastructure cluster template.
LocalObjectTemplate `json:",inline"`

// namingStrategy allows changing the naming pattern used when creating the infrastructure cluster object.
// +optional
NamingStrategy *InfrastructureClassNamingStrategy `json:"namingStrategy,omitempty"`
}

// ControlPlaneClass defines the class for the control plane.
type ControlPlaneClass struct {
// metadata is the metadata applied to the ControlPlane and the Machines of the ControlPlane
Expand All @@ -147,7 +150,7 @@ type ControlPlaneClass struct {
// +optional
Metadata ObjectMeta `json:"metadata,omitempty"`

// LocalObjectTemplate contains the reference to the control plane provider.
// LocalObjectTemplate contains the reference to a provider-specific control plane template.
LocalObjectTemplate `json:",inline"`

// machineInfrastructure defines the metadata and infrastructure information
Expand Down Expand Up @@ -221,8 +224,8 @@ type ControlPlaneClassNamingStrategy struct {
Template *string `json:"template,omitempty"`
}

// InfrastructureNamingStrategy defines the naming strategy for infrastructure objects.
type InfrastructureNamingStrategy struct {
// InfrastructureClassNamingStrategy defines the naming strategy for infrastructure objects.
type InfrastructureClassNamingStrategy struct {
// template defines the template to use for generating the name of the Infrastructure object.
// If not defined, it will fallback to `{{ .cluster.name }}-{{ .random }}`.
// If the templated string exceeds 63 characters, it will be trimmed to 58 characters and will
Expand Down
34 changes: 25 additions & 9 deletions api/v1beta2/zz_generated.deepcopy.go

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

Loading
Loading