Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for untagged enums in CRDs #1028

Merged
merged 13 commits into from
Oct 7, 2022

Conversation

sbernauer
Copy link
Contributor

Motivation

We want to use untagged enums in our CRDs. This allows us to better validate the users input, without the user realizing that under the hood enums are used.

Solution

Extending the Visitor for StructuralSchemaRewriter to rewrite anyOf clauses so that they are accepted by the k8s api-server.

Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
@sbernauer sbernauer changed the title Add support for untagged enums Add support for untagged enums in CRDs Sep 26, 2022
@nightkr nightkr self-requested a review September 27, 2022 12:06
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to work fine in my testing. My one concern is that this is basically the same logic as the existing one_of flattening (and seems to be ~93% copy/pasted). Could we factor it out to a separate function that we reuse?

After that I'd be happy to merge.

@nightkr nightkr added utils extra utility modules changelog-add changelog added category for prs labels Sep 27, 2022
@nightkr nightkr added this to the 0.76.0 milestone Sep 27, 2022
@nightkr nightkr self-assigned this Sep 27, 2022
@sbernauer
Copy link
Contributor Author

Refactored it out to a separate function, please have a look

Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
kube-core/src/schema.rs Outdated Show resolved Hide resolved
kube-core/src/schema.rs Outdated Show resolved Hide resolved
kube-core/src/schema.rs Outdated Show resolved Hide resolved
kube-core/src/schema.rs Outdated Show resolved Hide resolved
kube-core/src/schema.rs Outdated Show resolved Hide resolved
sbernauer and others added 5 commits September 28, 2022 08:09
Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Co-authored-by: Teo Klestrup Röijezon <teo@nullable.se>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
subschemas: &mut Vec<Schema>,
common_obj: &mut Option<Box<ObjectValidation>>,
instance_type: &mut Option<SingleOrVec<InstanceType>>,
allow_if_equal: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can get rid of allow_if_equal now and just assume that across the board?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That changes the current implementation for oneOf but sounds reasonable. Adopted!

Signed-off-by: Sebastian Bernauer <sebastian.bernauer@stackable.de>
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM!

@nightkr nightkr merged commit 3033e0f into kube-rs:main Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs utils extra utility modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants