Skip to content

Comments

fix: Generate valid CRDs for fields with Optional enums and doc-comments#1907

Open
NickLarsenNZ wants to merge 6 commits intokube-rs:mainfrom
stackabletech:fix/optional-enum
Open

fix: Generate valid CRDs for fields with Optional enums and doc-comments#1907
NickLarsenNZ wants to merge 6 commits intokube-rs:mainfrom
stackabletech:fix/optional-enum

Conversation

@NickLarsenNZ
Copy link
Contributor

Fixes #1906

Motivation

This is a continuation of #1821

Solution

Replace the OptionalEnum schema transformer with the one from #1839, and adds coverage to the existing test.

In addition, I moved the schema module into a subdirectory with hopes that the other transformers and their tests will be moved into individual modules below that. I am happy to do that in this PR, but decided to wait until you had a chance to look at it.

@doxxx93
Copy link
Member

doxxx93 commented Jan 19, 2026

Thanks for working on this!

I think the module restructuring could be split into a separate PR. It would be easier to review if this PR focuses only on fixing #1906, and the refactoring comes as a follow-up.

What do you think?

@codecov
Copy link

codecov bot commented Jan 19, 2026

Codecov Report

❌ Patch coverage is 95.34884% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.2%. Comparing base (a75b211) to head (6c533bb).

Files with missing lines Patch % Lines
kube-core/src/schema/transform_optional_enum.rs 95.2% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1907     +/-   ##
=======================================
+ Coverage   76.1%   76.2%   +0.1%     
=======================================
  Files         86      87      +1     
  Lines       8399    8429     +30     
=======================================
+ Hits        6387    6418     +31     
+ Misses      2012    2011      -1     
Files with missing lines Coverage Δ
kube-core/src/schema/mod.rs 95.9% <100.0%> (ø)
kube-derive/tests/crd_schema_test.rs 100.0% <ø> (ø)
kube-core/src/schema/transform_optional_enum.rs 95.2% <95.2%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@doxxx93
Copy link
Member

doxxx93 commented Jan 19, 2026

I've submitted #1908 with a minimal fix.

Your PR introduces 248 new lines with module restructuring and LazyLock, but the root cause is simpler - the o.len() == 1 check skips schemas when doc comments add a description field.

Just removing that check (1 line) is enough since the remaining conditions already ensure we only match Option<Enum> patterns:

  • anyOf with exactly 2 elements
  • First has enum without nullable
  • Second is { "enum": [null], "nullable": true }

The module restructuring could be a good follow-up, but I'd prefer to keep the bug fix minimal.

… CRD to be produced

Signed-off-by: Nick Larsen <nick.larsen@stackable.tech>
Schema tests included

Signed-off-by: Nick Larsen <nick.larsen@stackable.tech>
… now preserved

Signed-off-by: Nick Larsen <nick.larsen@stackable.tech>
Note: Cause by inline_attribute_width
Signed-off-by: Nick Larsen <nick.larsen@stackable.tech>
Signed-off-by: Nick Larsen <nick.larsen@stackable.tech>
Note: This should be done on other transformers in a future PR to keep this PR related to what it is fixing.
Signed-off-by: Nick Larsen <nick.larsen@stackable.tech>
@clux clux added the changelog-fix changelog fix category for prs label Jan 19, 2026
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

quick first pass. i think this is a much better size of PR. thank you for your patience.

appreciate you're not moving to too much code around

hopes that the other transformers and their tests will be moved into individual modules [..] but decided to wait

yeah, let's wait until after this PR for that please.

Comment on lines +80 to +82
if any_of.len() != 2 {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

if possible. when checking for length, you should ideally destructure like in the original impl

        let [first, second] = arr.as_slice() else {
            return;
        };

to avoid a naked [0] and [1] later (which is only safe if you look 10 lines up)

but i don't know if this makes sense with the impl below (follow-up comment)

return;
}

let entry_is_null: [bool; 2] = any_of
Copy link
Member

Choose a reason for hiding this comment

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

this map needs a comment for what it's doing

/// }
/// ```
///
/// This transform implementation prevents this specific case from happening.
Copy link
Member

Choose a reason for hiding this comment

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

this comment is a moved variant of the old implementation, but it's only talking about the old "specific case" even though the implementation has been completely changed. is this right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog-fix changelog fix category for prs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optional enums with comments emit invalid CRDs

3 participants