Page MenuHomePhabricator

[Docs] Remove apache configuration
ClosedPublic

Authored by jon on Dec 8 2022, 11:45 AM.
Tags
None
Referenced Files
F3513829: D5848.diff
Sun, Dec 22, 2:24 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Unknown Object (File)
Fri, Nov 22, 5:33 AM
Subscribers

Details

Summary

Update documentation around Apache as it's no longer necessary
for local development.

Test Plan

Steps:

  • Launch simulator
  • Delete existing Comm app (Hold press on application, then select "Remove App", confirm with "Delete App")
  • Update commapp_url.json and landing_url.json as described in diff (or delete them, and run nix develop)
  • Restart yarn dev from native/, and yarn-dev from keyserver/
  • Run simulator again

Rendered docs: https://github.com/CommE2E/comm/blob/jonringer/squadcal-apache/docs/dev_environment.md#urls

Diff Detail

Repository
rCOMM Comm
Branch
jonringer/squadcal-apache (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Not requesting changes because I'm about to go out, but please address this feedback before landing

docs/dev_environment.md
459 ↗(On Diff #19261)

Please make this look like the rest. You should always, always try to follow existing language when editing docs. Putting a command directly in like this instead of an English sentence, and not even wrapping it in a code block or inline code element is clearly a huge departure from the style in the docs. When you are editing the docs, YOU need to think of yourself as the editor of a technical doc. Part of an Editor's job is to maintain consistent style. Please think of this going forward, and please integrate this feedback into your workflow so I don't need to act as the team's only Editor. We need everybody on the team to be thoughtful about this

More consistent formatting and prose

jon edited the test plan for this revision. (Show Details)
docs/dev_environment.md
459 ↗(On Diff #19261)

Yep, that was my bad.

Should have double checked diff before submitting.

varun added inline comments.
docs/dev_environment.md
435 ↗(On Diff #19289)

we're sure this shouldn't be "/comm/"?

This revision is now accepted and ready to land.Dec 12 2022, 12:25 PM

Fix typo

docs/dev_environment.md
435 ↗(On Diff #19289)

good catch

ashoat requested changes to this revision.Dec 13 2022, 9:56 AM
ashoat added inline comments.
docs/dev_environment.md
459

Capitalize Comm

473

This needs proxy: "apache"

This revision now requires changes to proceed.Dec 13 2022, 9:56 AM
docs/dev_environment.md
459–475

Actually, not even clear why we need this part. The use of squadcal_url.json on dev is only necessary for handling the situation where an existing dev has a "sticky" urlPrefix set in the Redux store of the Comm app on a simulator/emulator, right? Somebody running through the dev_environment.md doc on their first go shouldn't have this issue... they'll get the new port-3000 urlPrefix from D5846

Apply typo fix to correct diff

jon marked 3 inline comments as done.

Remove squadcal altogether for new developers

jon added inline comments.
docs/dev_environment.md
459–475

I updated the linear task associated with this work. https://linear.app/comm/issue/ENG-2459

We should very rarely have "N/A" as the Test Plan. Can you amend the Test Plan to mention testing that these configs of commapp_url.json and landing_url.json work?

ashoat requested changes to this revision.Dec 15 2022, 9:18 AM

Back to you for Test Plan. Doesn't have to be "installing everything from scratch", but should be more than "N/A"

This revision now requires changes to proceed.Dec 15 2022, 9:18 AM
jon edited the test plan for this revision. (Show Details)
jon edited the test plan for this revision. (Show Details)
jon edited the test plan for this revision. (Show Details)

We should very rarely have "N/A" as the Test Plan. Can you amend the Test Plan to mention testing that these configs of commapp_url.json and landing_url.json work?

Updated test plan with something which should work outside or inside of nix develop. The original revision was part of the migration stack, so the logic was validated elsewhere (got apache running on m1, validating workflow now).

Since this aimed at new developers which don't have an existing Comm app on their simulator, I think it's reasonable to assume a clean slate.

Thanks @jon, that looks great!!

This revision is now accepted and ready to land.Dec 15 2022, 9:54 AM
This revision was automatically updated to reflect the committed changes.