-
Notifications
You must be signed in to change notification settings - Fork 23
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
CEL Interperter for Conditions #354
Conversation
…n parsing if the condition has the `cel:` prefix. Signed-off-by: Hiram Chirino <[email protected]>
Signed-off-by: Hiram Chirino <[email protected]>
Signed-off-by: Hiram Chirino <[email protected]>
Signed-off-by: Hiram Chirino <[email protected]>
…s with cel built in expressions. Signed-off-by: Hiram Chirino <[email protected]>
limitador/src/limit.rs
Outdated
|
||
// we can't access simple variables that conflict with built-ins (the vars map) | ||
let limit = limit_with_condition(vec![r#"cel: vars == "hello" "#]); | ||
assert!(!limit.applies(&values)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Real minor neat, but the !
I find so easy to miss (tho you did put a clear comment there), and clippy will complain about assert_eq!(cond, false)
, what I recently started doing for these assert!(!false)
, is that I use std::ops::Not
and try to express these as assert!(limit.applies(&values).not())
instead. Now don't feel you need to use the same pattern, just sharing…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I miss it all the time.. kinda wish there was an assert_not!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just bump the cel-interpreter (if only in the lock file)
But looks good to me! Great addition I think… There might be more work to do with this (e.g. possibly not allow for crazy complicated expression, make sure the expression results in a Bool one, more?…)
limitador/Cargo.toml
Outdated
@@ -33,6 +34,8 @@ async-trait = "0.1" | |||
cfg-if = "1" | |||
tracing = "0.1.40" | |||
metrics = "0.22.3" | |||
cel-parser = "0.6.0" | |||
cel-interpreter = "0.5.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.7 is out and you inherited my lock file I guess… might deal with our size
vs size()
better...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
Signed-off-by: Hiram Chirino <[email protected]>
helper functions. Signed-off-by: Hiram Chirino <[email protected]>
@@ -915,7 +923,7 @@ mod tests { | |||
values.insert("x".into(), "1".into()); | |||
values.insert("y".into(), "1".into()); | |||
|
|||
assert!(!limit.applies(&values)) | |||
assert_false!(limit.applies(&values)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉 ...
But so you know this will report in a slightly confusing way:
assertion failed: !limit.applies(&values)
with the change above it'd be, I think clearer:
assertion failed: assert_false!(limit.applies(&values)) was true!
where now assert_false!(limit.applies(&values))
matches code the user wrote
limitador/src/limit.rs
Outdated
macro_rules! assert_false { | ||
($cond:expr $(,)?) => { | ||
paste::item! { | ||
assert!(!$cond) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below, so I'd change this to:
- assert!(!$cond)
+ assert!(!$cond, "assertion failed: assert_false!({}) was true!", stringify!($cond))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sounds good to me. updated.
Signed-off-by: Hiram Chirino <[email protected]>
Signed-off-by: Hiram Chirino <[email protected]>
Signed-off-by: Hiram Chirino <[email protected]>
Closing this in favor of #396 which drops support for all the legacy syntax altogether |
Continues on #254 in a way that is backward compatible. To use cel based conditions, you must prefix your condition with
cel: