/cc @negz @hasheddan

This is tough because I agree that the solution here is just not using omitempty at all, but that would be a pretty big divergence. However, I feel that the way it is used even in Kubernetes native types is a bit of an anti-pattern. If I understand correctly, our code would not have to change except for literally removing the omitempty tags because we still want to handle nil optional values in the same manner, but right now we are being given nil values in some cases where we do not intend them (i.e. providing a zero-values explicitly to a pointer type). Is this your understanding as well?

It doesn't sound like omitting omitempty goes against conventions:

In most cases, optional fields should also have the omitempty struct tag (the omitempty option specifies that the field should be omitted from the json encoding if the field has an empty value). However, If you want to have different logic for an optional field which is not provided vs. provided with empty values, do not use omitempty (e.g. kubernetes/kubernetes#34641).

@muvaf @negz do we feel good about removing omitempty across the board? If so, I would like to go ahead and move on this while implementing more v1beta1 resources.

@hasheddan Are you confident it won't have any negative side-effects/behaviour changes? If so I'm okay with it.

I think I'll need to do some experiments to make sure we're able to capture all three: nil value, zero value, actual value. Or @hasheddan could also play with it and report here.

@muvaf yep I'll pull some of our real-world examples and show how the change would impact 👍

kubernetes-sigs/cluster-api#707

Some more prior art here. Having swapped back in the context of this issue I think we're on the right path. I'm not sure if we want to completely remove omitempty everywhere, but we should remove it for fields for which we want to distinguish between:

1. Spec author has no opinion, propagate the default value back from the cloud.
2. Spec author wants the field to be the zero value.
3. Spec author wants the field to be a non-zero value.

So we'd represent these cases (using an *int field as example) as:

1. Spec author omits the field, which gets serialized as a nil pointer.
2. Spec author writes 0, which gets serialised as a pointer to int 0.
3. Spec author writes 8, which gets serialised as a pointer to int 8.

https://crossplane.slack.com/archives/CKXQHM7U3/p1581113468101800
https://play.golang.org/p/NnWsj9BaTeJ
https://play.golang.org/p/oxQgQxtWK62

Cross-linking some discussion from Slack. I actually now think our late init code is working as expected a little broken when it comes to late-initializing an unspecified spec field from a cloud provider's zero value. 🤔 I believe:

func LateInitializeBool(b *bool, from bool) *bool {
	if b != nil || !from {
		return b
	}
	return &from
}

In the above case b is the user provided value and from is the value we get back from the cloud provider. If we walk through some cases, assuming the following struct:

type Example struct {
    Field *bool `json:"field,omitempty"`
}

1. The user writes field: false. This is unmarshalled as Field: *false. Therefore b != nil, so we return b, thus keeping the user provided value without ever considering what from (the cloud provider value) was set to.
2. The user writes field: true. Same as above - it's not nil so we keep it.
3. The user omits field from their spec. This is unmarshalled as Field: nil, so b == nil. We deem the user to have no opinion about the value of this field and proceed to late initialisation from the cloud provider value.
   1. If the provider value was true, we late initialize Field: *true.
   2. If the provider value was false, we return b, which is nil.

So actually in the case of *bool fields it seems like the alternative logic @muvaf proposed would actually be better:

func LateInitializeBool(b *bool, from bool) *bool {
	if b != nil {
		return b
	}
	return &from
}

With this logic if the cloud provider specified false and the user had no opinion (omitted the field) we'd late initialize the field to *false, rather than nil.

I think the two external issues relating to omitempty that were linked in this issue were red herrings, because they deal with fields that are slices (not pointer to slices).

Another option would be to add a flag to the CRDs that disallow late init stuff?
We may need to validate that all late init fields has a value if this new flag is set.

This will allow us to have complete control using the Crossplane CRDs.

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.