add eviction subresource - for #127 #393
Conversation
|
Wow that was fast! :-) Thanks a lot for that |
kube/src/api/subresource.rs
Outdated
| } | ||
|
|
||
| /// Marker trait for objects that can be evicted | ||
| pub trait EvictionObject {} |
There was a problem hiding this comment.
Nit: Evictable, maybe? EvictionObject sounds like it's the reified object created when evicting something.
There was a problem hiding this comment.
Hm, I thought we had a convention, on noun-ing. But I am wrong.
Maybe we should just go full on on the conjugated form and rename all the traits?
i.e.
pub trait Loggable
pub trait Evictable
pub trait Executable
pub trait Atttachable
There was a problem hiding this comment.
I was thinking EvictableObject like AttachableObject.
Logging and Executing sounds fine to me, but not against making them consistent.
Naming is hard.
There was a problem hiding this comment.
Sounds good to me. I have been thinking about proposing that for all resource actions at some point (for example: ComponentStatus is read-only: https://v1-18.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#componentstatus-v1-core).
There was a problem hiding this comment.
I'll make a follow up PR for standardising. Sounds like we are OK with the conjugation as a convention.
kube/src/api/subresource.rs
Outdated
| pub delete_options: Option<DeleteParams>, | ||
| /// Metadata describing the pod being evicted | ||
| /// we somehow need this despite providing the name of the pod.. | ||
| pub metadata: ObjectMeta, |
There was a problem hiding this comment.
That feels like a pretty big footgun.. Maybe Api::evict could take the name from the Eviction (or the reverse)?
There was a problem hiding this comment.
yeah, we could just pave over that interface, and take a (DeleteParams, PostParams, name) and only have the Eviction struct be internal. I don't see what good the metadata would do if it's tied to a named pod anyway.
There was a problem hiding this comment.
Made an EvictParams object containing the two params objects, and hid Eviction in the body of Resource::evict
|
false negative on kind ci again, need to fix that at some point. |
might rebase against the big hyper pr, but it was fairly trivial to get this to work at least.