Add API conventions, fix status fields

Signed-off-by: Stefan Bueringer [email protected]

Co-authored-by: fabriziopandini <[email protected]>

CONTRIBUTING.md

@@ -252,6 +252,71 @@ There may, at times, need to be exceptions where breaking changes are allowed in
 discretion of the project's maintainers, and must be carefully considered before merging. An example of an allowed
 breaking change might be a fix for a behavioral bug that was released in an initial minor version (such as `v0.3.0`).
 
+
+## API conventions
+
+In general we adhere to the [Kubernetes API conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#optional-vs-required).
+We have a small set of exceptions due to the fact that we're using CRDs while the API conventions predates the introduction 
+of CRDs and the way our controllers only patch actual changes to the status instead of updating the entire status in every 
+reconciliation.
+
+### Optional vs. Required
+
+Status fields must be optional, because our controllers are patching selected fields instead of updating the entire status 
+in every reconciliation.
+
+Optional fields have the following properties:
+* In general an optional field must have both an `+optional` marker and an `omitempty` JSON tag.
+* If the semantic difference between nil and the zero value is important for your code, the field must also be a pointer.
+  * Note: This doesn't apply to map or slice types as they already have a built-in `nil` value.
+  * Example: When using ClusterClass, the semantic difference is important when you have a field in a template which will 
+    have instance-specific different values in derived objects. Because in this case it's possible to set the field to `nil`
+    in the template and then the value can be set in derived objects without being overwritten by the cluster topology controller.
+
+* Exceptions:
+  * Fields in root objects should be kept as scaffolded by kubebuilder, e.g.:
+    ```golang
+    type Machine struct {
+      metav1.TypeMeta   `json:",inline"`
+      metav1.ObjectMeta `json:"metadata,omitempty"`
+
+      Spec   MachineSpec   `json:"spec,omitempty"`
+      Status MachineStatus `json:"status,omitempty"`
+    }
+    type MachineList struct {
+      metav1.TypeMeta `json:",inline"`
+      metav1.ListMeta `json:"metadata,omitempty"`
+      Items           []Machine `json:"items"`
+    }
+    ```
+  * Top-level fields in `status` must always have the `+optional` annotation. If we want the field to be always visible even if it 
+    has the zero value, it must **not** have the `omitempty` JSON tag, e.g.:
+    * Replica counters like `availableReplicas` in the `MachineDeployment`
+    * Flags expressing progress in the object lifecycle like `infrastructureReady` in `Machine`
+
+### CRD additionalPrinterColumns
+
+All our CRD objects should have the following `additionalPrinterColumns` order (if the respective field exists in the CRD):
+* Namespace (added automatically)
+* Name (added automatically)
+* Cluster
+* Other fields
+* Replica-related fields
+* Phase
+* Age (mandatory field for all CRDs)
+* Version
+* Other fields for -o wide (fields with priority `1` are only shown with `-o wide` and not per default)
+
+Examples:
+```bash
+$ kubectl get kubeadmcontrolplane
+NAMESPACE            NAME                               INITIALIZED   API SERVER AVAILABLE   REPLICAS   READY   UPDATED   UNAVAILABLE   AGE     VERSION
+quick-start-d5ufye   quick-start-ntysk0-control-plane   true          true                   1          1       1                       2m44s   v1.22.0
+$ kubectl get machinedeployment
+NAMESPACE            NAME                      CLUSTER              REPLICAS   READY   UPDATED   UNAVAILABLE   PHASE       AGE     VERSION
+quick-start-d5ufye   quick-start-ntysk0-md-0   quick-start-ntysk0   1                  1         1             ScalingUp   3m28s   v1.22.0
+```
+
 ## Google Doc Viewing Permissions
 
 To gain viewing permissions to google docs in this project, please join either the

api/v1beta1/cluster_types.go

@@ -49,7 +49,7 @@ type ClusterSpec struct {
 
 	// ControlPlaneEndpoint represents the endpoint used to communicate with the control plane.
 	// +optional
-	ControlPlaneEndpoint APIEndpoint `json:"controlPlaneEndpoint"`
+	ControlPlaneEndpoint APIEndpoint `json:"controlPlaneEndpoint,omitempty"`
 
 	// ControlPlaneRef is an optional reference to a provider-specific resource that holds
 	// the details for provisioning the Control Plane for a Cluster.
@@ -84,7 +84,7 @@ type Topology struct {
 
 	// ControlPlane describes the cluster control plane.
 	// +optional
-	ControlPlane ControlPlaneTopology `json:"controlPlane"`
+	ControlPlane ControlPlaneTopology `json:"controlPlane,omitempty"`
 
 	// Workers encapsulates the different constructs that form the worker nodes
 	// for the cluster.
@@ -217,7 +217,7 @@ type ClusterStatus struct {
 
 	// ControlPlaneReady defines if the control plane is ready.
 	// +optional
-	ControlPlaneReady bool `json:"controlPlaneReady,omitempty"`
+	ControlPlaneReady bool `json:"controlPlaneReady"`
 
 	// Conditions defines current service state of the cluster.
 	// +optional
@@ -432,7 +432,7 @@ func (in FailureDomains) GetIDs() []*string {
 type FailureDomainSpec struct {
 	// ControlPlane determines if this failure domain is suitable for use by control plane machines.
 	// +optional
-	ControlPlane bool `json:"controlPlane"`
+	ControlPlane bool `json:"controlPlane,omitempty"`
 
 	// Attributes is a free form map of attributes an infrastructure provider might use or require.
 	// +optional

api/v1beta1/machinedeployment_types.go

@@ -191,29 +191,29 @@ type MachineDeploymentStatus struct {
 	// Total number of non-terminated machines targeted by this deployment
 	// (their labels match the selector).
 	// +optional
-	Replicas int32 `json:"replicas,omitempty"`
+	Replicas int32 `json:"replicas"`
 
 	// Total number of non-terminated machines targeted by this deployment
 	// that have the desired template spec.
 	// +optional
-	UpdatedReplicas int32 `json:"updatedReplicas,omitempty"`
+	UpdatedReplicas int32 `json:"updatedReplicas"`
 
 	// Total number of ready machines targeted by this deployment.
 	// +optional
-	ReadyReplicas int32 `json:"readyReplicas,omitempty"`
+	ReadyReplicas int32 `json:"readyReplicas"`
 
 	// Total number of available machines (ready for at least minReadySeconds)
 	// targeted by this deployment.
 	// +optional
-	AvailableReplicas int32 `json:"availableReplicas,omitempty"`
+	AvailableReplicas int32 `json:"availableReplicas"`
 
 	// Total number of unavailable machines targeted by this deployment.
 	// This is the total number of machines that are still required for
 	// the deployment to have 100% available capacity. They may either
 	// be machines that are running but not yet available or machines
 	// that still have not been created.
 	// +optional
-	UnavailableReplicas int32 `json:"unavailableReplicas,omitempty"`
+	UnavailableReplicas int32 `json:"unavailableReplicas"`
 
 	// Phase represents the current phase of a MachineDeployment (ScalingUp, ScalingDown, Running, Failed, or Unknown).
 	// +optional

api/v1beta1/machinehealthcheck_types.go

@@ -99,18 +99,18 @@ type MachineHealthCheckStatus struct {
 	// total number of machines counted by this machine health check
 	// +kubebuilder:validation:Minimum=0
 	// +optional
-	ExpectedMachines int32 `json:"expectedMachines,omitempty"`
+	ExpectedMachines int32 `json:"expectedMachines"`
 
 	// total number of healthy machines counted by this machine health check
 	// +kubebuilder:validation:Minimum=0
 	// +optional
-	CurrentHealthy int32 `json:"currentHealthy,omitempty"`
+	CurrentHealthy int32 `json:"currentHealthy"`
 
 	// RemediationsAllowed is the number of further remediations allowed by this machine health check before
 	// maxUnhealthy short circuiting will be applied
 	// +kubebuilder:validation:Minimum=0
 	// +optional
-	RemediationsAllowed int32 `json:"remediationsAllowed,omitempty"`
+	RemediationsAllowed int32 `json:"remediationsAllowed"`
 
 	// ObservedGeneration is the latest generation observed by the controller.
 	// +optional

api/v1beta1/machineset_types.go

@@ -124,19 +124,19 @@ type MachineSetStatus struct {
 
 	// Replicas is the most recently observed number of replicas.
 	// +optional
-	Replicas int32 `json:"replicas,omitempty"`
+	Replicas int32 `json:"replicas"`
 
 	// The number of replicas that have labels matching the labels of the machine template of the MachineSet.
 	// +optional
-	FullyLabeledReplicas int32 `json:"fullyLabeledReplicas,omitempty"`
+	FullyLabeledReplicas int32 `json:"fullyLabeledReplicas"`
 
 	// The number of ready replicas for this MachineSet. A machine is considered ready when the node has been created and is "Ready".
 	// +optional
-	ReadyReplicas int32 `json:"readyReplicas,omitempty"`
+	ReadyReplicas int32 `json:"readyReplicas"`
 
 	// The number of available replicas (ready for at least minReadySeconds) for this MachineSet.
 	// +optional
-	AvailableReplicas int32 `json:"availableReplicas,omitempty"`
+	AvailableReplicas int32 `json:"availableReplicas"`
 
 	// ObservedGeneration reflects the generation of the most recently observed MachineSet.
 	// +optional

bootstrap/kubeadm/api/v1beta1/kubeadmconfig_types.go

@@ -100,7 +100,7 @@ type KubeadmConfigSpec struct {
 type KubeadmConfigStatus struct {
 	// Ready indicates the BootstrapData field is ready to be consumed
 	// +optional
-	Ready bool `json:"ready,omitempty"`
+	Ready bool `json:"ready"`
 
 	// DataSecretName is the name of the secret that stores the bootstrap data script.
 	// +optional

controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go

@@ -142,7 +142,7 @@ type KubeadmControlPlaneStatus struct {
 	// Total number of non-terminated machines targeted by this control plane
 	// (their labels match the selector).
 	// +optional
-	Replicas int32 `json:"replicas,omitempty"`
+	Replicas int32 `json:"replicas"`
 
 	// Version represents the minimum Kubernetes version for the control plane machines
 	// in the cluster.
@@ -152,19 +152,19 @@ type KubeadmControlPlaneStatus struct {
 	// Total number of non-terminated machines targeted by this control plane
 	// that have the desired template spec.
 	// +optional
-	UpdatedReplicas int32 `json:"updatedReplicas,omitempty"`
+	UpdatedReplicas int32 `json:"updatedReplicas"`
 
 	// Total number of fully running and ready control plane machines.
 	// +optional
-	ReadyReplicas int32 `json:"readyReplicas,omitempty"`
+	ReadyReplicas int32 `json:"readyReplicas"`
 
 	// Total number of unavailable machines targeted by this control plane.
 	// This is the total number of machines that are still required for
 	// the deployment to have 100% available capacity. They may either
 	// be machines that are running but not yet ready or machines
 	// that still have not been created.
 	// +optional
-	UnavailableReplicas int32 `json:"unavailableReplicas,omitempty"`
+	UnavailableReplicas int32 `json:"unavailableReplicas"`
 
 	// Initialized denotes whether or not the control plane has the
 	// uploaded kubeadm-config configmap.

test/infrastructure/docker/api/v1beta1/dockercluster_types.go

@@ -71,10 +71,12 @@ type ImageMeta struct {
 // DockerClusterStatus defines the observed state of DockerCluster.
 type DockerClusterStatus struct {
 	// Ready denotes that the docker cluster (infrastructure) is ready.
+	// +optional
 	Ready bool `json:"ready"`
 
 	// FailureDomains don't mean much in CAPD since it's all local, but we can see how the rest of cluster API
 	// will use this if we populate it.
+	// +optional
 	FailureDomains clusterv1.FailureDomains `json:"failureDomains,omitempty"`
 
 	// Conditions defines current service state of the DockerCluster.

test/infrastructure/docker/api/v1beta1/dockermachine_types.go

@@ -79,7 +79,7 @@ type DockerMachineStatus struct {
 	// LoadBalancerConfigured denotes that the machine has been
 	// added to the load balancer
 	// +optional
-	LoadBalancerConfigured bool `json:"loadBalancerConfigured,omitempty"`
+	LoadBalancerConfigured bool `json:"loadBalancerConfigured"`
 
 	// Addresses contains the associated addresses for the docker machine.
 	// +optional

test/infrastructure/docker/config/crd/bases/infrastructure.cluster.x-k8s.io_dockerclusters.yaml

@@ -461,8 +461,6 @@ spec:
                 description: Ready denotes that the docker cluster (infrastructure)
                   is ready.
                 type: boolean
-            required:
-            - ready
             type: object
         type: object
     served: true