Change Resource trait

[MikailBag]

• add a meta_mut method for mutable access to the metadata
• move some methods to ResourceExt trait

[kazk]

Maybe name_unchecked?

[clux]

I was going to advocate for hiding it like we did before, but after testing it against quick generateName variants, and seeing that it's a common thing (and not just an api wart that could have been an admission controller), we kind of have to deal with it.

• The response back when you create with generateName does not have metadata.name
• The first ADDED event from watch with generateName does not have metadata.name (so our runtime machinery can crash pretty quickly, will find a minimal case and open a bug)

EDIT: i might be wrong, unwrap was on something else, actually cannot reproduce the missing name now.

[clux]

I think this is a very good way forward. Thank you for this.

I think the trait can cover more of ObjectMeta's foundational types, and I think kazk's naming makes more sense.

Also think we should be using o.name_unchecked() over ResourceExt::name_unchecked(&o) in examples insofar as it's possible.

[clux]

I think we can have a lot more helpers in here if we are uncoupling the more foundational Resource trait, with a more user-friendly one.

This should be a helper trait for dealing with ObjectMeta, it needs to cover at least the basics: stuff like, labels, annotations, ownerrefs, finalizers, managedfields, where we can unpack optional vectors into empty vectors.

[MikailBag]

I will add labels, annotations, ownerrefs, and annotations (getters and setters), and also getter for UID.

I don't think we should add managedfields to the trait - AFAIU one does not need to look at .metadata.managedFields or modify it unless one is writing an alternative apiserver implementation.

[clux]

ok, fair point on managedFields.

btw, i am not sure about setters, because most of the time people change metadata fields with apply patches, or more awkward json patches, and setters kind of encourage use of api.replace which is awkward, and can run into idempotency issues.

i think maybe some patchbuilder utils for changing metadata fields might be more appropriate.

[clux]

oh, actually, this is a really nice use of get_or_insert_with!
yeah, I like these mut accessors.

[MikailBag]

I've reproduced CI failure locally: deployment does not have Completed condition (maybe Available was intended?), so it seems to be a tests issue.

[clux]

I'll fix up the CI later in the week. Thanks for the diagnosis.

The only thing left here is whether or not to call it name or name_unchecked.
I tried to find a case where we can get a null metadata.name, but even when creating an object with only generateName, it still gave a name back on the first event. If we can get away with using .name, i think it's worth keeping the simplicity, but not sure we can.
Can anyone confirm whether or not you can get a None back?

[MikailBag]

Can anyone confirm whether or not you can get a None back?
No, AFAIK you always get .metadata.name from the API.

On the other hand, a resource instance can be constructed by the application, and in this case, it can have name missing.

For example, following code will panic:

let p = Pod {spec: make_pod_spec(), ..Default::default()}
println!("{}", p.name());

That's why I have initially decided to guard against such panics. However, I understand this makes API less ergonomic, and this mistake is not too easy to make. Given that this is just a helper function, it really makes sense to allow panicking, so I'm going to rename name_unchecked to name.

Ideally, it should be separated at type level, but it requires serious changes to k8s-openapi (for example, instead of usual Pod one will have InPod (where all fields are optional) and OutPod (where e.g. .metadata.name is required, and .metadata.generateName doesn't exist), and e.g. Api::create will take InPod and return OutPod).

[clux]

Ah, interesting! Yeah, that makes it a bit more ambiguous. It's not like it's even invalid to not have a name, it's just invalid to not have both.

Alternatively, we could sidestep the issue by having ResourceExt::name fallback to {generateName}00000 if .name is missing (and to "unset" if both are missing to never panic), and then say in the docs that it only produces canonical results on valid objects from the apiserver.

[MikailBag]

While such a fallback feels natural, I'm afraid it can be too surprising. So now my proposal is to keep current behavior (i.e. panicking name() function) and leave better type safety as a future extension.

[clux]

Released in 0.53.0.