All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED

Status

P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jfrench, Assigned: emorley)

Tracking

(Blocks: 1 bug)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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.
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.
(Reporter)

Comment 2

3 years ago
(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).
(Assignee)

Comment 3

3 years ago
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).
Component: Treeherder → Treeherder: Docs & Development
Priority: P4 → P2
Summary: Possibly provide --service arg so user doesn't have to edit/manage local.conf.js → Remove the need to create a local.conf.js when using web-server.js to test the UI
(Assignee)

Updated

3 years ago
Assignee: nobody → emorley
(Assignee)

Comment 4

3 years ago
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.
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...  :)
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Updated

3 years ago
Assignee: emorley → nobody
Priority: P2 → P3
(Assignee)

Comment 7

2 years ago
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 :-)
Depends on: 1274054
Summary: Remove the need to create a local.conf.js when using web-server.js to test the UI → Remove the need to create a local.conf.js with gunicorn/GitHub pages
(Assignee)

Comment 8

2 years ago
With neutrino/webpack this isn't going to be possible.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 9

2 years ago
Actually this is possible with neutrino/webpack.
Assignee: nobody → emorley
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Assignee)

Updated

2 years ago
Summary: Remove the need to create a local.conf.js with gunicorn/GitHub pages → Prevent a stale local.conf.js from causing confusion as to what API URL is being used by the UI
Created attachment 8862040 [details] [review]
[treeherder] mozilla:rm-api-url-footgun > mozilla:master
(Assignee)

Updated

2 years ago
Attachment #8862040 - Flags: review?(cdawson)
(Assignee)

Updated

2 years ago
Blocks: 1353014
Comment on attachment 8862040 [details] [review]
[treeherder] mozilla:rm-api-url-footgun > mozilla:master

This is really great!  A vision finally realized.  :)
Attachment #8862040 - Flags: review?(cdawson) → review+
(Reporter)

Comment 12

2 years ago
:emorley excellent stuff, nice one.

Comment 13

2 years ago
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.
(Assignee)

Updated

2 years ago
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.