fix: struct fields to use omitzero

[EyalPazz]

/kind cleanup

Fixes #3945

Does this PR introduce a user-facing change?:

NONE

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: EyalPazz
Once this PR has been reviewed and has the lgtm label, please assign youngnick for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

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

[EyalPazz]

@JoelSpeed, is the fact that the fields are being marked as required the expected behavior here from a generator perspective?

[EyalPazz]

@rikatz

[rikatz]

this is not good :)

I think the right approach here is not to remove the omitempty, but add omitzero.

[rikatz]

btw, Joel is on PTO and will be back next week, we can probably wait for him to be back for the clarification of

Structs that are not pointers should use omitzero
Structs that are pointers should use omitempty

and if we can move for now with the omitzeroadditional without messing with requirements and applyconfig

[EyalPazz]

Yea that's what i thought, seems weird
Is it a problem with the codegen itself? or is this the expected behavior

[rikatz]

I think for these builtin metadata fields it is expected, and this is why we should not remove the "omitempty" right now.

There's some discussion around the differences between how CRDs consider optional fields vs how builtin consider optional fields, and at least for the sake of our safety, I would add omitempty,omitzero. This PR should not cause any change on generated files, we expect that the new tags are used right now just during the "wire" marshal/unmarshal process (aka converting the structs to/from json before putting it on the wire)

[EyalPazz]

Got it, will fix

[rikatz]

okie, please ping me once it is fixed so I don't miss the notification :)

[JoelSpeed]

The issue is that you are relying on fallback behaviour, add // +optional and // +required as appropriate to each field in your API and you won't need to care about whether omitempty affects the field being required or optional or not

[EyalPazz]

Thanks!
Ready for review @rikatz

[rikatz]

/lgtm
Thanks
/cc @robscott @youngnick

[JoelSpeed]

Changes here LGTM

Is the GW API community happy with the trade off of if obj.OptionalStruct != OptionalStruct{} style checks instead of nil checks for optional structs?

I know this was discussed on some other threads

The CAPI folks have adopted this and have created IsDefined methods on optional structs to help capture this need, but in general have found the changes have improved their code readability and maintainability