Page MenuHomePhabricator

[Tunnelbroker] Remove usages of unwrap and expect
ClosedPublic

Authored by jon on May 12 2023, 3:12 PM.
Tags
None
Referenced Files
F3409956: D7801.diff
Wed, Dec 4, 7:09 PM
Unknown Object (File)
Nov 1 2024, 5:48 AM
Unknown Object (File)
Nov 1 2024, 5:48 AM
Unknown Object (File)
Nov 1 2024, 5:48 AM
Unknown Object (File)
Nov 1 2024, 5:48 AM
Unknown Object (File)
Nov 1 2024, 5:40 AM
Unknown Object (File)
Sep 29 2024, 9:14 AM
Unknown Object (File)
Sep 29 2024, 9:14 AM
Subscribers

Details

Summary

Unwrap and expect were convenient for the happy cases. However,
they are unsatisfactory for actual edge cases as they cause panic which
puts tunnelbroker into a semi-broken state.

Depends on D7800

Test Plan
nix develop

# init localstack and run terraform
comm-dev services start
(cd services/terraform && ./run.sh)

(cd services/tunnelbroker && cargo run &)
(cd services/commtest && cargo test --test tunnelbroker_integration_test)

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

This revision is now accepted and ready to land.May 16 2023, 2:43 PM
bartek requested changes to this revision.May 17 2023, 10:09 PM

Requesting changes only to get answer about using comm-services-lib

services/tunnelbroker/src/database/message.rs
16–19 ↗(On Diff #26424)

Maybe we could use types from comm-services-lib/database.rs?

25–42 ↗(On Diff #26424)

Same as above, in comm-services-lib we have things as parse_string_attribute(), parse_number() etc... Perhaps they can be used here?

This revision now requires changes to proceed.May 17 2023, 10:09 PM
jon marked 2 inline comments as done.
jon added inline comments.
services/tunnelbroker/src/database/message.rs
25–42 ↗(On Diff #26424)

I tried to use these utilities. But ran into some issues with imports. comm-services-lib still uses aws-sdk-dynamodb v0.24, while I'm currently using v0.27. They changed where AttributeValue is located.

In v0.24, AttributeValue is in dynamodb::model, but in v0.27 it's dynamodb::types. Although reverting would be pretty easy for this diff. It would be a big change for much of my other code.

I think the best path forward would be to update comm-services-lib, and then apply your suggestion.

Created: https://linear.app/comm/issue/ENG-3960

Fair enough, accepting to unblock you now, but we definitely need to prioritize the comm-services-lib sync, or maintaining the lib will bring more costs than benefits. Anyway, this can be discussed in the Linear task you created ;)

This revision is now accepted and ready to land.May 30 2023, 6:30 AM
jon marked an inline comment as done.

Rebase on master and feedback in other diffs