Page MenuHomePhabricator

[Tunnelbroker] Remove usages of unwrap and expect

Authored by jon on Fri, May 12, 3:12 PM.
Referenced Files
Unknown Object (File)
Fri, May 26, 1:17 AM
Unknown Object (File)
Thu, May 25, 5:43 PM
Unknown Object (File)
Sun, May 21, 1:25 AM



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 && ./

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

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

jon requested review of this revision.Fri, May 12, 3:31 PM
This revision is now accepted and ready to land.Tue, May 16, 2:43 PM
bartek requested changes to this revision.Wed, May 17, 10:09 PM

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

16–19 ↗(On Diff #26424)

Maybe we could use types from comm-services-lib/

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.Wed, May 17, 10:09 PM
jon requested review of this revision.Tue, May 30, 6:19 AM
jon marked 2 inline comments as done.
jon added inline comments.
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.


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.Tue, May 30, 6:30 AM
jon marked an inline comment as done.

Rebase on master and feedback in other diffs