Skip to content
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

🌱 Make Cluster topology controlPlane optional #5165

Merged
merged 1 commit into from
Sep 6, 2021
Merged

🌱 Make Cluster topology controlPlane optional #5165

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions api/v1alpha4/cluster_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ type Topology struct {
RolloutAfter *metav1.Time `json:"rolloutAfter,omitempty"`

// ControlPlane describes the cluster control plane.
// +optional
Copy link
Member

@fabriziopandini fabriziopandini Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@sbueringer sbueringer Aug 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

@fabriziopandini What should we do here?

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@sbueringer sbueringer Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@sbueringer sbueringer Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

ControlPlane ControlPlaneTopology `json:"controlPlane"`

// Workers encapsulates the different constructs that form the worker nodes
Expand Down
1 change: 0 additions & 1 deletion config/crd/bases/cluster.x-k8s.io_clusters.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,6 @@ spec:
type: object
required:
- class
- controlPlane
- version
type: object
type: object
Expand Down