Skip to content

Update unions guidance to confrom with latest serialization guidance #2060

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

Open
JoelSpeed wants to merge 1 commit into
base: master
Choose a base branch
from
+11 −9
Open

Update unions guidance to confrom with latest serialization guidance #2060

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
20 changes: 11 additions & 9 deletions dev-guide/api-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,35 +346,37 @@ type MyPlatformConfig struct {
// should choose a sensible default on their behalf.
// +unionDiscriminator
// +kubebuilder:validation:Enum:="AWS";"Azure";"GCP"
// +kubebuilder:validation:Required
PlatformType string `json:"platformType"`
// +required

@patrickdillon patrickdillon Jul 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding dumb simple guidance (somewhere not necessarily here), like:

use omitempty for required fields and omitzero for optional (or union discriminator) members?

This may be obvious to others, but it wasn't obvious to me why we need omitempty for required fields.

Also it seems like there are many other examples in this document where a field is required but not omitempty, we should probably clean those up. For example:

// LocalDefabulatorReference references a defabulator.
type LocalDefabulatorReference struct {
	// name is the metadata.name of the referenced defabulator object.
	// +kubebuilder:validation:Required
	// +required
	Name string `json:"name"`
}

This should be omitempty right? There are at least two or three more like this. If scope is creeping we can follow up, but LMK if my understanding is correct.

@everettraven everettraven Jul 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, we would push for that field to have omitempty and validations that don't allow the value to be the empty string. The exception to this would be if the empty string is a valid value.

I agree that we should probably do a sweeping update of the document to update all the examples, but I'm also okay with incremental updates if we'd like to keep this tightly scoped.

PlatformType string `json:"platformType,omitempty"`

// aws is the AWS configuration.
// All structures within the union must be optional and pointers.
// All structures within the union must be optional and have the omitzero tag.
// +optional.
AWS *MyAWSConfig `json:"aws,omitempty"`
AWS MyAWSConfig `json:"aws,omitzero"`

// azure is the Azure configuration.
// All structures within the union must be optional and pointers.
// All structures within the union must be optional and have the omitzero tag.
// +optional.
Azure *MyAzureConfig `json:"azure,omitempty"`
Azure MyAzureConfig `json:"azure,omitzero"`

// gcp is the GCP configuration.
// All structures within the union must be optional and pointers.
// All structures within the union must be optional and have the omitzero tag.
// +optional.
GCP *MyGCPConfig `json:"gcp,omitempty"`
GCP MyGCPConfig `json:"gcp,omitzero"`
}
```

The discriminator here allows the consumer to determine which of the configuration structures they should be consuming, AWS, Azure or GCP.

Important to note:
* All structs within the union **MUST** be pointers
* All structs within the union **MUST** have the omitzero tag
* All structs within the union **MUST** be optional
* All structs within the union **MUST** not allow the zero value (i.e. avoid allowing `{}` for structs with required fields)

@saschagrunert saschagrunert Jul 9, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
* All structs within the union **MUST** not allow the zero value (i.e. avoid allowing `{}` for structs with required fields)
* All structs within the union **MUST** not allow the zero value (i.e. each struct should have at least 1 required field, or a `MinProperties` validation to prevent `{}` from being valid)

Nit: the current wording could be read as "for structs that have required fields, don't allow {}", which is already the case since required fields prevent {}. I think the intent is the other direction: here's how to prevent {}. This phrasing matches what the Pointers to structs section already says.

* The discriminant should be required
* The discriminant **MUST** be a string (or string alias) type
* Discriminant values should be PascalCase and should be equivalent to the camelCase field name (json tag) of one member of the union
* Empty union members (discriminant values without a paired union member) are also permitted
* Optional union members (discriminant values with an optional union member) are also permitted (use `+unionMember,optional` to mark the union member as optional)

#### Using union types

Expand Down