Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug 1123908 - use env vars for django config #2684

Conversation

rhelmer
Copy link
Contributor

@rhelmer rhelmer commented Mar 21, 2015

@peterbe @AdrianGaudebert I just mechanically merged local.py into base.py and converted all settings to use python-decouple (which supports reading configuration from the environment.)

There are a few TODOs in here, and we should discuss whether we want to further merge user-configurable settings with the base.py defaults, but I wanted to do something that was easier to understand in diff form to start the discussion.

  • resolve TODOs
  • allow local.py overrides to work, while we're still in PHX

@rhelmer rhelmer changed the title [DO NOT MERGE] fix bug 1123908 - use env vars for django config [NEEDS REBASE] fix bug 1123908 - use env vars for django config Mar 23, 2015
@@ -376,12 +346,260 @@ def COMPRESS_JINJA2_GET_ENVIRONMENT():
# all the migrations.
SOUTH_TESTS_MIGRATE = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not all of these?

The point of having SOUTH_TESTS_MIGRATE (for example) here in settings is so you can override them if you need to.
Well, it's kinda nice to have them written all down in one place too I guess.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after irc conversation, I'm taking this point back.

@peterbe
Copy link
Contributor

peterbe commented Mar 23, 2015

One thing that's missing the .env thing. For local development you don't want to override every single thing with environment variables. Developers have reliable disk and they can use a .env file to (example) to organize theirs.

At the moment I honestly don't know the difference between .env and settings.ini. I guess it's simply a choice of preference.

Either way, this PR is great for production whereby you will never rely on a disk file for settings but if we land this we'll leave developers stranded.

Also, I'm uncertain still why you left out some settings. Human oversight or deliberate?

@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 23, 2015

One thing that's missing the .env thing. For local development you don't want to override every single thing with environment variables. Developers have reliable disk and they can use a .env file to (example) to organize theirs.

Hm, so the problem is that python-decouple will look for .env first, then fall back the looking in os.environ, which is what we don't want for production. Could we have a env-dist that people need to copy perhaps?

Also, I'm uncertain still why you left out some settings. Human oversight or deliberate?

I'm not sure how to override some (they are marked with TODOs), some I am not sure of the utility of overriding (ROOT_URLCONF? INSTALLED_APPS?)

Having to edit the base.py is totally doable, we just need to push a new release so I'd like to avoid it for things unlikely to change.

@peterbe do you think we should allow every single thing to be overridden? Not sure how to even approach something like VCS_MAPPINGS

COMPRESS_OFFLINE = config('COMPRESS_OFFLINE', True, cast=bool)

# Make this unique, and don't share it with anybody. It cannot be blank.
SECRET_KEY = config('SECRET_KEY', 'you must change this')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you use SECRET_KEY = config('SECRET_KEY') I think it'll raise a UndefinedValueError if it's not provided with a value.

Here's how sugardough handles it

@peterbe
Copy link
Contributor

peterbe commented Mar 23, 2015

And .env-dist or something would be good. By the way, here's how sugardough generates a .env when you create a project.

I don't understand why .env has higher priority than environment variables. That just feels strange but maybe the people who've thought it for longer know a good reason.
One potential benefit with .env is that if you generate one, the right settings will be there automatically for mod_wsgi, ./manage.py syncdb, ./manage.py shell, ./manage.py some-thing, etc.

