Page MenuHomePhabricator

[Tunnelbroker] Remove usages of unwrap and expect
ClosedPublic

Authored by jon on May 12 2023, 3:12 PM.
Tags
None
Referenced Files
F1569303: D7801.id27355.diff
Thu, Apr 18, 3:18 AM
F1569302: D7801.id27334.diff
Thu, Apr 18, 3:18 AM
F1569301: D7801.id26424.diff
Thu, Apr 18, 3:18 AM
F1569292: D7801.id.diff
Thu, Apr 18, 3:18 AM
F1569285: D7801.diff
Thu, Apr 18, 3:12 AM
Unknown Object (File)
Sat, Apr 13, 3:19 AM
Unknown Object (File)
Wed, Apr 3, 12:26 PM
Unknown Object (File)
Feb 10 2024, 8:23 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
Branch
jonringer/tunnelbroker-dynamodb (branched from master)
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

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

25–42

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

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