feat: turn on event validation by default #562
Conversation
samika98
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
…efine an action on them to set them
@nathanielc tested it and turns out clap is weird with booleans. See this discussion : clap-rs/clap#1649 (comment). Turns out we need to explicitly pass the boolean and set the action so that we can set the variable. |
samika98
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
one/src/daemon.rs
Outdated
| value_parser = clap::value_parser!(bool), | ||
| num_args = 0..=1, | ||
| action = ArgAction::Set, | ||
| env = "CERAMIC_ONE_EVENT_VALIDATION" | ||
| )] | ||
| event_validation: bool, |
There was a problem hiding this comment.
Re booleans being weird in clap. I think all we need to do re the issue you linked is set the to Option<bool> and then default it to true in our code like, opts.event_validation.unwrap_or(true).
There was a problem hiding this comment.
Yeah! we make it optional and don'e set a default value. Seems to work :
warning: unused import: `ArgAction`
--> one/src/daemon.rs:17:12
|
17 | use clap::{ArgAction, Args};
| ^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: `ceramic-one` (lib) generated 1 warning (run `cargo fix --lib -p ceramic-one` to apply 1 suggestion)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.51s
Running `target/debug/ceramic-one daemon`
2024-10-22T18:08:19.257286Z INFO ceramic_one::daemon: Event service created, event_validation: true
^C
samikas@Samikas-MacBook-Pro rust-ceramic % RUST_LOG=info,ceramic_olap=debug cargo run --bin ceramic-one -- daemon --event-validation=false | grep "Event service created"
warning: unused import: `ArgAction`
--> one/src/daemon.rs:17:12
|
17 | use clap::{ArgAction, Args};
| ^^^^^^^^^
|
= note: `#[warn(unused_imports)]` on by default
warning: `ceramic-one` (lib) generated 1 warning (run `cargo fix --lib -p ceramic-one` to apply 1 suggestion)
Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.50s
Running `target/debug/ceramic-one daemon --event-validation=false`
2024-10-22T18:08:39.913484Z INFO ceramic_one::daemon: Event service created, event_validation: false```
There was a problem hiding this comment.
Added setting the default value using default_value. Added reasoning in the commit message incase anyone is interested in the workings of default_value vs default_value_t, why one works and the other does not
…_t=Some(true) will break the code. The default_value_t cannot be used here : The default_value_t attribute in Clap requires the type to implement Display because it needs to be able to print the default value in the help text. However, Option<T> does not implement Display, even if T does. To work around this, we use default_value instead This works because Clap will parse the string true into a bool and then wrap it in Some(...). We have additionally also handled this in the code parsing using an unwrap_or(). The default_value is added more for semantic purposes to aid readability of the code
samika98
temporarily deployed
to
github-tests-2024
GitHub Actions
Inactive
nathanielc
deleted the
feature/aes-382-enable-event-validation-by-default
branch
Do not merge - for now
I am collecting evidence that this feature has been stable on durable for over a 1 week