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
- Branch
- varun/identity_service (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| services/identity/src/database.rs | ||
|---|---|---|
| 30 | This should probably be a const, will fix | |
| 40 | This string slice should be a const too | |
| 70 | 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 | The Rusoto DynamoDbClient doesn't implement Debug so we can't derive Debug for MyIdentityService | |
| 29 | 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 | this string slice should be a const too | |
| services/identity/src/database.rs | ||
|---|---|---|
| 5 | do we use DynamoDb? | |
| 15 | so this new method is just a convention, right? | |
| 25–26 | can you explain what these do? | |
| 32 | so this means that consistent_read is of type Option<bool> and we just want to set its value to true, right? | |
| 33 | So that uses default values for the rest of the fields of GetItemInput? And this is done by impl Default for ...? | |
| 38 | What does this mean? What is item inside of Some? | |
| 40 | 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 | 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 | Yeah it's just a constructor. We can name it anything, new is just the norm | |
| 25–26 | 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 | correct | |
| 33 | correct | |
| 38 | 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 | Yeah, fair enough. I'll make it bytes or something | |