Use keyserver migration logic to apply developoment defaults which
work without use of Apache.
Update native url logic to account for explicit port usage
Details
Details
First need to create an existing app, which uses the apache configuration
git checkout master nix develop # optional, if legacy dev env already applied # In one terminal cd keyserver yarn dev # In another terminal cd native yarn dev # Run follwoing command in another terminal yarn react-native run-ios --simulator='iPhone 14 Pro' --configuration=Debug # Should be able to do normal iOS Dev workflow: # No "DISCONNECTED" bar appears, able to create chat threads # Profile > Developer tools, should show http://localhost/comm
Next, we need apply url-utils changes:
arc patch D5846 # Keyserver should apply the migration, but still needs to restart for routing changes # restart `yarn dev` in `keyserver/` # App should update to new defaultURL without intervention, but to start it again yarn react-native run-ios --simulator='iPhone 14 Pro' --configuration=Debug # Should be able to do normal iOS Dev workflow: # No "DISCONNECTED" bar appears, able to create chat threads # Profile > Developer tools, should show http://localhost:3000/comm
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
How are you making sure that this doesn't mess up the config in our production environment? Can you update your Test Plan to include steps to address that?
The developer paths seem to fallback to hardcoded paths here: https://github.com/CommE2E/comm/blob/824a62323ca29e4fbff784ecdf87e822ac5ca1fb/native/utils/url-utils.js#L57-L63
I'll try launching the native app in release mode and post the "Profile > Developer Tools" output.
Comment Actions
Nevermind, release mode removes developer tools section from profile.
Instead I was able to submit a friends request when running it in release:
Comment Actions
Thanks for updating the Test Plan! Last step here is this one:
How are you making sure that this doesn't mess up the config in our production environment? Can you update your Test Plan to include steps to address that?
To be clear, I'm more concerned about the keyserver side of production (eg. that we're going to create facts/squalcal_url.json / facts/commapp_url.json / facts/landing.json in production)... not worried about the native side of production (where I agree the app will just used the hardcoded paths).
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123 ↗ | (On Diff #19259) | Typo here, worries me that this wasn't tested |
| 123–125 ↗ | (On Diff #19259) | As discussed, this should not be done in Docker environment, where we don't use JSON configs. This should be wrapped in a check of either whether the file exists or whether an environmental variable is set, see importJSON |
Comment Actions
Test plan should be modified to check two more things:
- localhost:3000/ (root), not localhost:3000/comm/
- Docker keyserver build. Your keyserver/.env should look something like this:
COMM_DATABASE_DATABASE=<database> COMM_DATABASE_USER=<db_user> COMM_DATABASE_PASSWORD=<password> COMM_JSONCONFIG_facts_landing_url='<landing_url>' COMM_JSONCONFIG_facts_commapp_url='<commapp_url>' COMM_JSONCONFIG_facts_squadcal_url='<squadcal_url>'
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123 ↗ | (On Diff #19259) | Also this needs proxy: "apache" |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123–125 ↗ | (On Diff #19259) | I tested to see if the keyserver was being executed in a docker environment, and omitted writing the files in that case. I still don't like the idea of coupling configuration management and migrations; never going to be a case where there is 100% alignment in intent and effect. |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123 ↗ | (On Diff #19729) | I think something is wrong here... the old Apache config redirected http://localhost/comm/ to http://localhost:3000/, which means that the basePath and baseRoutePath have to be different. The point of the squadcal_url.json file is to work with this specific Apache config, so you need to make sure it works there. I once again suspect your Test Plan needs to be expanded. When the basePath is wrong the app may still initially load, but because it doesn't know how to construct URLs correctly you'll start to see issues when navigatind around and playing with the app. Perhaps you didn't do far enough in doing that? |
| 123–125 ↗ | (On Diff #19729) | Why is this an async function if you're not awaiting anything? This function will resolve immediately, and let its three invocations of moveToNonApacheConfig resolve "in the background". This breaks the "transactionality" of the migration. Please await the three calls |
| 192–194 ↗ | (On Diff #19729) | I think we can skip this warning. There's nothing really to warn about in my opinion |
| 197 ↗ | (On Diff #19729) | Typo |
| 203 ↗ | (On Diff #19729) | Shorthand |
Comment Actions
Also I don't think both of these things are in the test plan yet:
"localhost:3000/ root" was something I jotted down quickly during a 1:1 after explaining to you that squdcal_url.json was supposed to handle that baseRoutePath, but I'm guessing you didn't fully grasp that in the 1:1, and the diff comment didn't have much detail. Either way, you should make sure to test that, as based on my previous comment I think your solution here won't work.
As for the Docker keyserver build, we need you to test that to make sure the production environment doesn't break. Just like the rest of the environments you test, you should make sure to play around with the app a little bit to make sure it works.
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123 ↗ | (On Diff #19729) | I was able to post in personal thread using this setup. Given, the debug iOS application will reload with the new endpoint while the application is still open. So it might be that applying the url-utils.js changes causes this to not be an issue. |
| 123–125 ↗ | (On Diff #19729) | Thanks! |
Comment Actions
Almost there!!
Heads-up, your screenshot failed to attach... see here: https://www.notion.so/commapp/Attaching-files-and-images-on-Phabricator-af8e2195fc91462689cc8de5c96a6759
Thanks for the context on the testing. Maybe the reason the basePath being / wasn't an issue is that native is the only place that has the "sticky" configuration, and maybe native's usage doesn't rely so much on keyserver being able to construct URLs. Either glad, glad it's fixed.
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123–125 ↗ | (On Diff #19745) | Interesting about the Promise.all issue... that's really unexpected. Can you share any more details about that? Your earlier solution had all three going at the same time, just like my Promise.all solution. So that can't be the problem. I'd love if you could try again and do some investigation... I'm struggling to imagine how that is possible, so I'm suspecting there maybe was a typo or something |
| 188 ↗ | (On Diff #19745) | Maybe call this fileHandler, writeFile.writeFile seems like messy variable naming |
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123–125 ↗ | (On Diff #19745) |
I think the issue was that i forgot to await writeJSONToFilecalls. The updating ${filePath} to ${JSON.stringify(data)} message was being posted multiple times. Earlier migrations also use the async + await paradigm. I don't have much of a preference, but let me try it again. |
| 188 ↗ | (On Diff #19745) | want me to change it in the other locations? (E.g. makeSureBaseRoutePathExists, and fixBaseRoutePathForLocalhost) |
Comment Actions
Heads-up, your screenshot failed to attach.
Keep forgetting that phabricator doesn't do this by default.
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 123–125 ↗ | (On Diff #19745) |
The type for migrations map is $ReadOnlyMap<number, () => Promise<void>>, so just await Promise.all() isn't satisfactory, need it to be a function which takes no arguments. E.g. async () => { await Promise.all(); } |
Comment Actions
Okay, based on my review of the code this looks good. The test plan looks thorough, too – hope we caught everything!
| keyserver/src/database/migration-config.js | ||
|---|---|---|
| 166 ↗ | (On Diff #19748) | Thanks for refactoring this!! |