Details

Reviewers

tomek
michal

Commits

rCOMM9c9b9d26a8e6: [lib][web][native] Add configurable authoritativeKeyserverID

Summary

issue: ENG-6634
We need the constant that is currently called ashoatKeyserverID to be configurable. We have two different ways of configuring things on web vs native. On native we can just import .json files. On we we cannot do that (this is complicated why, see ENG-1181 for more info), so we use a workaround. I followed D7371 for that

Test Plan

Created authoritativeKeyserverID.json

{
  "authoritativeKeyserverID": "256"
}

in keyserver/facts and native/facts. Added a console log on both web and native and checked that value from the config is logged (it was set to something else than ashoatKeyserverID)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Feb 8 2024, 7:20 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 8 2024, 7:20 AM

inka added inline comments.Feb 8 2024, 7:22 AM

web/webpack.config.cjs
16 ↗	(On Diff #36849)	I had to do this, because eslint doesn't allow relative imports from keyserver/ I checked that after this change my .json was fetched correctly
197–200 ↗	(On Diff #36849)	This reads `keyserver/facts/authoritativeKeyserverID.json`

Harbormaster returned this revision to the author for changes because remote builds failed.Feb 8 2024, 7:32 AM

Harbormaster failed remote builds in B26652: Diff 36849!

inka added a child revision: D11006: [lib] Replace ashoatKeyserverID with authoritativeKeyserverID.Feb 9 2024, 2:10 AM

Handle .json not being there

inka added inline comments.Feb 9 2024, 7:18 AM

native/authoritative-keyserver-id.js
3–12 ↗	(On Diff #36928)	Because `./facts/authoritativeKeyserverID.json` may not be present, we need to use `require` in `try/catch` instead of `import`. It's the same approach as in D6373 I was facing some really weird problems when I put this code in `config.js`, so this file is a workaround. See ENG-6725

Harbormaster completed remote builds in B26705: Diff 36928.Feb 9 2024, 7:29 AM

inka requested review of this revision.Feb 9 2024, 7:29 AM

tomek accepted this revision.Feb 12 2024, 1:02 AM

tomek added inline comments.

native/authoritative-keyserver-id.js
8 ↗	(On Diff #36928)	We should update the docs to include some info about creating this file. Also, I think that it would be better to have `authoritativeKeyserver.json` file which contains an ID. If we ever decide to store more info about this keyserver, it would be more convenient.
12 ↗	(On Diff #36928)	Do we really need to have `fromJSON` suffix? It seems to be an implementation detail. Is there a risk of confusion if we name it `authoritativeKeyserverID`?
web/webpack.config.cjs
16 ↗	(On Diff #36849)	Was the error there before your changes? I guess we should test if the new code works on prod.

This revision is now accepted and ready to land.Feb 12 2024, 1:02 AM

ashoat added inline comments.Feb 12 2024, 6:07 AM

native/authoritative-keyserver-id.js
8 ↗	(On Diff #36928)	This filename does not match conventions in our codebase. Please always try to match conventions
12 ↗	(On Diff #36928)	I agree, but if we keep `fromJSON`, we should fix the capitalization ("f" should be capitalized)

ashoat added inline comments.Feb 12 2024, 6:09 AM

native/authoritative-keyserver-id.js
8 ↗	(On Diff #36928)	We should update the docs to include some info about creating this file. Is this really necessary, given that the file will be automatically created? (Let me know if I'm missing something)

tomek added inline comments.Feb 12 2024, 6:14 AM

native/authoritative-keyserver-id.js
8 ↗	(On Diff #36928)	It might help with debugging some issues, or when someone would want to manually change the value (but in that case reading the code might be a better idea). Also, if someone wants to reset / clear the user, they would need to delete the file.