Details

Why you don't put [services] prefix in your revisions' names for the identity service? I thought we had a convention that we put a prefix depending on the "module" we work on. So, for instance, [web], [server], [native]. [services] Is at the same level, [identity] is deeper one level. Is there a particular reason you do not do this here? Has this rule changed?

services/identity/src/token.rs
9–12	What is the reason to use `Option` here? Why `token` is not optional and the rest of the fields are?

This revision now requires changes to proceed.May 9 2022, 2:51 AM

In D3945#110907, @karol-bisztyga wrote:

Why you don't put [services] prefix in your revisions' names for the identity service? I thought we had a convention that we put a prefix depending on the "module" we work on. So, for instance, [web], [server], [native]. [services] Is at the same level, [identity] is deeper one level. Is there a particular reason you do not do this here? Has this rule changed?

I have this pattern but I've actually never pushed it as a rule. The other day @jonringer-comm asked me if he should prefix his commit titles with [nix], so this is coming up in multiple places. Open to creating a rule for this but curious for other perspectives

In D3945#111222, @ashoat wrote:

In D3945#110907, @karol-bisztyga wrote:

Why you don't put [services] prefix in your revisions' names for the identity service? I thought we had a convention that we put a prefix depending on the "module" we work on. So, for instance, [web], [server], [native]. [services] Is at the same level, [identity] is deeper one level. Is there a particular reason you do not do this here? Has this rule changed?

I have this pattern but I've actually never pushed it as a rule. The other day @jonringer-comm asked me if he should prefix his commit titles with [nix], so this is coming up in multiple places. Open to creating a rule for this but curious for other perspectives

I see. For me, personally, it would be handier because I set my herald rule to detect any revisions with [services] in their names. If you decide to user other naming then fine but I think it will be better for everybody to use one, consistent pattern.

As a side note, maybe we could use tags?

That is a really good point about Herald rules. @benschac was asking on ENG-1055 about whether we should come up with a new Herald rule, since we're no longer recommending subscribing to all diffs... this idea makes sense to me.

i'll use [services][identity] going forward. are herald rules case sensitive?

services/identity/src/token.rs
9–12	We use this struct to hold data returned from DynamoDB, so we need to handle incomplete data. I think token should actually be wrapped in an `Option` too... will fix

Rename Token to AccessToken to be more precise, make token field optional (to handle incomplete data in DynamoDB)

Harbormaster completed remote builds in B9037: Diff 12580.May 12 2022, 8:35 AM

Accepting to preempt somebody adding me due to dependency change

back to you

services/identity/src/token.rs
9–12	Sorry, but I don't really understand. This seems to be inconsistent with https://phabricator.ashoat.com/D3573?vs=on&id=11131: where's `userID`? why do you use just `auth_type`? I don't see `pakePasswordCiphertext`/`walletAddress`. What's the primary key here?

This revision now requires changes to proceed.May 16 2022, 1:00 AM

varun planned changes to this revision.May 19 2022, 11:21 AM

varun added inline comments.

services/identity/src/token.rs
9–12	I was passing userID and deviceID to functions as separate args in addition to the Token struct, but perhaps that's confusing. I can include all of it in the Token struct. https://phabricator.ashoat.com/D3573?vs=on&id=11902#toc in a later revision than the one you linked I realized we didn't actually need to store that stuff in the token table primary key is the userID (partition key) + deviceID (sort key)