1160561 - Cleanup treeherder's settings files and make them follow the 12-factor app methodology

Status: RESOLVED FIXED

(Tree Management :: Treeherder, defect, P3)

Description

[Mauro Doglio [:mdoglio]]

Our settings files are not very maintainable at the moment.
The way we handle environment-specific settings is both based on environment variables dectection and local settings override. Given that we are going to move to heroku soon-ish we should probably make it only based on environment variables.

Comment 1

[Ed Morley [:emorley]]

We currently have settings in:
https://github.com/mozilla/treeherder/blob/master/.travis.yml#L14-L25
https://github.com/mozilla/treeherder/blob/master/fig.yml#L14-L15
https://github.com/mozilla/treeherder/blob/master/docker/etc/profile.d/treeherder.sh
https://github.com/mozilla/treeherder/blob/master/puppet/files/treeherder/local.production.py
https://github.com/mozilla/treeherder/blob/master/puppet/files/treeherder/local.vagrant.py
https://github.com/mozilla/treeherder/blob/master/puppet/manifests/production.pp#L42-L53
https://github.com/mozilla/treeherder/blob/master/puppet/manifests/vagrant.pp#L44-L55
https://github.com/mozilla/treeherder/blob/master/treeherder/settings/base.py
https://github.com/mozilla/treeherder/blob/master/treeherder/settings/local.sample.py

I really really want to consolidate these.

