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

Don't enable the v1_13 feature in the k8s-openapi direct dep #44

Closed
Arnavion opened this issue Jun 24, 2019 · 2 comments
Closed

Don't enable the v1_13 feature in the k8s-openapi direct dep #44

Arnavion opened this issue Jun 24, 2019 · 2 comments

Comments

@Arnavion
Copy link

k8s-openapi only allows one feature to be enabled. If a crate's dep graph contains multiple crates like kube that decide to enable features in their direct deps, Cargo will union the features and enable multiple of them simultaneously, which will not compile.

(It's mentioned in a little detail at the top of https://docs.rs/k8s-openapi/0.4.0/k8s_openapi/ , but I'm planning to expand that and the build script panic messages in Arnavion/k8s-openapi#44 )

Basically I would like binary crates to enable a specific feature depending on what minimum version of API server they want to run against. So for something like your controller-rs crate it's okay to enable the v1_13 feature there.

But for library crates like kube, my ask is:

  • Don't enable any features in your k8s-openapi direct dep.

  • Do enable it in your k8s-openapi dev-dep for your crate's tests and examples.

  • Use the k8s_* version detection macros to limit what versions you want your crate to compile with. For example you can use compile_error like in the docs example.

It does mean that controller-rs would need to add a dependency on k8s-openapi just to enable the feature, even though all its interaction with Kubernetes is through kube.

Please tell me if you have any objections. (I have not received any feedback positive or negative to this design decision.)


I know this goes against Cargo's general idea that features should be additive. The original implementation of this in 0.3.0 and earlier did have additive features, by having the crate's API exposed under a module of the same name. Eg the v1_13 feature enabled the k8s_openapi::v1_13 module.

But it meant you had to write imports like this:

/~https://github.com/Arnavion/k8s-openapi/blob/v0.3.0/k8s-openapi-tests/src/pod.rs#L3-L20

which was a PITA, and still wouldn't have worked if more than one feature was enabled. See Arnavion/k8s-openapi#18 for more details.

@clux
Copy link
Member

clux commented Jun 25, 2019

Hey, thanks for all this info. I'm happy with this reasoning for not enabling k8s-openapi features in kube and letting that be decided by the app themselves. It feels like the better pattern. Will make the change in the next version 👍

AFAIKT, by removing the default feature choice from kube, it should only force it on controller-rs IF it actually needed k8s-openapi features in the first place. That style of CRD only app was my main reason for keeping k8s-openapi behind a feature.

Aside: There's plenty of good reasons to have mutually exclusive features, it's a shame cargo makes it slightly awkward, there's a real need for them if you see the various issues. Have been using static_assertions to help with this recently.

@clux
Copy link
Member

clux commented Jun 25, 2019

Also, thanks a lot for building k8s-openapi it's been essential for building stuff on top of kubernetes. I will likely send some feature requests or questions your way at some point 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants