k8s-openapi icon indicating copy to clipboard operation
k8s-openapi copied to clipboard

Extra operations on data types?

Open dpc opened this issue 3 years ago • 16 comments

I'm researching implementing a kind-of-helm-replacement that would generate input for kubectl apply -f - be just executing Rust source code. In such tool I would like to reuse types generates by k8s-openapi to define all the resources. In that respect the Rust code would become somewhat of a semi-DSL.

However, the usability of the types generated by k8s-openapi is not great, since they are very bare: just the type definition. Ideally I wish they supported builder pattern and maybe things like fn override_with(&self, other: &Self) -> Self for helm-like value shadowing where useful.

To make an example of what I have in mind:

let pod_spec = PodSpec::default()
  .set_restart_policy("Always")
  .append_container(get_default_node_js_container_spec())
  .override_with(overrides.generic_pod_overrides)
  .override_with(overrides.node_js_pod_overrides);

Would that be something that could be part of this crate? Potentially behind a feature flags for all the users that don't need it? Should it be another crate, using extension crates to add these operations?

dpc avatar Nov 26 '20 07:11 dpc

The types don't have setters or builders because all the members are pub.

Arnavion avatar Nov 26 '20 08:11 Arnavion

@Arnavion Oh, I realize they are pub. But unfortunately that doesn't make them very ergonomic. Which is not the end of the world, but is making them inconvenient to work with directly.

dpc avatar Nov 26 '20 17:11 dpc

I don't follow what setters and builders would allow you to do that setting the fields directly does not.

Arnavion avatar Nov 26 '20 20:11 Arnavion

I don't follow what setters and builders would allow you to do that setting the fields directly does not.

They would allow writing this:

let pod_spec = PodSpec::default()
  .set_restart_policy("Always")
  .append_container(get_default_node_js_container_spec())
  .override_with(overrides.generic_pod_overrides)
  .override_with(overrides.node_js_pod_overrides);

and similar for any arbitrary resource type.

dpc avatar Nov 26 '20 22:11 dpc

And the reason you can't set .restart_policy, call .containers.push(), do whatever override_with is supposed to do... is?

Arnavion avatar Nov 26 '20 23:11 Arnavion

It doesn't compose well.

let pod_spec = PodSpec::default()
  .set_restart_policy("Always")
  .append_container(get_default_node_js_container_spec())

vs ...

let mut pod_spec = PodSpec::default();
pod_spec.restart_policy = "Always".to_string();
pod_spec.container.push(get_default_node_js_container_spec());

?

It's not a huge deal, unless you want to use Rust code directly as a semi-DSL, where such code is going to be common and writen by hand a lot.

And the override_with is possibly not achievable at all, without defining it manually for every type or serializing everything to yamls and deserializing to some HashMap<String, serde_yaml::Value> or something like that. (It is supposed to work in a similar way how multiple -f <file> arguments work when passed to helm).

dpc avatar Nov 27 '20 06:11 dpc

To be honest, k8s-openapi already builds really slowly, and the second example looks cleaner to me anyway (aside from the str::to_string calls, but it's pretty un-rusty to hide those anyway IMO).

One place where the current API does break down is with manipulating the deeply nested structures that K8s is so fond of, but I don't see how this API would make that cleaner, aside from maybe flattening down /all/ the operations to the root objects...

nightkr avatar Nov 29 '20 03:11 nightkr

I'm trying to design some form of Rust quasi-DSL, that could be used as a helm replacement for the end user to specify a configuration in. As is RN, it's not very pleasant, and having to name and repeat everything over and over is really meh.

I totally understand that compilation time etc. might be a big downside, and I'm fine making this a fork/separate crate. E.g. I could make a trait PodSpecExtand alikes and define additional operations there.

If you could tell me which parts should I reuse exactly and any tips how to get it done, that could help me a lot. Thanks!

dpc avatar Nov 29 '20 03:11 dpc

k8s-openapi-codegen-common contains the code for understanding a swagger spec, and k8s-openapi-codegen uses it to emit the k8s-openapi crate. So you can try copying k8s-openapi-codegen-common and changing its templates to instead be the extension traits and methods.

Arnavion avatar Nov 29 '20 08:11 Arnavion

Thanks!

dpc avatar Nov 30 '20 05:11 dpc

Stumbling across this: Could this be an extra non-default feature that uses typed-builder/derive_builder to provide builder methods for people that need/want it?

lfrancke avatar Mar 04 '21 13:03 lfrancke

I think most of those people will stop wanting it after they see the crate takes 50% more time to compile when they enable the feature. That's why this crate doesn't use #[derive(serde::Deserialize, serde::Serialize)] and emits the impls pre-generated instead. ( https://github.com/Arnavion/k8s-openapi/issues/4 )

Arnavion avatar Mar 04 '21 18:03 Arnavion

I can only speak for myself obviously but I don't care about extra compile times. Due to the compiler cache I don't really mind if a dependency takes longer. But I agree that this will probably be different for many so I thought having it as an optional feature would be a good compromise.

Is this something you'd consider accepting PRs for?

lfrancke avatar Mar 07 '21 19:03 lfrancke

Is this something you'd consider accepting PRs for?

Yes, but please make sure to add tests for it because it wouldn't get exercised otherwise.

Arnavion avatar Mar 07 '21 19:03 Arnavion

I really need this functionality now, so I'm working on it in #120 . I still need to improve on it. Hopefully I can get it landed behind a cfg flag.

dpc avatar Jul 10 '22 09:07 dpc

I'm kicking the tires of the PR I've opened. Some thoughts and code samples: https://github.com/rustshop/rustshop/discussions/12

dpc avatar Jul 11 '22 07:07 dpc

The deep merge functionality just landed and it satisfies my needs, so closing. Thanks a lot for getting it in!

dpc avatar Aug 11 '22 00:08 dpc