search

Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
379 stars 41 forks source link

Make resource metadata non-optional #71

Closed Arnavion closed 4 years ago

Arnavion commented 4 years ago

The metadata field is generally required, so having it be optional required awkward code to handle it not being set even when it ought to be set.

For situations like PATCH requests, the field is indeed not required, but setting it to an empty object {} via Default::default() also works the same way, so there is no loss of functionality.

Example: https://github.com/Arnavion/k8s-openapi/blob/resource-metadata-not-optional/src/v1_18/api/core/v1/pod.rs#L7

As the commit message says, the only case I could find where metadata does not need to be set is in PATCH requests. In all other requests, and in all server responses, the field is always set. And for PATCH requests, sending a patch with metadata: { } does not cause a problem.

cc @clux Do you think this change is correct? Will it break kube users in any way?

cc @ctron This removes the set_ function from the Metadata trait that you added. Also, does this break OpenShift in any way?

ctron commented 4 years ago

Sounds reasonable, and much easier to use. I don't see any issues with that approach.

clux commented 4 years ago

Yeah, I like this a lot. We basically unwrap in our Meta trait to hide this optional anyway - and never seen any problems with it. We have:

impl<K> Meta for K
where
    K: Metadata<Ty = ObjectMeta>,
{
    fn meta(&self) -> &ObjectMeta {
        self.metadata().expect("kind has metadata")
    }

...
}

Changing this to not be optional means that we have to make a kube release dedicated to the next k8s-openapi bump (which should probably not be a patch increment). Then we can advise people upgrade both at the same time. But I can't think of any issues with it given the workaround for PATCH. :+1: