Allow opting out of schema derivation per kind

[nightkr]

The feature-flag approach was problematic since it applies to the whole dependency tree: as long as any crate depends on kube_derive without disabling default features then the derived schema becomes mandatory.

This PR adds a new #[kube(derive_schema = false)] attribute that allows you to specify this per kind.

[clux]

I think this is a better solution. Thanks a lot!
It will force schemars into the dependency tree of people not overriding the schema, but because it's going to be required to have a schema anyway, that's probably OK.

One minor suggestion on docs, but otherwise LGTM.

[nightkr]

It will force schemars into the dependency tree of people not overriding the schema, but because it's going to be required to have a schema anyway, that's probably OK.

I don't see how this PR changes anything here? kube-derive doesn't depend on schemars either way.

[clux]

I don't see how this PR changes anything here? kube-derive doesn't depend on schemars either way.

oh, right that's in the generated code! that's even better. definitely no need for the feature then 👍

[nightkr]

@clux I redesigned the attribute a bit so that we now also support custom impl JsonSchema for MyKind.

[clux]

some nits and questions on the new mode

[clux]

I originally misunderstood the intention of these names, so proposing some new ones:

• Disabled -> Elide (to distance ourselves from the separate requirement of having these)
• Custom -> Specialize (to indicate more clearn that a user has to specialize its impl)
• Derived -> Derive or Auto (to more indicate no user action needed)

That also potentially reads nicer with a short-hand name:

#[kube(schema = "Elide")]

[clux]

although, that said, the imperative form of the verbs, while helpful for Derive ("you do it program"), is probably not so helpful for Specialize..

[clux]

maybe we just have two bools instead:

• #[kube(schema_specialize)
• #[kube(schema_elide)

that matches the internal model (and the bools have no meaningful overlap)

[nightkr]

I'm not married to any of the current names, but I'm not sold on any of your proposed changes either.

Disabled -> Elide (to distance ourselves from the separate requirement of having these)

Elide sounds like lifetime elision to me (in a Rust context).. the lifetimes still exist but we can imply them from the context (which, if anything, is closer to deriving than disabling IMO)

Custom -> Specialize (to indicate more clearn that a user has to specialize its impl)

This sounds like trait/type specialization, and I think the connection might confuse more than it helps..

Derived -> Derive or Auto (to more indicate no user action needed)

Auto implies to me that we automatically select the mode (which is currently implied when there is no schema_mode attribute)

two bools

Bleh.

---

That said, I think this raises the question about whether SchemaMode::Disabled is operating at the wrong abstraction level.. Maybe the better question would be whether we should have a flag to disable Self::crd() wholesale (for a given kind)?

[nightkr]

On the other hand, it might be worth keeping SchemaMode::Disabled around (in some form) as a migration path for people who have used the current override...

[nightkr]

This is a rustc issue, not a darling (or syn) one. When I was testing it I was dumping the raw token stream that we get fed.

[clux]

Yeah, you can dump the full input in kube-derive/lib.rs and the doc comments before are in fact available to the input, but not the root #[derive(..)] :-(

[kazk]

I see derive if I move them into a separate derive, but that's useless:

#[derive(CustomResource)]
//
#[derive(Serialize, Deserialize, Debug, PartialEq, Clone, JsonSchema)]
struct ExampleSpec {
}

[kazk]

Yeah, we can't do this. rust-lang/rust#40574 (comment)

To be honest, I would have preferred to not special-case the derive at all, and just let people #[kube(derive(JsonSchema))] it if they wanted to.

How does this work? Maybe I'm misremembering, but I thought we couldn't modify the input struct. Is it possible to add derive(JsonSchema) to it? Doesn't the spec struct need derive(JsonSchema) to have schema generated?

[clux]

How does this work?

We cannot modify the input, but we were just thinking about reading that derive property.

The key struct here isn't the spec struct, but the generated root struct, which needs to implement JsonSchema (with our injected derive on it, or manually by the user) for the schema to get included in the ::crd() method.

The only remaining possibility here afaikt is that we could use #[kube(derive = "JsonSchema")] as an indicator of whether to automatically derive JsonSchema on the root struct (rather than select it as part of a tri-state enum, or, do as we do know and inject it by default if the feature is on). But that would be a pretty big breaking change (and it would break in a way-too-subtle way to introduce at this stage).

out of date

Changelog was updated

[clux]

I'd like to have this merged in because it's a great improvement, and all actionable issues have been addressed 👍 .

The only thing we miss here is resolving the merge conflicts and possibly thinking of better names, but none of my suggestions seem much better either, so lacking any new suggestions, let's merge this?

[nightkr]

The merge conflicts should be fixed now @clux

[clux]

LGTM. Although tests need the same replace. Happy to approve at green though!

[nightkr]

There's always something, huh. 🤦