Page MenuHomePhabricator

[lib][web][native] Add configurable authoritativeKeyserverID
ClosedPublic

Authored by inka on Feb 8 2024, 7:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Apr 25, 8:09 AM
Unknown Object (File)
Mar 30 2024, 5:50 PM
Unknown Object (File)
Mar 1 2024, 12:12 PM
Unknown Object (File)
Feb 29 2024, 2:04 PM
Unknown Object (File)
Feb 28 2024, 4:45 AM
Unknown Object (File)
Feb 22 2024, 4:16 AM
Unknown Object (File)
Feb 22 2024, 1:05 AM
Unknown Object (File)
Feb 21 2024, 7:01 PM
Subscribers

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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!

Handle .json not being there

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

inka requested review of this revision.Feb 9 2024, 7:29 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
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)

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)

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.

native/authoritative-keyserver-id.js
8 ↗(On Diff #36928)

This will be done as part of creating docs that is the main point of my goal. I made a note about this in the task: ENG-6676

We want to use authoritativeKeyserverID from web on web

inka requested review of this revision.Feb 20 2024, 4:00 AM
tomek added inline comments.
native/authoritative-keyserver.js
17 ↗(On Diff #37344)

I'd say it's get - because we get a value and assign the result. This function doesn't set anything

native/config.js
20 ↗(On Diff #37344)
web/app.react.js
106 ↗(On Diff #37344)
This revision is now accepted and ready to land.Feb 21 2024, 1:26 AM