And we will need to start django in other places than just uwsgi. E.g. syncdb, collectstatic and compress.
Could we, similarly, have a script that generates the .env in terraform or something (...that I don't understand). We'd expect this file to live as long as the EC2 instance lives.

We basically want production to be 100% environment variables and local developers to be 100% .env. We can just have a mailing list message or something to train existing devs to convert their aging settings/local.py to a .env but either way we'll need a sustainable message to help new developers.

@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 23, 2015

Could we, similarly, have a script that generates the .env in terraform or something (...that I don't understand). We'd expect this file to live as long as the EC2 instance lives.

We want to use environment variables in prod and no config files, it'll be running like this:

envconsul -prefix socorro/webapp uwsgi webapp-django.wsgi.socorro-crashstats

So whenever the key DEBUG changes in the prefix socorro/webapp in consul, envconsul will automatically restart uwsgi.

@rhelmer rhelmer changed the title [NEEDS REBASE] fix bug 1123908 - use env vars for django config [DO NOT MERGE] fix bug 1123908 - use env vars for django config Mar 23, 2015
@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 24, 2015

@peterbe you mentioned in IRC possibly having a .env for production to allow for syncdb, collectstatic, compress, and so on.

I wonder if it would be possible instead to inject configuration into the shell.. for instance I could see doing sudo su - socorro and ending up with a shell that had an activated socorro virtualenv, and also all configuration values already set in the environment. So you could simply cd to the right dir and ./manage.py would do the right thing.

We really want to avoid pushing any kind of state to the nodes other than the apps themselves.

@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 24, 2015

I wonder if it would be possible instead to inject configuration into the shell.. for instance I could see doing sudo su - socorro and ending up with a shell that had an activated socorro virtualenv, and also all configuration values already set in the environment. So you could simply cd to the right dir and ./manage.py would do the right thing.

To answer my own question - I don't think you can start an interactive shell with envconsul, but we could run it like this:

envconsul -prefix socorro/shell -once \
  /data/socorro/socorro-virtualenv/bin/python \
  /data/socorro/webapp-django/manage.py syncdb --noinput

We could easily have an alias or script that acts as an entry point for manage.py on the nodes.

@rhelmer rhelmer force-pushed the bug1123908-decouple-settings-for-django-app branch from 8aa5ae5 to ce26fd6 Compare March 25, 2015 16:29
@rhelmer rhelmer changed the title [DO NOT MERGE] fix bug 1123908 - use env vars for django config fix bug 1123908 - use env vars for django config Mar 25, 2015
@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 25, 2015

@peterbe actually I realized that we need to not break PHX (we need to keep using local.py there), so we need to have a compromise here in the short-term... I think we should allow local.py overrides to work for now, and make it a warning not fatal if this does not exist.

This gives us some time to move the docs and dev config over, without blocking us using environment variables for production in AWS.

What do you think?

raise exc
except ImportError:
print ('WARNING - no local.py'
' (deprecated, use environment variables instead)')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this to just after from .local import *? I.e. it's deprecated to use local.py.
Also, instead of print can you change to:

import warnings
warnings.warn(
    "Use environment variables instead or a .env file", 
    DeprecationWarning
)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peterbe sorry I don't understand:

Can you move this to just after from .local import *

Isn't it already just after it?

Also - I changed from print to warnings.warn but I don't see it printed when I do e.g. ./manage.py runserver, is that because logging is disabled by default?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean; if it succeeds to import local.py you're potentially stuck in the old way of doing things. And you need to re-organize yourself away from local.py to a new .env file instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean; if it succeeds to import local.py you're potentially stuck in the old way of doing things. And you need to re-organize yourself away from local.py to a new .env file instead.

Oh! I see, so warn if there is a local.py, not if there is not. Sure, makes sense.

@peterbe peterbe self-assigned this Mar 25, 2015
@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 26, 2015

@peterbe ah the deprecation warning problem is https://code.djangoproject.com/ticket/18985 and they include an example in their docs (which is the default in manage.py from django 1.5 onward) to re-enable deprecation warnings.

@rhelmer rhelmer force-pushed the bug1123908-decouple-settings-for-django-app branch from 2ae7db3 to 5d7a052 Compare March 26, 2015 16:37
@rhelmer
Copy link
Contributor Author

rhelmer commented Mar 26, 2015

@peterbe ok this is ready for review again!

),
'LOCATION': config(
'CACHE_LOCATiON',
'crashstats',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to default to 127.0.0.1:11211?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this supposed to default to 127.0.0.1:11211?

Hm well the default is LocMemCache right?

@peterbe
Copy link
Contributor

peterbe commented Apr 1, 2015

r+

The only nit is that I don't see any developer-hand-holding with regards to setting up that .env file.

@rhelmer
Copy link
Contributor Author

rhelmer commented Apr 6, 2015

The only nit is that I don't see any developer-hand-holding with regards to setting up that .env file.

@peterbe what hand-holding did you have in mind? Something in the developer section of the docs, or were you thinking of something else?

@peterbe
Copy link
Contributor

peterbe commented Apr 6, 2015

Yes, documentation is what I meant. Soft stuff. You're so ahead of me on terms of the beginner documentation so it's rubberstamp=r+ on it if you do anything.

@rhelmer rhelmer force-pushed the bug1123908-decouple-settings-for-django-app branch from 45a4d8a to 7e9861c Compare April 6, 2015 20:48
@rhelmer rhelmer force-pushed the bug1123908-decouple-settings-for-django-app branch from 7e9861c to e9bf32e Compare April 6, 2015 20:58
rhelmer added a commit that referenced this pull request Apr 8, 2015
…r-django-app

fix bug 1123908 - use env vars for django config
@rhelmer rhelmer merged commit b071836 into mozilla-services:master Apr 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants