We need to remove `omitempty` from all values which aren't struct pointers

[EmilienM]

Originally posted by @mdbooth in #1729 (comment)

After offline discussion with @JoelSpeed about this, I think I understand a new API principal: omitempty is only for struct pointers. IOW we're massively over-using it.

I'd like this to be another v1beta1 cleanup: remove omitempty from all values which aren't struct pointers. Unfortunately this may mean turning some structs (back) into pointers if we want to ensure they're omitted, which I hate because Go makes struct pointers in large data structures a syntactic minefield¹.

Footnotes

1. Because it doesn't require a -> operator like more civilised pointer languages, it's impossible to visually distinguish a pointer dereference from regular access of a struct field, which means it's easy to miss that you need a null check. Even worse, turning a struct into a struct pointer won't break compilation of existing references, meaning they will all have to be checked manually when making this change. ↩

[EmilienM]

@kubernetes-sigs/cluster-api-provider-openstack-maintainers please add the v1beta1 label.

[JoelSpeed]

Just to clarify, the statement applies only to structs. Non-struct, non-pointer fields may have omitempty, but you need to be wary of the unstructured vs structured serialisation of fields.

For structs, the json marshaller cannot determine if a struct is at its zero value, therefore, it struct fields must be pointers to allow it to omit them.

For a string, you can use a pointer or not, as the json marshaller will omit the "" in the case it's not a pointer, or the nil value if it is.

The gotcha here is that, if you have a non-pointer field, some users may set the value to "" using unstructured data (Eg yaml and kubectl apply), and when they do so, nothing applies that omitempty. If something later marshals the struct using a structured approach, it will clear the field from the representation, which could be confusing.

To avoid that, all fields that are optional are recommended, by the Kube API conventions, to have a nil value (either a pointer or a type that already has that such as a map/slice)

[EmilienM]

Will be addressed by #1897

[EmilienM]

/assign mdbooth

[lentzi90]

I promised to get back to this with an opinion once I had a chance to try to properly understand it. Now I have done that, and I agree that we should do this. 👍

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[EmilienM]

@mdbooth and team did this already.