Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic wait-for-condition Condition #679

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nightkr
Copy link
Member

@nightkr nightkr commented Oct 26, 2021

Super unhappy with the API, but it's a start...

Fixes #669

Super unhappy with the API, but it's a start...

Fixes kube-rs#669

Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
@nightkr nightkr requested a review from clux October 26, 2021 13:52
Signed-off-by: Teo Klestrup Röijezon <teo@nullable.se>
Comment on lines +250 to +253
let serialized_obj = serde_json::to_value(obj).ok();
status_cond.matches_object(serialized_obj.as_ref().and_then(|obj| {
obj.get("status")?
.get("conditions")?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we'd get around this with a HasConditions trait, but that would have to be upstream in k8s-openapi to be of much help

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those types of traits we'd need to define in the protobuf repo. I'll open some issues there to track.

Comment on lines +254 to +260
.as_array()?
.iter()
.find(|cond| {
cond.get("type").and_then(serde_json::Value::as_str) == Some(condition_type)
})?
.get("status")?
.as_str()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤮

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, I thought we were going ot start with DynamicObject first. This is pretty gross :D

Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor comments. I think this definitely works, and agree with the stability marking of this. would be nice to have a better typed alternative.

Comment on lines +244 to +245
#[must_use]
pub fn unstable_has_status_condition<'a, K: Serialize + Resource, StatusCond: Condition<str> + 'a>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should look into something like https://docs.rs/stability/0.1.0/stability/attr.unstable.html rather than prefixing fn names

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That kind of makes sense. I'd almost rather have this be something that the end user has to opt into specifically with a --cfg, rather than a feature you can inherit by accident from a dependency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is the tokio solution for this as well #508 (comment). Anyway, I've made notes about this in the big stability roadmap issue, because it's something we should solve for that.

Comment on lines +250 to +253
let serialized_obj = serde_json::to_value(obj).ok();
status_cond.matches_object(serialized_obj.as_ref().and_then(|obj| {
obj.get("status")?
.get("conditions")?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those types of traits we'd need to define in the protobuf repo. I'll open some issues there to track.

///
/// ```rust
/// # use k8s_openapi::api::core::v1::{Pod, PodCondition, PodStatus};
/// # use kube_runtime::wait::{conditions::unstable_has_status_condition, Condition};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for reference; in docs you can use kube::runtime in doc paths for consistency. i've done this everywhere in all crates to refer to kube

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a doctest, which is built before the kube crate is. Either way, the line is hidden from readers (by prefixing with #).

Comment on lines +239 to +240
/// let cond_status_ready: fn(Option<&str>) -> bool = |status| status == Some("True");
/// let cond_pod_ready = unstable_has_status_condition("Ready", cond_status_ready);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I almost feel like two &str inputs here are better than this closure. It's a struggle to understand why cond_status_ready needs so much wrapping from a consumer standpoint when all you are really doing here is passing in the value of an expected string.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't cover something like |status| status == Some("Unknown") || status == Some("True"). It's also worth keeping in mind that status can ultimately be any string, the True/False/Unknown troolean is just a convention.

Then again, I'm not sure those advanced use-cases are something people actually want?

kube-runtime/src/wait.rs Outdated Show resolved Hide resolved
Comment on lines +254 to +260
.as_array()?
.iter()
.find(|cond| {
cond.get("type").and_then(serde_json::Value::as_str) == Some(condition_type)
})?
.get("status")?
.as_str()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, I thought we were going ot start with DynamicObject first. This is pretty gross :D

Co-authored-by: Eirik A <sszynrae@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add general conditions::is_met helper to runtime::wait
2 participants