Improve dev server room routing code clarity

RESOLVED FIXED

Status

P3
normal
Rank:
35
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dmose, Assigned: dmose)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
Created attachment 8705869 [details] [diff] [review]
Fix dev server regression to handle local server calls
(Assignee)

Comment 2

3 years ago
https://github.com/mozilla/loop/commit/4c69c769e3b007b7e5aa9960ca2342c4f3ea0aac#diff-8d44631bfd02d64ee75348ab92f466c0L106 regressed the ability to run a local loop-server pointing to a local dev server, which makes debugging some key standalone changes (eg CSP changes) impossible.
Assignee: nobody → dmose
Blocks: 1212428
Rank: 20
Priority: -- → P2
Created attachment 8705874 [details] [review]
[loop] dmose:local-server-content > mozilla:master
(Assignee)

Comment 4

3 years ago
Created attachment 8705875 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/62
Attachment #8705869 - Attachment is obsolete: true
Attachment #8705875 - Flags: review?(standard8)
Attachment #8705875 - Flags: review?(edilee)
Attachment #8705875 - Flags: review?(dcritchley)
Comment on attachment 8705875 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/62

If I understand this right, then I don't want to do this.

What I think the PR is allowing for is /content/<token> as well as /<token>.

When we set up the new standalone server I purposely decided to drop the /content part of the url. This was to make us closer to the real production system, and avoid a lot of the complicated hacks that we had in place.

Unfortunately I didn't do a very good job of remembering to communicate that.

Hence, I really don't think we should allow /content, and rather update the loop-server default configs and our example dev config to drop the /content from the urls (and inform all devs, which I'll send out on Monday).
Attachment #8705875 - Flags: review?(standard8)
Attachment #8705875 - Flags: review?(edilee)
Attachment #8705875 - Flags: review?(dcritchley)
Attachment #8705875 - Flags: review-
(Assignee)

Comment 6

3 years ago
Comment on attachment 8705875 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/62

Updated to not add extra routing, but keep clarity.
Attachment #8705875 - Flags: review- → review?(standard8)
(Assignee)

Updated

3 years ago
Summary: Fix dev server regression to handle local server calls → Improve dev server room routing code clarity
(Assignee)

Updated

3 years ago
Rank: 20 → 35
Priority: P2 → P3
Comment on attachment 8705875 [details] [review]
Link to Github pull-request: https://github.com/mozilla/loop/pull/62

Looks good.
Attachment #8705875 - Flags: review?(standard8) → review+
(Assignee)

Comment 8

3 years ago
https://github.com/mozilla/loop/commit/42ab05c213ffe56b6834b19428b837dadc309252
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.