Closed Bug 1128979 Opened 9 years ago Closed 9 years ago

Make it possible for the loop-client/standalone server to be run against different servers for debugging purposes

Categories

(Hello (Loop) :: Client, defect, P2)

defect
Points:
1

Tracking

(firefox40 fixed)

RESOLVED FIXED
mozilla40
Iteration:
40.3 - 11 May
Tracking Status
firefox40 --- fixed
backlog tech-debt

People

(Reporter: standard8, Assigned: standard8)

Details

Attachments

(1 file)

To make life easier for developers how wish to adjust the standalone pages, but not run their own loop-server, we should make it easier to run against active servers (e.g. dev).

I did a quick test and one way we can achieve this is to modify the standalone's server.js file so that loop.config.serverUrl is set from an environment variable.

The developer would then start Firefox, get a url, change the url portion to point to localhost (e.g. https://loop-webapp-dev.stage.mozaws.net/12345678912 to http://localhost:3000/12345678912) and they'd then be able to connect as normal.

When we do this, we need to be careful how we set up the environment variables and not break the functional tests.
I'm wrapping a couple of things up in this patch. The first one is to allow the standalone server to be easily redirected at non-localhost servers. This actually brings the config in line with the separate config we have in the Makefile that should make things easier later on.

It also slightly improves the situation for contributors (who can now more easily use the standalone against one of the existing servers), though I agree its still not the most ideal.

I'm also updating to express 4.x though the good news is there's no changes required other than the version bump.

Lastly there's a new static route for "/test" - rpapa requested this for running tests in the github repo that he does as part of the deployment checks (he also wants make test hooked up, but that's another bug ;-) ).
Attachment #8599952 - Flags: review?(dmose)
Assignee: nobody → standard8
Iteration: --- → 40.3 - 11 May
Points: --- → 1
Comment on attachment 8599952 [details] [diff] [review]
Improve usability of loop-client test server - allow tests to be loaded when in the github context; also allow the full server url to be specified, not just the port. NPOTB DONTBUILD

Review of attachment 8599952 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me; r=dmose.  A few nits interleaved.

::: browser/components/loop/standalone/server.js
@@ +17,5 @@
>                       "https://input.allizom.org/api/v1/feedback";
>  var feedbackProductName = process.env.LOOP_FEEDBACK_PRODUCT_NAME || "Loop";
> +var loopServerUrl = process.env.LOOP_SERVER_URL || "http://localhost:5000";
> +
> +// Remove trailing spaces.

I can guess why this is nice, but it'd be good to add a comment making it explicit to future readers.

@@ +74,5 @@
>  // These two are based on the above, but handle call urls, that have a /c/ in them.
>  app.use('/content/c', express.static(__dirname + '/content'));
>  app.use('/content/c', express.static(__dirname + '/../content'));
>  
> +app.use('/test', express.static(__dirname + '/test'));

Adding a comment about what this is used by and whom to contact about it could be helpful to future maintainers.

@@ +75,5 @@
>  app.use('/content/c', express.static(__dirname + '/content'));
>  app.use('/content/c', express.static(__dirname + '/../content'));
>  
> +app.use('/test', express.static(__dirname + '/test'));
> +app.use('/test', express.static(__dirname + '/../test'));

Out of curiousity, why did this move around?
Attachment #8599952 - Flags: review?(dmose) → review+
(In reply to Dan Mosedale (:dmose) - needinfo? me for response from comment #2)
> @@ +75,5 @@
> >  app.use('/content/c', express.static(__dirname + '/content'));
> >  app.use('/content/c', express.static(__dirname + '/../content'));
> >  
> > +app.use('/test', express.static(__dirname + '/test'));
> > +app.use('/test', express.static(__dirname + '/../test'));
> 
> Out of curiousity, why did this move around?

From a readability it picks up the comments surrounding /content which apply to the /test rules as well.
Flags: qe-verify-
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/00be9007fc70
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.