Add run_command_async
#993
Conversation
|
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #993 +/- ##
==========================================
- Coverage 58.31% 58.08% -0.24%
==========================================
Files 193 193
Lines 13553 13608 +55
==========================================
Hits 7904 7904
- Misses 5649 5704 +55 ☔ View full report in Codecov by Sentry. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
crates/bitwarden-core/Cargo.toml
Outdated
| [target.'cfg(debug_assertions)'.dependencies] | ||
| tokio = { version = "1.36.0", features = ["rt", "macros", "time"] } |
There was a problem hiding this comment.
I barely know what I did here, I need the time feature but only in debug, is this the right way to go?
There was a problem hiding this comment.
It is, but I can't say I'm a fan of different behaviors on debug and release builds.
There was a problem hiding this comment.
This feels like the exact thing a debug build is for, it helps you write tests to make sure you've built the language wrapper correctly. That said I was able to move it into the bitwarden-json crate instead of core.
|
I added support for |
|
|
||
| #[cfg(debug_assertions)] | ||
| pub async fn cancellation_test(&mut self, duration_millis: u64) -> Result<i32> { | ||
| use std::time::Duration; | ||
|
|
||
| tokio::time::sleep(Duration::from_millis(duration_millis)).await; | ||
| println!("After wait #1"); | ||
| tokio::time::sleep(Duration::from_millis(duration_millis)).await; | ||
| println!("After wait #2"); | ||
| tokio::time::sleep(Duration::from_millis(duration_millis)).await; | ||
| println!("After wait #3"); | ||
| Ok(42) | ||
| } | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| pub fn error_test(&mut self) -> Result<i32> { | ||
| use crate::Error; | ||
|
|
||
| Err(Error::Internal(std::borrow::Cow::Borrowed( | ||
| "This is an error.", | ||
| ))) | ||
| } |
There was a problem hiding this comment.
Are these needed in the core crate? Consider moving them to a separate test crate, or bitwarden-c, or remove it completely.
There was a problem hiding this comment.
I moved them to bitwarden-json, if I moved it into bitwarden-c I would have to double parse the input string to check for special commands. Do you think that is alright?
There was a problem hiding this comment.
Why not just expose two regular functions for it?
There was a problem hiding this comment.
The cancellation test is mostly testing the very code written in run_command_async so we'd have to expose a method with very similar internals to that and repeat ourselves. The error test is testing that the C# wrapper properly handles how errors are given back to it through the bitwarden-json crate.
There was a problem hiding this comment.
I'll let @bitwarden/team-secrets-manager-dev decide if they want to accept net 8 as the new target.
crates/bitwarden-core/Cargo.toml
Outdated
| [target.'cfg(debug_assertions)'.dependencies] | ||
| tokio = { version = "1.36.0", features = ["rt", "macros", "time"] } |
There was a problem hiding this comment.
It is, but I can't say I'm a fan of different behaviors on debug and release builds.
|
I added SM as reviewers, we should try and get this in before the 1.0 release of the C# sdk. |
Co-authored-by: Oscar Hinton <[email protected]>
Co-authored-by: Oscar Hinton <[email protected]>
… add-run-command-async
| #[cfg(debug_assertions)] | ||
| #[derive(Serialize, Deserialize, JsonSchema, Debug)] | ||
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
| pub enum DebugCommand { | ||
| CancellationTest { duration_millis: u64 }, | ||
| ErrorTest {}, | ||
| } |
There was a problem hiding this comment.
An alternative way. of doing this would be to add one or two separate commands to bitwarden-c.
There was a problem hiding this comment.
This looks good to me, thanks @justindbaur!
Have you tested that state files still work on the client creation and during token renewal?
🎟️ Tracking
📔 Objective
[LibraryImport]for auto-marshalling of functionrun_command_asyncmethod inbitwarden-cthat allows for a non-blocking call that can notify the caller when the action is completed.Task-ifies one of the C# SDK calls into the SDK.1.0.0release.📸 Screenshots
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes