Make Api::create and Api::replace take type-safe KubeObjects
#143
Conversation
`Vec<u8>`s are still supported, but deprecated. Sadly, `#[deprecated]` doesn't work for trait impls, so a doccomment was used instead. Fixes kube-rs#142.
| "image": "alpine:latest" | ||
| spec: JobSpec { | ||
| template: PodTemplateSpec { | ||
| metadata: Some(OpenApiObjectMeta { |
There was a problem hiding this comment.
This seems like a pretty big API wart to expose.. :/
There was a problem hiding this comment.
Yeah, the json! macro is pretty nice to avoid having to explicitly type out all the defaults.
We could maybe do some magic with TryFrom json::Value -> relevant K, but that would only give us runtime type-safety, which only saves us from having to actually post it to the k8s api.
There was a problem hiding this comment.
I was mostly talking about the ObjectMeta vs OpenApiObjectMeta distinction.
The Default proliferation sounds like it could be worked around with a macro that implicitly adds a ..Default::default() member to every object declaration? Alternatively, builders..
There was a problem hiding this comment.
Ah. Yeah, fair enough, that is a bit ugly.
Btw keep in mind that we need to let people create incomplete objects anyway for server-side-apply (we set the fields we care about, api server handles the rest). I think it would be a lot easier if they could just use the json! macro (or something similar for easy yaml if it exists) for it.
There was a problem hiding this comment.
Yeah.. I considered partial updates (apply, patch) out of scope for this PR.
I think it would be a lot easier if they could just use the json! macro (or something similar for easy yaml if it exists) for it.
Not a huge fan of telling users to send untyped blobs.. I guess we could impl<K> SerializeKubeObject<K> for serde_json::Value, so they at least get types that last that far...
There was a problem hiding this comment.
I should probably have clarified.. OwnedSubset itself doesn't care about the data itself, it's just a glorified From<K>. The macro would generate the (equivalent of) into, enforcing that all fields are OwnedSubsets of their corresponding fields in the base. That should work as long as we also blanket impl<K> OwnedSubset<K> for K (for the leaves).
There was a problem hiding this comment.
Then again, k8s-openapi seems to go out of their way to avoid serializing empty fields, so all of this extra machinery probably wouldn't actually help. From what I can tell, just Api::apply(serde_json::to_vec(&deployment)) should actually do the right thing? Then again, I'm probably missing something and would have to experiment more to get anywhere.
There was a problem hiding this comment.
I should also clarify my own needs before kicking off a huge over-engineering project: at the moment I mostly care about controllers that fully own their resources, create, replace, and watch are what I really care about.
There was a problem hiding this comment.
Then again, k8s-openapi seems to go out of their way to avoid serializing empty fields, so all of this extra machinery probably wouldn't actually help. From what I can tell, just
Api::apply(serde_json::to_vec(&deployment))should actually do the right thing? Then again, I'm probably missing something and would have to experiment more to get anywhere.
Oh wow. k8s-openapi really have gotten a lot smarter.
Yeah, if with had Api::apply as an alias for patch with server-side apply, then that should do the right thing. Particularly since we get to take advantage over that serde impl in k8s-openapi.
There was a problem hiding this comment.
I should also clarify my own needs before kicking off a huge over-engineering project: at the moment I mostly care about controllers that fully own their resources,
create,replace, andwatchare what I really care about.
If you have any defaulting behaviour from the openapi schema in the CRD, or any mutating admission controllers, then using server-side Api::apply instead of both replace and create would probably be the best thing (as that preserves the behaviour of those). But in your simpler case, you can just use Api::create and Api::replace in the simple case.
|
Btw, I wouldn't worry about being so well-behaved with breaking changes atm. It's pretty-well advertised that things are in flux at the top of the readme. |
|
I think there are some good ideas in here that's worth salvaging, but as you said, given #173 we should rethink them a bit. I think the owned subset makes a lot of sense for |
Vec<u8>s are still supported, but deprecated. Sadly,#[deprecated]doesn't work for trait impls, so a doccomment was used instead.Fixes #142.
This is mostly a spike to get feedback on whether it'd be a good idea. The rest of the examples should probably be ported before merging...