📖 Add API conventions

[sbueringer]

What this PR does / why we need it:
This PR adds API conventions to establish a standard for our APIs going forward. The main focus is optional fields.

Related PRs:

• 🌱 Remove +required marker #5302: Drops +required annotations, because they are not handled by controller-tools
• ⚠️ Make condition.lastTransitionTime required #5303 Drops omitempty for LastTransitionTime which makes the field required in OpenAPI Schema as intended
• 🌱 Add missing optional tag to omitempty fields #5301 Adds +optional to all optional fields which currently only have omitempty. This doesn't have any consequences as those fields already have been optional in the OpenAPI schema before (because of the omitempty)
• 🌱 Add omitempty to +optional fields, drop omitempty for some status fields #5305
  • Adds omitempty to some of our +optional fields
  • Removes omitempty from our object lifecycle boolean status fields and our replica counter status fields so they are always visible (e.g. via kubectl get)

Notes:

• this PR contains a few cherry-picks from other PRs for now to provide a full picture
• after this PR and the ones I've cherry-picked all optional fields have omitempty and +optional with the exception of:
  • boolean flags which signal progress in the object lifecycle and replica counters (as mentioned in CONTRIBUTING.md)

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

[enxebre]

The comment and definition for Replicas field for scalable resources i.e MachineDeployments, MachineSets, MachinePools seems misguiding.

I know we discussed this in the past. But since we are here probably worth using the chance to discuss before promoting to beta.
I think the comment "// This is a pointer to distinguish between explicit zero and unspecified." in replicas is outdated and misguiding. That's never the case since we are defaulting 1, at least for MachineSets and MachineDeployments. in MachinePools I I can't see default marker, so the comment seems even more misguiding.
I see no reason for replicas to be a pointer and I think the only reason this is marked as optional is that required pointers with default breaks client side validation e.g kubectl kubernetes-sigs/controller-tools#567.

I think all ours scalable resources replicas should probably be commented an specified as:
If some one can think of a reason to keep it as a pointer:

	// Number of desired machines. Defaults to 1.
	// +optional
	// +kubebuilder:default=1
         Replicas *int32 `json:"replicas,omitempty"`

Or even better:

	// Number of desired machines. Defaults to 1.
	// +kubebuilder:default=1
         Replicas int32 `json:"replicas,omitempty"`

[sbueringer]

@enxebre It's already enough if the field is either a pointer or it has omitempty or +optional to make it not required in OpenAPI schema. The combination of required in the schema + defaulting breaks client-side kubectl validation because the schema is validated without taking the defaulting into account.

I suspect it is a pointer so that client in controllers can explicitly not set it and an auto-scaler can take over (without the controller overwriting it again in the next reconciliation).

[sbueringer]

/assign @vincepri @CecileRobertMichon @fabriziopandini @enxebre @JoelSpeed

[enxebre]

@enxebre It's already enough if the field is either a pointer or it has omitempty or +optional to make it not required in OpenAPI schema. The combination of required in the schema + defaulting breaks client-side kubectl validation because the schema is validated without taking the defaulting into account.

Conceptually this field should no be optional, do we agree?
It's only marked optional because we are defaulting a pointer and as you mention otherwise it break client side validation.

So I think it should be required and defaulted with no pointer:

	// Number of desired machines. Defaults to 1.
	// +kubebuilder:default=1
         Replicas int32 `json:"replicas,omitempty"`

Or if we foresee an issue with that, at minimum we should drop the misguiding comment ("// This is a pointer to distinguish between explicit zero and unspecified.") and make sure we are consistent in MachinePools and add the default marker.

i.e:

	// Number of desired machines. Defaults to 1.
	// +optional
	// +kubebuilder:default=1
         Replicas *int32 `json:"replicas,omitempty"`

I suspect it is a pointer so that client in controllers can explicitly not set it and an auto-scaler can take over (without the controller overwriting it again in the next reconciliation).

Not sure I can think of case where the above would be problematic.
Anyway may be this topic is easier to discuss synchronously :)

[fabriziopandini]

thanks for writing this up
/lgtm

[sbueringer]

/assign @randomvariable
I assume you're probably also interested in this.

[CecileRobertMichon]

/lgtm

[fabriziopandini]

/lgtm

[killianmuldoon]

/lgtm

[JoelSpeed]

/lgtm

[fabriziopandini]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 [fabriziopandini]

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