Do we see everything customisable moving to ENV variables? Or do we still need treeherder/settings/local.py for some things? (eg the adjusting the logging output for local development? Though perhaps that should be keyed off of DEBUG and put in base.py or something - I'm a fan of sensible defaults rather than making the user do it all every time locally)

Comment 2

[Ed Morley [:emorley]]

Attachment: Part 1: Consolidate/clean up prefs

Comment 3

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/43da904de09832db6c4d116659378e8a0681e40f
Bug 1160561 - Docs: Remove out of date deployment advice

We're no longer using the in-repo puppet config in production, nor would I
recommend anyone do so again in the future. In addition, copying and pasting
default configs into the docs isn't great, since they are a pain to maintain.
Let's just remove this from the docs.

https://github.com/mozilla/treeherder/commit/b3b76ff346008ad6fd06213d82429985c510a2ed
Bug 1160561 - Remove TEST_DB_PREFIX pref

Since we really don't need it.

https://github.com/mozilla/treeherder/commit/313e9dbbe45e8a6cef55c407d918c1d19cb4087e
Bug 1160561 - Remove prefs from local.sample.py that are identical to base.py

We're trying to move away from specifying prefs in files.

https://github.com/mozilla/treeherder/commit/dec412e12d44a3c5949a543c31807da634a909ad
Bug 1160561 - Consolidate DB prefs

Sets the default values to something sensible, to reduce the number of
prefs we have to set in the environment everywhere.

https://github.com/mozilla/treeherder/commit/b10646e74e055e2176ac6aa6dca92df86eeb7ee3
Bug 1160561 - Consolidate memcached prefs

https://github.com/mozilla/treeherder/commit/82de9c16fd6a8d14346c5dfc2fcfd7c6ae051231
Bug 1160561 - Consolidate rabbitmq prefs

Sets the default values (and now also those used by Vagrant) to the same
as those used by Travis, so we can avoid specifying different values all
over the place.

Comment 4

[Ed Morley [:emorley]]

This is slightly lower priority now that (a) the part 1 PR has already landed, and (b) Heroku is already up and running due to the PR in bug 1145606.

Comment 5

[Ed Morley [:emorley]]

I might see if https://github.com/rconradharris/envparse could clean up some of the boilerplate.

Comment 7

[Ed Morley [:emorley]]

Attachment: Part 2: Use correct env variable for PULSE_URI

Comment 8

[Ed Morley [:emorley]]

So it turns out https://github.com/rconradharris/envparse has some pretty basic bugs (eg https://github.com/rconradharris/envparse/issues/1) and very few users, so I think we'd be better off using https://github.com/joke2k/django-environ , which is a superset of a number of related packages - however is still pretty lightweight - and the path() stuff it can do looks pretty handy too. It also supports "?foo=bar" for adding options to the DB URLs, which is something that dj-database-url hasn't added even though there's been a PR open for months (https://github.com/kennethreitz/dj-database-url/pull/52).

Comment 9

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/d164ff5fcbd3ecf4d2dc8e6fc88da81ba5ea1691
Bug 1160561 - Use correct env variable for PULSE_URI

PULSE_USERNAME and PULSE_PASSWORD are not actually defined anywhere,
including in production. Instead we're relying on production's
local.py to set the value, rather than prod's PULSE_URI, which _is_ set.
By switching to use PULSE_URI, we're one step closer to not needing
local.py on prod.

Comment 10

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/f6a673a493ed0e4214d52e366910257374760c16
Bug 1160561 - Define IS_HEROKU in the dyno profile rather than Heroku env

`IS_HEROKU` isn't something that will differ between stage/prod, and is
not going to change any time soon. So let's just get post_compile to
add it to the dyno profile script, so it's one less variable to have to
remember to set via the Heroku CLI/dashboard.

Quoting from:
http://blog.doismellburning.co.uk/2014/10/06/twelve-factor-config-misunderstandings-and-advice/

"12factor says your applications should read their config from the
environment; it has very little to say about how you populate the
environment ? use whatever works for you".

Comment 11

[Ed Morley [:emorley]]

(In reply to Treeherder Bugbot from comment #10)
> https://github.com/mozilla/treeherder/commit/
> f6a673a493ed0e4214d52e366910257374760c16
> Bug 1160561 - Define IS_HEROKU in the dyno profile rather than Heroku env

I've removed IS_HEROKU from the vars defined in the web UI/CLI, now that it's set in the Dyno profile script, and confirmed it still appears in the output from |heroku run env|.

Comment 12

[Ed Morley [:emorley]]

Attachment: Part 3: Switch more settings to django-environ & other cleanup

Need to green up travis first (by w0orking around https://github.com/joke2k/django-environ/issues/56)

Comment 13

[Cameron Dawson [:camd]]

Comment on attachment 8689109 [details] [review]
Part 3: Switch more settings to django-environ & other cleanup

This all looks great!  Nice cleanup!

Comment 14

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/e8444645d997cafc58a57d48f1b4db8f84282979
Bug 1160561 - Use django-environ rather than os.environ.get

Since it's cleaner & simplifies casting of non-string values. For usage
guide, see:
https://github.com/joke2k/django-environ/blob/develop/README.rst

https://github.com/mozilla/treeherder/commit/5fc4a1a8b9eaa72335ef97240aced1dd5e4b5fbd
Bug 1160561 - Remove unnecessary DEBUG define in local settings file

Now that we're using django-environ's env.bool() in settings.py, the
value of TREEHERDER_DEBUG set in the Vagrant environment is correctly
picked up, so the local settings file DEBUG define is unnecessary.

https://github.com/mozilla/treeherder/commit/04dbd2b98b0e4a6308c20f3057c163168ef347e9
Bug 1160561 - Handle the Django DB password being missing or None

Whilst Django itself handles the password property being set to `None`,
we use the Django DB configs in several places outside of Django (either
directly with MySQLdb, or via datasource which uses MySQLdb too).

MySQLdb will handle the password being the empty string, but raises
if `None` is passed, which can occur if using django-environ, due to:
https://github.com/joke2k/django-environ/issues/56

This change both works around that issue, and is also likely the right
thing to do regardless, since we shouldn't assume that the password is
even set in settings.py at all. (Django defaults the password to the
empty string, so it's perfectly acceptable to omit the password
property in the DATABASES dict entirely.)

https://github.com/mozilla/treeherder/commit/d1b1cb534c896e53763f3da65da55c084bc11b01
Bug 1160561 - Replace dj-database-url with django-environ

We're already using django-environ for standard environment variables,
however it also supports DB URL parsing too, so we might as well use
it for everything, rather than using two packages.

In addition, django-environ already supports DB URI options (eg for
specifying the SSL cert), whereas the dj-database-url PR for that
feature has sat unmerged since May.

Compare:
https://github.com/kennethreitz/dj-database-url/blob/v0.3.0/dj_database_url.py
...with:
https://github.com/joke2k/django-environ/blob/v0.4/environ/environ.py#L323-L392

https://github.com/mozilla/treeherder/commit/cd353f5e3c2d70b2dca46c22db430382f39171c1
Bug 1160561 - Add support for setting BROKER_URL via the environment

The individual RABBITMQ_* variables are never used on their own, so it
makes more sense to switch to a URL variable that combines all of them.

Travis/Vagrant have also had BROKER URL explicitly set, since we're
generally moving away from having testing defaults set in settings.py.

Once stage/prod have BROKER_URL set, we can remove the fallback to the
old variable names, and also remove defaults entirely, making missing
settings fail fast.

Comment 15

[GitHub Autolander Bot]

Attachment: [treeherder] mozilla:rm-unused-envs > mozilla:master

Comment 16

[Treeherder GitHub Bugbot]

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

https://github.com/mozilla/treeherder/commit/574eb684e131768a8fcd6b13df2945a35d42fe76
Bug 1160561 - Remove legacy RABBITMQ_* environment variable support

They've been replaced by BROKER_URL, which is set for stage, prod,
Heroku, Vagrant (after a recent provision) and Travis.

https://github.com/mozilla/treeherder/commit/5151ffa0b3350f35acbc060f352505a5f2790527
Bug 1160561 - Clean up Heroku database SSL options

The previous comment was incorrect, django-environ doesn't support
setting the ssl options yet:
https://github.com/joke2k/django-environ/issues/72

https://github.com/mozilla/treeherder/commit/7ff307a9a24a64bd8b05826455d68db2d865d4a1
Bug 1160561 - Stop setting BROKER_URL to CLOUDAMQP_URL on Heroku

Since `BROKER_URL` on Heroku is now set to `$CLOUDAMQP_URL` and so
django-environ's proxied environment variables feature will fetch the
value from `CLOUDAMQP_URL` for us.

eg:
[~/src/treeherder]$ heroku run python
...
>>> import os
>>> import environ
>>> os.environ['BROKER_URL']
'$CLOUDAMQP_URL'
>>> env = environ.Env()
>>> env('BROKER_URL')
'amqp://REDACTED:[e-mail]/REDACTED'