1168117 - Prevent a stale local.conf.js from causing confusion as to what API URL is being used by the UI

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Jonathan French (:jfrench)]

Feel free to shoot down the idea if I've missed something :)

User Problem: Users can forget or not realize they have a local.conf.js set to an unintended value when running a local server. eg. they are unknowingly running 'vagrant' and wondering why they have no jobs, or are pointing to 'stage/production' and don't realize the jobs they are looking at are not from their own ingestion. Also, new users have to manually copy/create a local.conf.js during initial setup.

What if instead we do something like:

- add three additional minimal-content service config files to the repo
ui/js/config/local.conf.stage.js
ui/js/config/local.conf.production.js
ui/js/config/local.conf.vagrant.js

And in some way modify these below, or their child executables that run the server: 
./web-server
./manage.py runserver
./bin/run_gunicorn

..to be run with some sort of "--service", "-s" arg. The user then specifies the service they want in the shell when they launch the server:

./web-server.js -s production
./bin/run_gunicorn --service stage

...or some similar approach.

It seems it would almost eliminate the need to manage local.conf.js, they won't have to set it up/copy it on initial repo install, and will always know what they are running.

Maybe I am missing something though.

Comment 1

[William Lachance (:wlach)]

This makes a ton of sense to me, at least with respect to the nodejs server (I don't see why you'd ever want to use anything but the actual local server with gunicorn). I'd probably default for the nodejs server to hit production, that way no configuration at all would be necessary in the common case.

Comment 2

[Jonathan French (:jfrench)]

(In reply to William Lachance (:wlach) from comment #1)
> (I don't see why you'd ever want to use anything but the actual local server with gunicorn)

In a related and addmittedly odd scenario today, I'm using gunicorn but pointing to stage, because of an unknown issue with bug 1167419 constantly reloading tabs w/vagrant, and because UI navigation (Repo menu, Logo menu) generally behaves properly in gunicorn (bug 1060313).

Comment 3

[Ed Morley [:emorley]]

Yeah I think we should definitely do this for web-server.js. As an added bonus it would avoid the case I often hit: when switching between using web-server.js and the vagrant environment, I often forget to remove local.conf.js, meaning I'm not using the backend I think I am, when the vagrant environment is running.

For the gunicorn/runserver server, I think this would be much harder (and less frequently used).

Comment 4

[Ed Morley [:emorley]]

As part of fixing this, we could also try to engineer it such that the pushing to gh-pages technique does not need to create a loca.conf.js either.

Comment 5

[Cameron Dawson [:camd]]

I frequently run gunicorn and switch between local, stage or prod (depending on if I need to use local data or not).  And I would love this feature.  

I'm not sure the *best* way to do this, but perhaps one simple way would be for the ``bin/run_gunicorn`` script to check for that param, if present, then have it modify the local.conf.js file with the appropriate flavor of ``window.thServiceDomain = stage;``

Perhaps just deleting the last line, and appending the new one with the right value.
Just the first idea that came to mind.  But fantastic idea, Jon.  I'm frequently changing the value in that file...  :)

Comment 6

[Ed Morley [:emorley]]

So another confusing thing about this, is that for the requests that we *do* prefix with `thServiceDomain`, if I forget to rename/move the local.conf.js file (when it is pointing at prod) - when testing in vagrant I get:

POST https://treeherder.mozilla.org/browserid/login/ [HTTP/1.1 403 FORBIDDEN 465ms]

This really confused me for a while whilst testing bug 1160111.

Comment 7

[Ed Morley [:emorley]]

So this has been partly fixed by bug 1274054 - there just remains:
* making this work automatically when using the gh-pages approach
* supporting via gunicorn

To fix these would require logic client-side, similar to what TBPL did. Such logic may even make the approach in bug 1274054 redundant.

Whether it's worth doing is another matter :-)

Comment 8

[Ed Morley [:emorley]]

With neutrino/webpack this isn't going to be possible.

Comment 9

[Ed Morley [:emorley]]

Actually this is possible with neutrino/webpack.

Comment 10

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:rm-api-url-footgun > mozilla:master

Comment 11

[Cameron Dawson [:camd]]

Comment on attachment 8862040 [details] [review]
[treeherder] mozilla:rm-api-url-footgun > mozilla:master

This is really great!  A vision finally realized.  :)

Comment 12

[Jonathan French (:jfrench)]

:emorley excellent stuff, nice one.

Comment 13

[Treeherder GitHub Bugbot]

Commit pushed to master at https://github.com/mozilla/treeherder

https://github.com/mozilla/treeherder/commit/f422a027302e64aab5f0dd61721a8efead9509ce
Bug 1168117 - Remove support for defining the API URL via local.conf.js

Previously a forgotten-about `local.conf.js` (which is git-ignored)
would override the URL passed by the `SERVICE_URL` environment variable.

With webpack and environment variables, there is no need to use a local
config file to control the API URL, so we can now remove this footgun.