From a06ed2c7d4f33766913492ba4ab0b35e779fda32 Mon Sep 17 00:00:00 2001 From: clux Date: Wed, 22 May 2024 21:54:09 +0100 Subject: [PATCH 1/6] Try defining runtime Condition in terms of option functions Takes advantage of `?` on `Option` to allow simpler `Condition` impls. Not a huge change internally, but it is a breaking change. Signed-off-by: clux --- kube-runtime/src/wait.rs | 93 ++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 51 deletions(-) diff --git a/kube-runtime/src/wait.rs b/kube-runtime/src/wait.rs index e7451cb60..fcdab94da 100644 --- a/kube-runtime/src/wait.rs +++ b/kube-runtime/src/wait.rs @@ -9,6 +9,7 @@ use thiserror::Error; use crate::watcher::{self, watch_object}; +/// Wait errors from [`await_condition`] #[derive(Debug, Error)] pub enum Error { #[error("failed to probe for whether the condition is fulfilled yet: {0}")] @@ -79,21 +80,18 @@ where /// use k8s_openapi::api::core::v1::Pod; /// fn my_custom_condition(my_cond: &str) -> impl Condition + '_ { /// move |obj: Option<&Pod>| { -/// if let Some(pod) = &obj { -/// if let Some(status) = &pod.status { -/// if let Some(conds) = &status.conditions { -/// if let Some(pcond) = conds.iter().find(|c| c.type_ == my_cond) { -/// return pcond.status == "True"; -/// } -/// } -/// } -/// } -/// false +/// let cond = obj.status.as_ref()?.conditions.as_ref()?.iter().find(|c| c.type_ == my_cond); +/// Some(cond.status == "True") /// } /// } /// ``` pub trait Condition { - fn matches_object(&self, obj: Option<&K>) -> bool; + /// Condition function giving a clear answer + fn matches_object(&self, obj: Option<&K>) -> bool { + self.matches_object_option(obj).unwrap_or_default() + } + /// Condition function giving an option wrapped answer + fn matches_object_option(&self, _obj: Option<&K>) -> Option; /// Returns a `Condition` that holds if `self` does not /// @@ -153,8 +151,13 @@ pub trait Condition { } } -impl) -> bool> Condition for F { - fn matches_object(&self, obj: Option<&K>) -> bool { +//impl) -> bool> Condition for F { +// fn matches_object(&self, obj: Option<&K>) -> bool { +// (self)(obj) +// } +//} +impl) -> Option> Condition for F { + fn matches_object_option(&self, obj: Option<&K>) -> Option { (self)(obj) } } @@ -176,12 +179,12 @@ pub mod conditions { #[must_use] pub fn is_deleted(uid: &str) -> impl Condition + '_ { move |obj: Option<&K>| { - obj.map_or( + Some(obj.map_or( // Object is not found, success! true, // Object is found, but a changed uid would mean that it was deleted and recreated |obj| obj.meta().uid.as_deref() != Some(uid), - ) + )) } } @@ -192,48 +195,36 @@ pub mod conditions { #[must_use] pub fn is_crd_established() -> impl Condition { |obj: Option<&CustomResourceDefinition>| { - if let Some(o) = obj { - if let Some(s) = &o.status { - if let Some(conds) = &s.conditions { - if let Some(pcond) = conds.iter().find(|c| c.type_ == "Established") { - return pcond.status == "True"; - } - } - } - } - false + let cond = obj + .as_ref()? + .status + .as_ref()? + .conditions + .as_ref()? + .iter() + .find(|c| c.type_ == "Established")?; + Some(cond.status == "True") } } /// An await condition for `Pod` that returns `true` once it is running #[must_use] pub fn is_pod_running() -> impl Condition { - |obj: Option<&Pod>| { - if let Some(pod) = &obj { - if let Some(status) = &pod.status { - if let Some(phase) = &status.phase { - return phase == "Running"; - } - } - } - false - } + |obj: Option<&Pod>| Some(obj?.status.as_ref()?.phase.as_ref()? == "Running") } /// An await condition for `Job` that returns `true` once it is completed #[must_use] pub fn is_job_completed() -> impl Condition { |obj: Option<&Job>| { - if let Some(job) = &obj { - if let Some(s) = &job.status { - if let Some(conds) = &s.conditions { - if let Some(pcond) = conds.iter().find(|c| c.type_ == "Complete") { - return pcond.status == "True"; - } - } - } - } - false + let cond = obj? + .status + .as_ref()? + .conditions + .as_ref()? + .iter() + .find(|c| c.type_ == "Complete")?; + Some(cond.status == "True") } } @@ -241,8 +232,8 @@ pub mod conditions { #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct Not(pub(super) A); impl, K> Condition for Not { - fn matches_object(&self, obj: Option<&K>) -> bool { - !self.0.matches_object(obj) + fn matches_object_option(&self, obj: Option<&K>) -> Option { + Some(!self.0.matches_object(obj)) } } @@ -254,8 +245,8 @@ pub mod conditions { A: Condition, B: Condition, { - fn matches_object(&self, obj: Option<&K>) -> bool { - self.0.matches_object(obj) && self.1.matches_object(obj) + fn matches_object_option(&self, obj: Option<&K>) -> Option { + Some(self.0.matches_object(obj) && self.1.matches_object(obj)) } } @@ -267,8 +258,8 @@ pub mod conditions { A: Condition, B: Condition, { - fn matches_object(&self, obj: Option<&K>) -> bool { - self.0.matches_object(obj) || self.1.matches_object(obj) + fn matches_object_option(&self, obj: Option<&K>) -> Option { + Some(self.0.matches_object(obj) || self.1.matches_object(obj)) } } } From d99ddca71ec5be7d18bb3e07ec8552d6f48c0144 Mon Sep 17 00:00:00 2001 From: clux Date: Wed, 12 Mar 2025 20:45:32 +0000 Subject: [PATCH 2/6] port over new conditions Signed-off-by: clux --- kube-runtime/src/wait.rs | 80 ++++++++++++---------------------------- 1 file changed, 23 insertions(+), 57 deletions(-) diff --git a/kube-runtime/src/wait.rs b/kube-runtime/src/wait.rs index 7a84cc3d2..1022d3109 100644 --- a/kube-runtime/src/wait.rs +++ b/kube-runtime/src/wait.rs @@ -200,15 +200,10 @@ pub mod conditions { #[must_use] pub fn is_crd_established() -> impl Condition { |obj: Option<&CustomResourceDefinition>| { - let cond = obj - .as_ref()? - .status - .as_ref()? - .conditions - .as_ref()? - .iter() - .find(|c| c.type_ == "Established")?; - Some(cond.status == "True") + let status = obj.as_ref()?.status.as_ref()?; + let conds = status.conditions.as_ref()?; + let established = conds.iter().find(|c| c.type_ == "Established")?; + Some(established.status == "True") } } @@ -222,14 +217,10 @@ pub mod conditions { #[must_use] pub fn is_job_completed() -> impl Condition { |obj: Option<&Job>| { - let cond = obj? - .status - .as_ref()? - .conditions - .as_ref()? - .iter() - .find(|c| c.type_ == "Complete")?; - Some(cond.status == "True") + let status = obj.as_ref()?.status.as_ref()?; + let conds = status.conditions.as_ref()?; + let complete = conds.iter().find(|c| c.type_ == "Complete")?; + Some(complete.status == "True") } } @@ -240,18 +231,11 @@ pub mod conditions { #[must_use] pub fn is_deployment_completed() -> impl Condition { |obj: Option<&Deployment>| { - if let Some(depl) = &obj { - if let Some(s) = &depl.status { - if let Some(conds) = &s.conditions { - if let Some(dcond) = conds.iter().find(|c| { - c.type_ == "Progressing" && c.reason == Some("NewReplicaSetAvailable".to_string()) - }) { - return dcond.status == "True"; - } - } - } - } - false + let conds = obj?.status.as_ref()?.conditions.as_ref()?; + let progressing = conds.iter().find(|c| { + c.type_ == "Progressing" && c.reason == Some("NewReplicaSetAvailable".to_string()) + })?; + Some(progressing.status == "True") } } @@ -259,23 +243,9 @@ pub mod conditions { #[must_use] pub fn is_service_loadbalancer_provisioned() -> impl Condition { |obj: Option<&Service>| { - if let Some(svc) = &obj { - // ignore services that are not type LoadBalancer (return true immediately) - if let Some(spec) = &svc.spec { - if spec.type_ != Some("LoadBalancer".to_string()) { - return true; - } - // carry on if this is a LoadBalancer service - if let Some(s) = &svc.status { - if let Some(lbs) = &s.load_balancer { - if let Some(ings) = &lbs.ingress { - return ings.iter().all(|ip| ip.ip.is_some() || ip.hostname.is_some()); - } - } - } - } - } - false + let status = obj?.status.as_ref()?; + let ingress = status.load_balancer.as_ref()?.ingress.as_ref()?; + Some(ingress.iter().all(|ip| ip.ip.is_some() || ip.hostname.is_some())) } } @@ -283,16 +253,9 @@ pub mod conditions { #[must_use] pub fn is_ingress_provisioned() -> impl Condition { |obj: Option<&Ingress>| { - if let Some(ing) = &obj { - if let Some(s) = &ing.status { - if let Some(lbs) = &s.load_balancer { - if let Some(ings) = &lbs.ingress { - return ings.iter().all(|ip| ip.ip.is_some() || ip.hostname.is_some()); - } - } - } - } - false + let status = obj?.status.as_ref()?; + let ingress = status.load_balancer.as_ref()?.ingress.as_ref()?; + Some(ingress.iter().all(|ip| ip.ip.is_some() || ip.hostname.is_some())) } } @@ -887,7 +850,10 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - assert!(is_service_loadbalancer_provisioned().matches_object(Some(&s))) + assert_eq!( + is_service_loadbalancer_provisioned().matches_object_option(Some(&s)), + None + ) } #[test] From 7c9bacad6126ca2ce18cf64e1e0b738b3e791742 Mon Sep 17 00:00:00 2001 From: clux Date: Wed, 12 Mar 2025 23:39:44 +0000 Subject: [PATCH 3/6] rename a bit and make tests work Signed-off-by: clux --- kube-runtime/src/wait.rs | 126 ++++++++++++++++++++++----------------- 1 file changed, 70 insertions(+), 56 deletions(-) diff --git a/kube-runtime/src/wait.rs b/kube-runtime/src/wait.rs index 1022d3109..da0e766f2 100644 --- a/kube-runtime/src/wait.rs +++ b/kube-runtime/src/wait.rs @@ -56,7 +56,7 @@ where { // Skip updates until the condition is satisfied. let mut stream = pin!(watch_object(api, name).try_skip_while(|obj| { - let matches = cond.matches_object(obj.as_ref()); + let matches = cond.matches_ok(obj.as_ref()); future::ready(Ok(!matches)) })); @@ -80,18 +80,27 @@ where /// use k8s_openapi::api::core::v1::Pod; /// fn my_custom_condition(my_cond: &str) -> impl Condition + '_ { /// move |obj: Option<&Pod>| { -/// let cond = obj.status.as_ref()?.conditions.as_ref()?.iter().find(|c| c.type_ == my_cond); +/// let cond = obj?.status.as_ref()?.conditions.as_ref()?.iter().find(|c| c.type_ == my_cond)?; /// Some(cond.status == "True") /// } /// } /// ``` pub trait Condition { - /// Condition function giving a clear answer - fn matches_object(&self, obj: Option<&K>) -> bool { - self.matches_object_option(obj).unwrap_or_default() + /// Condition function for general truthiness + /// + /// This function does NOT distinguish between a missing property and property found to be false. + /// Use only whenever this distinction does not matter. + fn matches_ok(&self, obj: Option<&K>) -> bool { + self.matches(obj).unwrap_or_default() } - /// Condition function giving an option wrapped answer - fn matches_object_option(&self, _obj: Option<&K>) -> Option; + + /// Condition function with a clear answer + /// + /// This function is the raw underlying fn used in an `impl Condition` distinguishing missing and false information. + /// + /// This function must return None when require properties are not found. + /// If the properties are found, but the condition is not satisfied, it must return Some(false). + fn matches(&self, _obj: Option<&K>) -> Option; /// Returns a `Condition` that holds if `self` does not /// @@ -99,9 +108,9 @@ pub trait Condition { /// /// ``` /// # use kube_runtime::wait::Condition; - /// let condition: fn(Option<&()>) -> bool = |_| true; - /// assert!(condition.matches_object(None)); - /// assert!(!condition.not().matches_object(None)); + /// let condition: fn(Option<&()>) -> Option = |_| Some(true); + /// assert!(condition.matches_ok(None)); + /// assert!(!condition.not().matches_ok(None)); /// ``` fn not(self) -> conditions::Not where @@ -116,12 +125,12 @@ pub trait Condition { /// /// ``` /// # use kube_runtime::wait::Condition; - /// let cond_false: fn(Option<&()>) -> bool = |_| false; - /// let cond_true: fn(Option<&()>) -> bool = |_| true; - /// assert!(!cond_false.and(cond_false).matches_object(None)); - /// assert!(!cond_false.and(cond_true).matches_object(None)); - /// assert!(!cond_true.and(cond_false).matches_object(None)); - /// assert!(cond_true.and(cond_true).matches_object(None)); + /// let cond_false: fn(Option<&()>) -> Option = |_| Some(false); + /// let cond_true: fn(Option<&()>) -> Option = |_| Some(true); + /// assert!(!cond_false.and(cond_false).matches_ok(None)); + /// assert!(!cond_false.and(cond_true).matches_ok(None)); + /// assert!(!cond_true.and(cond_false).matches_ok(None)); + /// assert!(cond_true.and(cond_true).matches_ok(None)); /// ``` fn and>(self, other: Other) -> conditions::And where @@ -136,12 +145,12 @@ pub trait Condition { /// /// ``` /// # use kube_runtime::wait::Condition; - /// let cond_false: fn(Option<&()>) -> bool = |_| false; - /// let cond_true: fn(Option<&()>) -> bool = |_| true; - /// assert!(!cond_false.or(cond_false).matches_object(None)); - /// assert!(cond_false.or(cond_true).matches_object(None)); - /// assert!(cond_true.or(cond_false).matches_object(None)); - /// assert!(cond_true.or(cond_true).matches_object(None)); + /// let cond_false: fn(Option<&()>) -> Option = |_| Some(false); + /// let cond_true: fn(Option<&()>) -> Option = |_| Some(true); + /// assert!(!cond_false.or(cond_false).matches_ok(None)); + /// assert!(cond_false.or(cond_true).matches_ok(None)); + /// assert!(cond_true.or(cond_false).matches_ok(None)); + /// assert!(cond_true.or(cond_true).matches_ok(None)); /// ``` fn or>(self, other: Other) -> conditions::Or where @@ -151,13 +160,8 @@ pub trait Condition { } } -//impl) -> bool> Condition for F { -// fn matches_object(&self, obj: Option<&K>) -> bool { -// (self)(obj) -// } -//} impl) -> Option> Condition for F { - fn matches_object_option(&self, obj: Option<&K>) -> Option { + fn matches(&self, obj: Option<&K>) -> Option { (self)(obj) } } @@ -243,6 +247,10 @@ pub mod conditions { #[must_use] pub fn is_service_loadbalancer_provisioned() -> impl Condition { |obj: Option<&Service>| { + // ignore services that are not type LoadBalancer (return false) + if obj?.spec.as_ref()?.type_.as_ref()? != "LoadBalancer" { + return Some(false); + } let status = obj?.status.as_ref()?; let ingress = status.load_balancer.as_ref()?.ingress.as_ref()?; Some(ingress.iter().all(|ip| ip.ip.is_some() || ip.hostname.is_some())) @@ -263,8 +271,8 @@ pub mod conditions { #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct Not(pub(super) A); impl, K> Condition for Not { - fn matches_object_option(&self, obj: Option<&K>) -> Option { - Some(!self.0.matches_object(obj)) + fn matches(&self, obj: Option<&K>) -> Option { + Some(!self.0.matches_ok(obj)) } } @@ -276,8 +284,8 @@ pub mod conditions { A: Condition, B: Condition, { - fn matches_object_option(&self, obj: Option<&K>) -> Option { - Some(self.0.matches_object(obj) && self.1.matches_object(obj)) + fn matches(&self, obj: Option<&K>) -> Option { + Some(self.0.matches_ok(obj) && self.1.matches_ok(obj)) } } @@ -289,8 +297,8 @@ pub mod conditions { A: Condition, B: Condition, { - fn matches_object_option(&self, obj: Option<&K>) -> Option { - Some(self.0.matches_object(obj) || self.1.matches_object(obj)) + fn matches(&self, obj: Option<&K>) -> Option { + Some(self.0.matches_ok(obj) || self.1.matches_ok(obj)) } } @@ -345,7 +353,7 @@ pub mod conditions { "#; let c = serde_yaml::from_str(crd).unwrap(); - assert!(is_crd_established().matches_object(Some(&c))) + assert!(is_crd_established().matches_ok(Some(&c))) } #[test] @@ -398,7 +406,7 @@ pub mod conditions { "#; let c = serde_yaml::from_str(crd).unwrap(); - assert!(!is_crd_established().matches_object(Some(&c))) + assert!(!is_crd_established().matches_ok(Some(&c))) } #[test] @@ -406,7 +414,7 @@ pub mod conditions { fn crd_established_missing() { use super::{is_crd_established, Condition}; - assert!(!is_crd_established().matches_object(None)) + assert!(!is_crd_established().matches_ok(None)) } #[test] @@ -465,7 +473,7 @@ pub mod conditions { "#; let p = serde_yaml::from_str(pod).unwrap(); - assert!(is_pod_running().matches_object(Some(&p))) + assert!(is_pod_running().matches_ok(Some(&p))) } #[test] @@ -499,7 +507,7 @@ pub mod conditions { "#; let p = serde_yaml::from_str(pod).unwrap(); - assert!(!is_pod_running().matches_object(Some(&p))) + assert!(!is_pod_running().matches_ok(Some(&p))) } #[test] @@ -507,7 +515,7 @@ pub mod conditions { fn pod_running_missing() { use super::{is_pod_running, Condition}; - assert!(!is_pod_running().matches_object(None)) + assert!(!is_pod_running().matches_ok(None)) } #[test] @@ -556,7 +564,7 @@ pub mod conditions { "#; let j = serde_yaml::from_str(job).unwrap(); - assert!(is_job_completed().matches_object(Some(&j))) + assert!(is_job_completed().matches_ok(Some(&j))) } #[test] @@ -596,7 +604,7 @@ pub mod conditions { "#; let j = serde_yaml::from_str(job).unwrap(); - assert!(!is_job_completed().matches_object(Some(&j))) + assert!(!is_job_completed().matches_ok(Some(&j))) } #[test] @@ -604,7 +612,7 @@ pub mod conditions { fn job_completed_missing() { use super::{is_job_completed, Condition}; - assert!(!is_job_completed().matches_object(None)) + assert!(!is_job_completed().matches_ok(None)) } #[test] @@ -668,7 +676,7 @@ pub mod conditions { "#; let d = serde_yaml::from_str(depl).unwrap(); - assert!(is_deployment_completed().matches_object(Some(&d))) + assert!(is_deployment_completed().matches_ok(Some(&d))) } #[test] @@ -731,7 +739,7 @@ pub mod conditions { "#; let d = serde_yaml::from_str(depl).unwrap(); - assert!(!is_deployment_completed().matches_object(Some(&d))) + assert!(!is_deployment_completed().matches_ok(Some(&d))) } #[test] @@ -739,7 +747,7 @@ pub mod conditions { fn deployment_completed_missing() { use super::{is_deployment_completed, Condition}; - assert!(!is_deployment_completed().matches_object(None)) + assert!(!is_deployment_completed().matches_ok(None)) } #[test] @@ -768,7 +776,7 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - assert!(is_service_loadbalancer_provisioned().matches_object(Some(&s))) + assert!(is_service_loadbalancer_provisioned().matches_ok(Some(&s))) } #[test] @@ -797,7 +805,7 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - assert!(is_service_loadbalancer_provisioned().matches_object(Some(&s))) + assert!(is_service_loadbalancer_provisioned().matches_ok(Some(&s))) } #[test] @@ -824,7 +832,11 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - assert!(!is_service_loadbalancer_provisioned().matches_object(Some(&s))) + // returns false because it does not match the condition + assert_eq!(is_service_loadbalancer_provisioned().matches_ok(Some(&s)), false); + // but it's falsy because the properties we look for does not exist + assert_eq!(is_service_loadbalancer_provisioned().matches(Some(&s)), None); + // TODO: maybe the distinction is not valuable in this case, remove it? } #[test] @@ -850,10 +862,12 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); + // should return a definitive false because it's not a load balancer type assert_eq!( - is_service_loadbalancer_provisioned().matches_object_option(Some(&s)), - None + is_service_loadbalancer_provisioned().matches(Some(&s)), + Some(false) ) + // TODO: maybe this is not valueable ? } #[test] @@ -861,7 +875,7 @@ pub mod conditions { fn service_lb_provisioned_missing() { use super::{is_service_loadbalancer_provisioned, Condition}; - assert!(!is_service_loadbalancer_provisioned().matches_object(None)) + assert!(!is_service_loadbalancer_provisioned().matches_ok(None)) } #[test] @@ -896,7 +910,7 @@ pub mod conditions { "#; let i = serde_yaml::from_str(ingress).unwrap(); - assert!(is_ingress_provisioned().matches_object(Some(&i))) + assert!(is_ingress_provisioned().matches_ok(Some(&i))) } #[test] @@ -931,7 +945,7 @@ pub mod conditions { "#; let i = serde_yaml::from_str(ingress).unwrap(); - assert!(is_ingress_provisioned().matches_object(Some(&i))) + assert!(is_ingress_provisioned().matches_ok(Some(&i))) } #[test] @@ -964,7 +978,7 @@ pub mod conditions { "#; let i = serde_yaml::from_str(ingress).unwrap(); - assert!(!is_ingress_provisioned().matches_object(Some(&i))) + assert!(!is_ingress_provisioned().matches_ok(Some(&i))) } #[test] @@ -972,7 +986,7 @@ pub mod conditions { fn ingress_provisioned_missing() { use super::{is_ingress_provisioned, Condition}; - assert!(!is_ingress_provisioned().matches_object(None)) + assert!(!is_ingress_provisioned().matches_ok(None)) } } } From dcc77280a52bc64f356596b8719ee8cac36a477b Mon Sep 17 00:00:00 2001 From: clux Date: Wed, 12 Mar 2025 23:48:47 +0000 Subject: [PATCH 4/6] avoid changing the name of the fn that returns bool less breaking for people using it because the signature is the same! Signed-off-by: clux --- kube-runtime/src/wait.rs | 73 +++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 35 deletions(-) diff --git a/kube-runtime/src/wait.rs b/kube-runtime/src/wait.rs index da0e766f2..36ef4115c 100644 --- a/kube-runtime/src/wait.rs +++ b/kube-runtime/src/wait.rs @@ -56,7 +56,7 @@ where { // Skip updates until the condition is satisfied. let mut stream = pin!(watch_object(api, name).try_skip_while(|obj| { - let matches = cond.matches_ok(obj.as_ref()); + let matches = cond.matches_object(obj.as_ref()); future::ready(Ok(!matches)) })); @@ -90,7 +90,7 @@ pub trait Condition { /// /// This function does NOT distinguish between a missing property and property found to be false. /// Use only whenever this distinction does not matter. - fn matches_ok(&self, obj: Option<&K>) -> bool { + fn matches_object(&self, obj: Option<&K>) -> bool { self.matches(obj).unwrap_or_default() } @@ -109,8 +109,8 @@ pub trait Condition { /// ``` /// # use kube_runtime::wait::Condition; /// let condition: fn(Option<&()>) -> Option = |_| Some(true); - /// assert!(condition.matches_ok(None)); - /// assert!(!condition.not().matches_ok(None)); + /// assert!(condition.matches_object(None)); + /// assert!(!condition.not().matches_object(None)); /// ``` fn not(self) -> conditions::Not where @@ -127,10 +127,10 @@ pub trait Condition { /// # use kube_runtime::wait::Condition; /// let cond_false: fn(Option<&()>) -> Option = |_| Some(false); /// let cond_true: fn(Option<&()>) -> Option = |_| Some(true); - /// assert!(!cond_false.and(cond_false).matches_ok(None)); - /// assert!(!cond_false.and(cond_true).matches_ok(None)); - /// assert!(!cond_true.and(cond_false).matches_ok(None)); - /// assert!(cond_true.and(cond_true).matches_ok(None)); + /// assert!(!cond_false.and(cond_false).matches_object(None)); + /// assert!(!cond_false.and(cond_true).matches_object(None)); + /// assert!(!cond_true.and(cond_false).matches_object(None)); + /// assert!(cond_true.and(cond_true).matches_object(None)); /// ``` fn and>(self, other: Other) -> conditions::And where @@ -147,10 +147,10 @@ pub trait Condition { /// # use kube_runtime::wait::Condition; /// let cond_false: fn(Option<&()>) -> Option = |_| Some(false); /// let cond_true: fn(Option<&()>) -> Option = |_| Some(true); - /// assert!(!cond_false.or(cond_false).matches_ok(None)); - /// assert!(cond_false.or(cond_true).matches_ok(None)); - /// assert!(cond_true.or(cond_false).matches_ok(None)); - /// assert!(cond_true.or(cond_true).matches_ok(None)); + /// assert!(!cond_false.or(cond_false).matches_object(None)); + /// assert!(cond_false.or(cond_true).matches_object(None)); + /// assert!(cond_true.or(cond_false).matches_object(None)); + /// assert!(cond_true.or(cond_true).matches_object(None)); /// ``` fn or>(self, other: Other) -> conditions::Or where @@ -272,7 +272,7 @@ pub mod conditions { pub struct Not(pub(super) A); impl, K> Condition for Not { fn matches(&self, obj: Option<&K>) -> Option { - Some(!self.0.matches_ok(obj)) + Some(!self.0.matches_object(obj)) } } @@ -285,7 +285,7 @@ pub mod conditions { B: Condition, { fn matches(&self, obj: Option<&K>) -> Option { - Some(self.0.matches_ok(obj) && self.1.matches_ok(obj)) + Some(self.0.matches_object(obj) && self.1.matches_object(obj)) } } @@ -298,7 +298,7 @@ pub mod conditions { B: Condition, { fn matches(&self, obj: Option<&K>) -> Option { - Some(self.0.matches_ok(obj) || self.1.matches_ok(obj)) + Some(self.0.matches_object(obj) || self.1.matches_object(obj)) } } @@ -353,7 +353,7 @@ pub mod conditions { "#; let c = serde_yaml::from_str(crd).unwrap(); - assert!(is_crd_established().matches_ok(Some(&c))) + assert!(is_crd_established().matches_object(Some(&c))) } #[test] @@ -406,7 +406,7 @@ pub mod conditions { "#; let c = serde_yaml::from_str(crd).unwrap(); - assert!(!is_crd_established().matches_ok(Some(&c))) + assert!(!is_crd_established().matches_object(Some(&c))) } #[test] @@ -414,7 +414,7 @@ pub mod conditions { fn crd_established_missing() { use super::{is_crd_established, Condition}; - assert!(!is_crd_established().matches_ok(None)) + assert!(!is_crd_established().matches_object(None)) } #[test] @@ -473,7 +473,7 @@ pub mod conditions { "#; let p = serde_yaml::from_str(pod).unwrap(); - assert!(is_pod_running().matches_ok(Some(&p))) + assert!(is_pod_running().matches_object(Some(&p))) } #[test] @@ -507,7 +507,7 @@ pub mod conditions { "#; let p = serde_yaml::from_str(pod).unwrap(); - assert!(!is_pod_running().matches_ok(Some(&p))) + assert!(!is_pod_running().matches_object(Some(&p))) } #[test] @@ -515,7 +515,7 @@ pub mod conditions { fn pod_running_missing() { use super::{is_pod_running, Condition}; - assert!(!is_pod_running().matches_ok(None)) + assert!(!is_pod_running().matches_object(None)) } #[test] @@ -564,7 +564,7 @@ pub mod conditions { "#; let j = serde_yaml::from_str(job).unwrap(); - assert!(is_job_completed().matches_ok(Some(&j))) + assert!(is_job_completed().matches_object(Some(&j))) } #[test] @@ -604,7 +604,7 @@ pub mod conditions { "#; let j = serde_yaml::from_str(job).unwrap(); - assert!(!is_job_completed().matches_ok(Some(&j))) + assert!(!is_job_completed().matches_object(Some(&j))) } #[test] @@ -612,7 +612,7 @@ pub mod conditions { fn job_completed_missing() { use super::{is_job_completed, Condition}; - assert!(!is_job_completed().matches_ok(None)) + assert!(!is_job_completed().matches_object(None)) } #[test] @@ -676,7 +676,7 @@ pub mod conditions { "#; let d = serde_yaml::from_str(depl).unwrap(); - assert!(is_deployment_completed().matches_ok(Some(&d))) + assert!(is_deployment_completed().matches_object(Some(&d))) } #[test] @@ -739,7 +739,7 @@ pub mod conditions { "#; let d = serde_yaml::from_str(depl).unwrap(); - assert!(!is_deployment_completed().matches_ok(Some(&d))) + assert!(!is_deployment_completed().matches_object(Some(&d))) } #[test] @@ -747,7 +747,7 @@ pub mod conditions { fn deployment_completed_missing() { use super::{is_deployment_completed, Condition}; - assert!(!is_deployment_completed().matches_ok(None)) + assert!(!is_deployment_completed().matches_object(None)) } #[test] @@ -776,7 +776,7 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - assert!(is_service_loadbalancer_provisioned().matches_ok(Some(&s))) + assert!(is_service_loadbalancer_provisioned().matches_object(Some(&s))) } #[test] @@ -805,7 +805,7 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - assert!(is_service_loadbalancer_provisioned().matches_ok(Some(&s))) + assert!(is_service_loadbalancer_provisioned().matches_object(Some(&s))) } #[test] @@ -833,7 +833,10 @@ pub mod conditions { let s = serde_yaml::from_str(service).unwrap(); // returns false because it does not match the condition - assert_eq!(is_service_loadbalancer_provisioned().matches_ok(Some(&s)), false); + assert_eq!( + is_service_loadbalancer_provisioned().matches_object(Some(&s)), + false + ); // but it's falsy because the properties we look for does not exist assert_eq!(is_service_loadbalancer_provisioned().matches(Some(&s)), None); // TODO: maybe the distinction is not valuable in this case, remove it? @@ -875,7 +878,7 @@ pub mod conditions { fn service_lb_provisioned_missing() { use super::{is_service_loadbalancer_provisioned, Condition}; - assert!(!is_service_loadbalancer_provisioned().matches_ok(None)) + assert!(!is_service_loadbalancer_provisioned().matches_object(None)) } #[test] @@ -910,7 +913,7 @@ pub mod conditions { "#; let i = serde_yaml::from_str(ingress).unwrap(); - assert!(is_ingress_provisioned().matches_ok(Some(&i))) + assert!(is_ingress_provisioned().matches_object(Some(&i))) } #[test] @@ -945,7 +948,7 @@ pub mod conditions { "#; let i = serde_yaml::from_str(ingress).unwrap(); - assert!(is_ingress_provisioned().matches_ok(Some(&i))) + assert!(is_ingress_provisioned().matches_object(Some(&i))) } #[test] @@ -978,7 +981,7 @@ pub mod conditions { "#; let i = serde_yaml::from_str(ingress).unwrap(); - assert!(!is_ingress_provisioned().matches_ok(Some(&i))) + assert!(!is_ingress_provisioned().matches_object(Some(&i))) } #[test] @@ -986,7 +989,7 @@ pub mod conditions { fn ingress_provisioned_missing() { use super::{is_ingress_provisioned, Condition}; - assert!(!is_ingress_provisioned().matches_ok(None)) + assert!(!is_ingress_provisioned().matches_object(None)) } } } From 2ba1a8e321b213c29cde2863f320e2575cd0c644 Mon Sep 17 00:00:00 2001 From: clux Date: Wed, 12 Mar 2025 23:58:20 +0000 Subject: [PATCH 5/6] fix integration test Signed-off-by: clux --- kube/src/lib.rs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/kube/src/lib.rs b/kube/src/lib.rs index 615f27353..aca04b6ef 100644 --- a/kube/src/lib.rs +++ b/kube/src/lib.rs @@ -519,16 +519,9 @@ mod test { // TODO: remove these once we can write these functions generically fn is_each_container_ready() -> impl Condition { |obj: Option<&Pod>| { - if let Some(o) = obj { - if let Some(s) = &o.status { - if let Some(conds) = &s.conditions { - if let Some(pcond) = conds.iter().find(|c| c.type_ == "ContainersReady") { - return pcond.status == "True"; - } - } - } - } - false + let conds = obj?.status.as_ref()?.conditions.as_ref()?; + let pcond = conds.iter().find(|c| c.type_ == "ContainersReady")?; + Some(pcond.status == "True") } } let is_fully_ready = await_condition( From e3b341cad0f63b1eed3ea1a7b3be8010b36d1066 Mon Sep 17 00:00:00 2001 From: clux Date: Thu, 13 Mar 2025 00:09:08 +0000 Subject: [PATCH 6/6] docs Signed-off-by: clux --- kube-runtime/src/wait.rs | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/kube-runtime/src/wait.rs b/kube-runtime/src/wait.rs index 36ef4115c..11b17b81e 100644 --- a/kube-runtime/src/wait.rs +++ b/kube-runtime/src/wait.rs @@ -86,19 +86,18 @@ where /// } /// ``` pub trait Condition { - /// Condition function for general truthiness + /// Condition function with bool return /// - /// This function does NOT distinguish between a missing property and property found to be false. - /// Use only whenever this distinction does not matter. + /// This function does NOT distinguish between a missing property and property declared to be false. fn matches_object(&self, obj: Option<&K>) -> bool { self.matches(obj).unwrap_or_default() } - /// Condition function with a clear answer + /// Condition function with optional return /// - /// This function is the raw underlying fn used in an `impl Condition` distinguishing missing and false information. + /// This function is the raw underlying fn used in an `impl Condition` distinguishing missing and false conditions. /// - /// This function must return None when require properties are not found. + /// This function should return None when required properties are missing. /// If the properties are found, but the condition is not satisfied, it must return Some(false). fn matches(&self, _obj: Option<&K>) -> Option; @@ -247,7 +246,7 @@ pub mod conditions { #[must_use] pub fn is_service_loadbalancer_provisioned() -> impl Condition { |obj: Option<&Service>| { - // ignore services that are not type LoadBalancer (return false) + // explicitly reject services that are not type LoadBalancer if obj?.spec.as_ref()?.type_.as_ref()? != "LoadBalancer" { return Some(false); } @@ -832,14 +831,10 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - // returns false because it does not match the condition - assert_eq!( - is_service_loadbalancer_provisioned().matches_object(Some(&s)), - false - ); - // but it's falsy because the properties we look for does not exist + // matches object is false because it does not match the condition + assert!(!is_service_loadbalancer_provisioned().matches_object(Some(&s)),); + // but via None because the underlying matches method is missing properties assert_eq!(is_service_loadbalancer_provisioned().matches(Some(&s)), None); - // TODO: maybe the distinction is not valuable in this case, remove it? } #[test] @@ -865,12 +860,13 @@ pub mod conditions { "; let s = serde_yaml::from_str(service).unwrap(); - // should return a definitive false because it's not a load balancer type + // false; not matching because it's not a load balancer + assert!(!is_service_loadbalancer_provisioned().matches_object(Some(&s)),); + // but via explicit false, because method rejected the value of the properties assert_eq!( is_service_loadbalancer_provisioned().matches(Some(&s)), Some(false) ) - // TODO: maybe this is not valueable ? } #[test]