Introduces a DynamoDB client and implements a method to fetch PAKE registration data from a DDB table.
Depends on D3852
Details
Details
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| services/identity/src/database.rs | ||
|---|---|---|
| 30 ↗ | (On Diff #11904) | This should probably be a const, will fix |
| 40 ↗ | (On Diff #11904) | This string slice should be a const too |
| 70 ↗ | (On Diff #11904) | Defaulting to us-east-2 for now, since this is where all our tables are currently. We can revise this later to fetch the region from config |
| services/identity/src/service.rs | ||
| 18 ↗ | (On Diff #11904) | The Rusoto DynamoDbClient doesn't implement Debug so we can't derive Debug for MyIdentityService |
| 29 ↗ | (On Diff #11904) | Instruments a function to create and enter a tracing span every time the function is called. We skip self since we don't have a Debug implementation for it currently |
| services/identity/src/database.rs | ||
|---|---|---|
| 23 ↗ | (On Diff #11904) | this string slice should be a const too |
| services/identity/src/database.rs | ||
|---|---|---|
| 5 ↗ | (On Diff #11904) | do we use DynamoDb? |
| 15 ↗ | (On Diff #11904) | so this new method is just a convention, right? |
| 25–26 ↗ | (On Diff #11904) | can you explain what these do? |
| 32 ↗ | (On Diff #11904) | so this means that consistent_read is of type Option<bool> and we just want to set its value to true, right? |
| 33 ↗ | (On Diff #11904) | So that uses default values for the rest of the fields of GetItemInput? And this is done by impl Default for ...? |
| 38 ↗ | (On Diff #11904) | What does this mean? What is item inside of Some? |
| 40 ↗ | (On Diff #11904) | shouldn't we use a better name than b? |
Comment Actions
Shouldn't @jimpo be a blocker reviewer for every identity diff since he was pointed out as one of two devs in the team who knows rust well (next to @varun)?
Comment Actions
This diff is rather big - is there a way to split it somehow? It also looks like the diff does at least two things: we also add some instrumentation, which probably can stand on its own (correct me if I'm wrong).
@karol-bisztyga you can request changes even if it's just for the clarification. Also, when you think someone should be a blocking reviewer, you should add them.
Comment Actions
Yeah, the tracing instrumentation should be in its own diff. I'm moving the get_pake_registration method to a separate diff. cargo emits warnings if code is unused, so I was trying to avoid that, but it's not a big deal.
| services/identity/src/database.rs | ||
|---|---|---|
| 5 ↗ | (On Diff #11904) | Yeah, DynamoDb is a trait implemented by DynamoDbClient, but the trait impl is only available if we import it. We need this implementation to call get_item() |
| 15 ↗ | (On Diff #11904) | Yeah it's just a constructor. We can name it anything, new is just the norm |
| 25–26 ↗ | (On Diff #11904) | AttributeValue represents the data for an attribute. userID is a string so we set the s field to the user_id String and then set the rest to their defaults (None) |
| 32 ↗ | (On Diff #11904) | correct |
| 33 ↗ | (On Diff #11904) | correct |
| 38 ↗ | (On Diff #11904) | I'm checking if the item field, which is an Option<HashMap<String, AttributeValue>>, is the Some variant, and then assigning the HashMap to a variable named item so I can use it on line 40. |
| 40 ↗ | (On Diff #11904) | Yeah, fair enough. I'll make it bytes or something |