🌱 Enable optionalorrequired linter

[sivchari]

What this PR does / why we need it:

Part of #11834

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

/area api

[sivchari]

Hi @sbueringer
I'm ready for review, thanks :)

[JoelSpeed]

Reviewing the additions of markers, everything here looks in order as far as I can tell. Just one nit about making sure we have the order of the enabled linters correct, and then good to go, thanks @sivchari

[sbueringer]

Why are we making this optional? Shouldn't this be required? (see also the delta in zz_generated.openapi.go)

(cc @JoelSpeed)

[sivchari]

Exactly, thanks.

[sbueringer]

What do you mean with exactly? :)

[sivchari]

Edited comments were not reflected.
required is correct here, ideally zz_generated_api.go should not be changed!

[JoelSpeed]

In general, the List version of a resource doesn't matter too much, it doesn't effect the generated CRD or behaviour of the cluster, but does affect the openapi as you've seen.

I'm intending to check with API machinery folks about their expectations here, whether this should be optional or required and from there will set up the linters to ensure consistency.

[sbueringer]

If getting that feedback from apimachinery takes too long I would suggest to drop all the markers for the Items field for now. Then we can get at least everything else done for the code freeze

(I don't want to block everything else on this PR just for a marker that don't have any impact anywhere (not in CRDs and the generated OpenAPI code is as far as I know only used to generate some specs for Runtime Extensions that don't even use lists)

[sbueringer]

Thinking a bit about it. @sivchari Can we directly remove the markers from the Items fields on this PR, add an exclude and follow-up? I have multiple other PRs with potential merge conflicts that I have to coordinate with this PR

(adding the Items markers later is trivial)

[sivchari]

@sbueringer
Make sense to me, thanks.
I fixed them.

[sbueringer]

Thx!

[sivchari]

Deletion line is 1.
It means we enable this linter and there are no breaking changes. 👍
It seems to be good.

[sbueringer]

@sivchari Thx! Last round of nits. Sorry these non-CRD API types are a bit strange and there's a lot of nuance

Basically optional/required has no impact on them as they are not used for CRDs, but I still don't want to declare fields as required that are actually optional (adding omitempty to a bunch of these won't change their behavior at runtime)

But I think even though these are not used in CRDs it's good to explicitly declare if the fields are optional or required.

[sivchari]

I fixed all them that you commented!

[sbueringer]

Seems like the exclude doesn't cover the Provider CRD + we need regen (the diff there is fine)

[sivchari]

I edited exclude ranges and did re-gen 👍

[sbueringer]

Thank you!

Really appreciate the work on this. I know it was a lot of work :)

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: f9db9a4d11dae5fdba7fe62d3023cef0fb9ea98a

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

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

[sbueringer]

I added a sub-task on #11834 to track the Items work. Feel free to continue with that one :)