1071541 - grunt build uses the development values in local.conf.js (we can accidentally deploy local dev prefs to prod)

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P2)

Description

[Ed Morley [:emorley]]

I was under the impression local.conf.js was for local-only prefs used for development/testing.

However I just ran grunt build, with the intention of checking the result into the repo - but the minified JS included the custom line that exists in my local.conf.js:

window.thServiceDomain = "http://treeherder-dev.allizom.org";

Now I guess there may be times when we _do_ want to customise the grunt build output, if we're trying to reproduce something locally, that only occurs after minification. However this should perhaps be behind a grunt build param/pref, with the default case producing production-suitable output.

Comment 1

[Ed Morley [:emorley]]

Using local.conf.js also means that even if people comment out custom lines in their local files, it's likely that the checksum of the minified JS will change, causing unnecessary churn on grunt build commits.

Comment 2

[Ed Morley [:emorley]]

I've re-run grunt build using the sample config in the repo, but there are still differences from the minified output committed to the repo. Looks like whomever ran grunt build last had logging enabled, since this line is present, but not when I run with the sample config.:

treeherder.config(["$logProvider",function($logProvider){$logProvider.debugEnabled(!0)}])

Comment 3

[Ed Morley [:emorley]]

Cameron, this seems like a massive footgun waiting to go off. I was going to say: should we just get the grunt build script to use sample.local.conf.js and not local.conf.js to avoid this issue? However, that still doesn't help with how we ensure dev's copy of local.conf.js stays in sync with the example file?

It seems like we should perhaps switch to using:
* main.conf.js (which is in version control and matches the current sample.local.conf.js)
* local.conf.js (which would be optional, and override main.conf.js for testing instead of replacing it).

That way on dev, local.conf.js would just not exist, and dev + the grunt build would only use main.conf.js.

What do you think?

Comment 4

[Ed Morley [:emorley]]

(In reply to Ed Morley [:edmorley] from comment #3)
> * local.conf.js (which would be optional, and override main.conf.js for
> testing instead of replacing it).

s/override/supplement by allowing overriding of specific variables/

Comment 5

[Cameron Dawson [:camd]]

Ed--  Yeah, I totally agree with comment 3.  Let's do that.  It is too easy (I have done it) to accidentally push bad conf settings to prod.  It is... unpleasant.  :)

Comment 6

[Cameron Dawson [:camd]]

Attachment: ui pr 339

Comment 7

[Cameron Dawson [:camd]]

I hit this today when pushing to stage and since it's so easy to fix, here we go...

Comment 8

[Ed Morley [:emorley]]

Comment on attachment 8549159 [details] [review]
ui pr 339

Thanks for sorting this :-)

It needn't hold up landing this (since it won't break anything) - but I presume this will mean we'll get 404s on prod now, since the file doesn't exist there? (For people looking in the webconsole)

Comment 9

[Treeherder GitHub Bugbot]

Commits pushed to master at https://github.com/mozilla/treeherder-ui

https://github.com/mozilla/treeherder-ui/commit/7923f5c82296c9b3d1cff83d63b63aeb764dab46
bug 1071541 - remove local.conf.js from the grunt build

Now that we default to the same host for the service, we don’t need a
local.conf.js file to work properly.  Removing this prevents the
accident of stage pointing to prod or vice-versa, etc.

https://github.com/mozilla/treeherder-ui/commit/ff220a5f950480744b6805460b20572123edead6
Merge pull request #339 from mozilla/grunt-wo-local-conf

bug 1071541 - remove local.conf.js from the grunt build

Comment 10

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/5191b8dc841acc4662fb71f05073888112c190a3
bug 1071541 - remove local.conf.js from the grunt build

Now that we default to the same host for the service, we don’t need a
local.conf.js file to work properly.  Removing this prevents the
accident of stage pointing to prod or vice-versa, etc.

https://github.com/mozilla/treeherder/commit/918dd852b35e633c9e42b6a30e30eaad538d7adb
Merge pull request #339 from mozilla/grunt-wo-local-conf

bug 1071541 - remove local.conf.js from the grunt build