Details
- Reviewers
bartek varun kamil - Commits
- rCOMM1cadde3e5aa5: [commtest] Move backup client to shared
- cargo run in backup and blob
- yarn run-integration-tests backup
- yarn run-performance-tests blob
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
shared/backup_client/Cargo.toml | ||
---|---|---|
6–17 ↗ | (On Diff #34433) | All dependencies with these specific versions are already being used in our codebase. |
19–22 ↗ | (On Diff #34433) | This will be required for native. On Android we have to use rustls-tls because the native-tls bundles openssl which conflicts with our openssl setup during linking. I added it in this diff to make sure that the commtest works with default features (native-tls). |
shared/backup_client/src/lib.rs | ||
10–20 ↗ | (On Diff #34433) | Instead of passing url to each function call I added a struct that will hold the configuration. |
37–40 ↗ | (On Diff #34433) | Previously the user of the backup client passed in the blob hash. Now we just calculate it here. |
41 ↗ | (On Diff #34433) | Previously it was: Part::stream(Body::wrap_stream( stream! { yield Ok::<Vec<u8>, Infallible>(user_keys); }, )), I changed it because I don't think it gave us anything -> we were still sending one big chunk (it wasn't really "streaming"). |
56 ↗ | (On Diff #34433) | Previously this was converted to commtest-specific error. Now we just return reqwest::Error if something went wrong. |
63 ↗ | (On Diff #34433) | Previously this took ownership to BackupDescriptor. I changed it to take a reference as we don't need the complete ownership of the value inside the function. |
123–130 ↗ | (On Diff #34433) | Added an error struct. |
sorry for the delay. accepting, but with a question: how are we setting the CA Certificate against which to verify the server’s TLS certificate?
shared/backup_client/src/lib.rs | ||
---|---|---|
47 ↗ | (On Diff #34433) | can we make the newline character ("\n") a named constant? |
From reqwest docs we can see that rustls-tls feature is equivalent to rustls-tls-webpki-roots, which uses the wepki-roots crate. There is an alternative crate rustls-native-certs (that can be activated with a feature) but the docs don't say anything about Android/ iOS compatibility and I haven't tested it.
shared/backup_client/src/lib.rs | ||
---|---|---|
123–130 ↗ | (On Diff #34433) | To be honest I think derive_more get's us to about 95% percent of what thiserror gives us, with a slightly worse syntax. I would prefer to switch to thiserror but it would be better to change it throughout the whole codebase. |