✨ Add support for oneOf/anyOf/allOf/not ClusterClass variable schema constructs

[jimmidyson]

What this PR does / why we need it:
Add support for oneOf/anyOf/allOf/not ClusterClass variable schema constructs

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 #10636

/area clusterclass

[jimmidyson]

/cc @sbueringer

[sbueringer]

Will get back to this directly after the CEL PR merges

[srm09]

@jimmidyson can you rebase this PR and we can queue it up for review, many thanks!

[jimmidyson]

@srm09 @sbueringer Rebased! Sorry for slow response, but it should be ready now.

[jimmidyson]

@srm09 @sbueringer any chance of a review? 🙏

[sbueringer]

@jimmidyson Thx, looks already pretty good

[sbueringer]

@jimmidyson Do you have time to address the findings above?

[jimmidyson]

@sbueringer Sorry this slipped off my radar, taking a look now.

[sbueringer]

@jimmidyson I'll get back to this soon. Just have to finish something else first 😃

[sbueringer]

@jimmidyson Just some smaller findings and a few additional test cases.

Otherwise ready to merge from my side!

[sbueringer]

Almost there :)

[sbueringer]

@jimmidyson ^^ last one from my side

[sbueringer]

+ looks like some regen is needed. If you already did it, maybe you somehow have the wrong controller-gen binary locally, then make clean-bin should help

[jimmidyson]

• looks like some regen is needed. If you already did it, maybe you somehow have the wrong controller-gen binary locally, then make clean-bin should help

seems to be the right version 🤔

$ hack/tools/bin/controller-gen --version
Version: v0.15.0

No idea why it's different in CI to local though - running make generate results in no changes locally.

[jimmidyson]

Ah need to merge in latest main changes - one sec.

[sbueringer]

Ah that makes sense :)

[jimmidyson]

@sbueringer Regenerated and all should be good now.

[sbueringer]

Okay last ask from my side. Can you please rebase on top of main to get rid of the merge commit? (I don't remember the exact details, but I think they potentially lead to some issues)

Feel free to also squash

[jimmidyson]

@sbueringer I set merge method to squash via tide - shouldn't that do the job on merge?

[sbueringer]

@sbueringer I set merge method to squash via tide - shouldn't that do the job on merge?

It should definitely take care of the squash. I'm not sure what the problem with merge commits might be.

I only know that there is a mergecommitblocker plugin in Prow. But I just checked it doesn't seem to be enabled in the Cluster API repo (https://github.com/kubernetes/test-infra/blob/ba4cf3ddd9f5f570dd7fee899993c99c6a1753d7/config/prow/plugins.yaml#L1242)

@neolit123 Do you maybe know why so many other repositories in the kubernetes ecosystem are blocking merge commits?

[sbueringer]

Hm, this might be the original issue: kubernetes/test-infra#5376

[sbueringer]

I would prefer simply getting rid of the merge commit to be honest

[sbueringer]

Here's some guidance in the Kubernetes contributor guide: https://github.com/kubernetes/community/blob/242f468944406e5f5fe0466343cfd4d4fa1f4471/contributors/guide/github-workflow.md#4-keep-your-branch-in-sync

(seems like the GitHub UI does some magic to hide the merge commit, looking at the commit locally it indeed seems pretty messy)

[jimmidyson]

Rebased on main, squashed, and force pushed.

[sbueringer]

Thank you very much!!

Especially for the patience and also implementing int-or-string :)

/lgtm
/approve

/assign @fabriziopandini @chrischdi (just if you want to take a look)
/hold
Just want to give folks a few days to take a look as well

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 30f916cc831fe4fac85e0dc371efc449d52ba7cd

[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

[neolit123]

doesn't this field break the naming convention of camelCase?
why not xIntOrString ?

[sbueringer]

Our primary goal here is to keep this as consistent as possible with the Kubernetes CRD API type.

So basically that folks who are familiar with Kubernetes CRDs can write ~ the same schema with variables (without having to map keywords)

(That being said, I don't know the reasons why Kubernetes itself chose this format)

[sbueringer]

But seems like they are sticking to it even for newer features like CEL, xref: https://github.com/kubernetes/kubernetes/blob/67a171a1422cc5861491aadd69e51ce718196434/staging/src/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1/types_jsonschema.go#L196

[jimmidyson]

OpenAPI extensions are custom properties prefixed by x- and it seems it is more common to use kebab-case for these extensions in the wild than camelCase for these extensions as opposed to a mix like x-intOrString. I assume the kubernetes part is there to show this is a kubernetes specific openapi extension. More details at https://swagger.io/docs/specification/v3_0/openapi-extensions/.

[sbueringer]

@jimmidyson Thank you very much!!

Merging this PR to avoid further rebases on the v1beta2 conditions PR later.
(cc @fabriziopandini )

Let us know if there are further findings and we'll follow up

/hold cancel