🌱 Make Cluster topology controlPlane optional

[sbueringer]

Signed-off-by: Stefan Büringer [email protected]

What this PR does / why we need it:

Let’s say you want to create a Cluster with the following topology:

spec:
  topology:
    class: my-cluster-class
    version: v1.22.0

This won’t work with kubectl, because according to the OpenAPI Schema topology.controlPlane is mandatory. When you disable the client-side OpenAPI schema validation with --validate=false it will work. Why?

• kubectl client-side does not block with validate==false
• somehow the server-side validation does not fail. I guess it’s because controlPlane is not a pointer
• The resulting Cluster looks like this:

spec:
  topology:
    class: my-cluster-class
    version: v1.22.0
    controlPlane:
      metadata: {}

This can be fixed by making the field optional, which kind of makes sense because it doesn't have to be provided by the user.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

[fabriziopandini]

I have no strong opinions, but TBH, I kind of like having

topology:
    class: my-cluster-class
    version: v1.22.0
    controlPlane: {}

Because the topology actually gets a control plane, even if the users does not specify any field for it when doing kubectl apply.

What instead I don't like Is to have

topology:
    controlPlane: 
        metadata: {}

when metadata are not provided, because they are optional (probably the same applies to md as well)

[sbueringer]

My point was only that it's a bit awkward from a user perspective to be forced to provide an empty controlPlane object. But I also don't have strong opinions about that.

[sbueringer]

I'm also not sure if omitempty would also make sense in that case. I guess then the empty controlPlane struct wouldn't be added.

EDIT: adding omitempty doesn't change anything

[vincepri]

@fabriziopandini What should we do here?

[imikushin]

What if the controlPlane is managed, for example? Then even an empty topology for it doesn't make sense.

[sbueringer]

I don't really have a strong opinion.

I just think if the struct is optional (and it is in any case, we don't need any values), then it's not a great UX to enforce that users provide an empty object. When I think about it a bit longer. I think we should maybe also make the field a pointer (same reason, because it's optional to provide it)

[sbueringer]

Looking at the API conventions about optional vs required and comparing them with our structs leads to a bunch of new questions: optional vs required
Optional fields:

• pointer type or have a built-in nil value (e.g. maps and slices).
• +optional tag
• In most cases, omitempty struct tag

Required fields:

• not a pointer type.
• no +optional tag.
• no omitempty struct tag.

I see a lot of required fields (non-pointer) which have omitempty and other fields have +optional and omitempty but are not a pointer.

[vincepri]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment