add django-dotenv, and compensate for the removal of drama-free-django runtime features #5133

rosskarchner · 2019-07-23T17:07:28Z

These changes compensate for the removal of the "runtime" customizations in Drama Free Django, proposed here. This means that cfgov-refresh needs to do a little more work:

dfd will no longer populate environment variables from a JSON file. To deal with that, this PR adds django-dotenv and directly supports the old 'environment.json' files, so that they continue to work while we make the transition.
dfd will no longer add 'static.in' subdirectories to the STATICFILES_DIRS list, so that functionality has been added to cfgov.settings.base, triggered by a 'DJANGO_STATICFILES_IN' environment variable. You can see that in action in docker/drama-free-django/_build.sh
the scripts in docker/drama-free-django/ have been adapted to these changes.

Additions

django-dotenv
DJANGO_STATICFILES_IN functionality in cfgov.settings.base

Testing

docker/drama-free-django/build.sh && docker/drama-free-django/test.sh should work as expected

Todos

Merge Remove runtime customizations drama-free-django#25
docker/drama-free-django/_build.sh is currently set to install drama-free-django's magic-removal-branch. That should be fixed before merging.
The ansible side of this effort is not quite done yet.

Checklist

Testing checklist

Browsers

Accessibility

Keyboard friendly
Screen reader friendly

Other

hkeeler

Overall looks good. Just a few questions/thoughts on environment_candidates.

hkeeler · 2019-07-23T18:18:44Z

cfgov/cfgov/wsgi.py

+
+
+def initialize_environment():
+    environment_candidates = [


Why do we need these 3 possible options? Seems like a chance to have a .env in more than one place, and mistake which one is actually being used. Seems worth of a comment and/or some docs describing when to use these different locations.

Agree, it'd be nice to standardize on one location for .env. Can we standardize on using the repo root (../.env), which would also work out to the DFD root during deployments?

That makes sense to me.

it now only looks for '.env' as a sibling of 'cfgov'

hkeeler · 2019-07-23T18:21:55Z

cfgov/cfgov/wsgi.py

+    ]
+
+    for candidate in environment_candidates:
+        if os.path.exists(candidate):


What about when none of the candidates are found? Is that OK? In a Docker world, I'm guessing you wouldn't use a .env file at all, and would just pass in envvars right to the container?

Perhaps a log statement saying which .env file is being used, or none at all?

now, instead of searching for candidates, it tries to read ../../env. If it isn't there, the dotenv library will issue a warning

hkeeler · 2019-07-23T18:23:03Z

cfgov/manage.py

+this_dir = os.path.dirname(os.path.abspath(__file__))
+
+
+def initialize_environment():


Same questions as in wsgi.py.

addressed above

hkeeler · 2019-07-23T18:28:06Z

docker/drama-free-django/_test.sh

@@ -58,7 +60,8 @@ grep -Eho \
    | xargs touch

 # Now that we're sure that we have webfont files, we can test collectstatic.
-./venv/bin/django-admin collectstatic
+#./venv/bin/django-admin collectstatic


Probably don't need this commented out code here.

docker/drama-free-django/_build.sh

chosak · 2019-07-23T18:46:28Z

cfgov/cfgov/settings/base.py

@@ -264,6 +265,11 @@
    PROJECT_ROOT.child('templates', 'wagtailadmin')
 ]

+if 'DJANGO_STATICFILES_IN' in os.environ:
+    pattern = os.path.join(
+        os.environ['DJANGO_STATICFILES_IN'],


What's the argument over making this configurable vs. just hardcoding use of our current static.in as the one place we always place these files? We already document this location, for example here.

right now, I don't think the cfgov-refresh version of static.in gets included in the build artifact?

I guess we could make it relative, though-- it will always be a sibling to the cfgov directory.

Is there any reason why not to modify the Jenkins build to use static.in? In my branch modifying the Jenkins job to use the Docker-based build (docker-dfd-build), I use that and it works well. We don't use it in the non-Docker-based build currently.

I don't see a reason not to do that-- I think they just came about at different times, and for different reasons, and nobody ever connected the dots.

chosak · 2019-07-23T18:48:47Z

cfgov/cfgov/wsgi.py

+
+
+def initialize_environment():
+    environment_candidates = [


Agree, it'd be nice to standardize on one location for .env. Can we standardize on using the repo root (../.env), which would also work out to the DFD root during deployments?

chosak · 2019-07-23T18:49:40Z

cfgov/cfgov/wsgi.py

+
+    for candidate in environment_candidates:
+        if os.path.exists(candidate):
+                dotenv.read_dotenv(candidate)


Suggested change

dotenv.read_dotenv(candidate)

dotenv.read_dotenv(candidate)

I've removed that whole function

chosak · 2019-07-23T18:49:49Z

cfgov/cfgov/wsgi.py

+    for candidate in environment_candidates:
+        if os.path.exists(candidate):
+                dotenv.read_dotenv(candidate)
+                return


Suggested change

return

return

chosak · 2019-07-24T13:09:25Z

I note your comment:

The ansible side of this effort is not quite done yet.

Would it make sense to push back loading of .env and instead alter this PR to load from environment.json, essentially pulling in this code from DFD?

This would accomplish "stop using DFD at runtime" and allow us to proceed more incrementally, working as-is with the current Ansible code that generates environment.json files. In a subsequent PR (or multiple PRs) we could then further alter cfgov-refresh and the Ansible code to instead generate .env files.

rosskarchner · 2019-07-25T13:20:32Z

This would accomplish "stop using DFD at runtime" and allow us to proceed more incrementally, working as-is with the current Ansible code that generates environment.json files. In a subsequent PR (or multiple PRs) we could then further alter cfgov-refresh and the Ansible code to instead generate .env files.

That's an interesting idea-- I've been pondering a different approach to breaking the dependencies (having ansible deploy both the json and the .env)-- but since this exists now and the ansible changes don't yet, your approach makes more sense.

rosskarchner · 2019-07-25T14:22:31Z

@chosak I just pushed an implementation of that

chosak · 2019-07-25T14:42:01Z

@rosskarchner, thanks, I'll take another look at this PR.

One thing: don't we need to make at least some Ansible changes before this can go live, e.g. setting the DJANGO_STATIC_ROOT environment variable to the right location?

rosskarchner · 2019-07-25T15:35:30Z

@chosak what do you think about changing this default, which I don't think we actually use anywhere (outside of Docker experiments, maybe) https://github.com/cfpb/cfgov-refresh/blob/master/cfgov/cfgov/settings/base.py#L244

chosak · 2019-07-25T15:41:42Z

@rosskarchner that seems sensible to me, you mean, to /srv/cfgov/static? That seems like it'd be good to add to this PR.

rosskarchner · 2019-07-25T15:52:39Z

Yeah, that's what I meant

chosak

I notice that this logic in the sample .env file causes these warning messages to appear whenever you run a management command:

$ cfgov/manage.py shell
/Users/chosaka/.virtualenvs/cfgov-refresh/lib/python2.7/site-packages/dotenv.py:115: SyntaxWarning: Line 'if [ $(uname -s) = "Darwin" ]; then' doesn't match format
  SyntaxWarning
/Users/chosaka/.virtualenvs/cfgov-refresh/lib/python2.7/site-packages/dotenv.py:115: SyntaxWarning: Line '  export DATABASE_URL=postgres://cfpb@localhost/cfgov' doesn't match format
  SyntaxWarning
/Users/chosaka/.virtualenvs/cfgov-refresh/lib/python2.7/site-packages/dotenv.py:115: SyntaxWarning: Line '  export ES_HOST=localhost' doesn't match format
  SyntaxWarning
/Users/chosaka/.virtualenvs/cfgov-refresh/lib/python2.7/site-packages/dotenv.py:115: SyntaxWarning: Line 'fi' doesn't match format
  SyntaxWarning

This is only an annoyance but it sure feels like it could be a real annoyance for people running management commands constantly. Is there anything we can do about this?

chosak · 2019-07-25T16:01:44Z

cfgov/cfgov/settings/base.py

@@ -264,6 +265,8 @@
    PROJECT_ROOT.child('templates', 'wagtailadmin')
 ]

+static_in = REPOSITORY_ROOT.child('static.in')
+STATICFILES_DIRS += [str(d) for d in static_in.listdir(filter=DIRS)]


If you add this logic into the base settings, can you also remove this duplicative line from local settings?

cfgov/cfgov/settings/base.py

chosak · 2019-07-25T16:04:32Z

cfgov/cfgov/wsgi.py

+environmentdotjson_path = os.path.join(this_dir, '../../environment.json')
+
+if os.path.exists(envfile_path):
+    dotenv.read_dotenv(envfile_path)


Do we want to use override=True here (see docs)? That would make the .env file override any existing variables already in the environment. I think we do want that behavior, and that's how environment.json works now.

I'd say I'm mildly uncomfortable with that-- when I read about it, it seemed like an elegant way to recognize that if a variable has been set through some other mechanism, it reflects somebody's intent.

That said, I don't feel too strongly. I'll go ahead and make the change.

I think I found the reason not to use override=True: if .env takes precedence, then we can't set environment variables in docker-compose.

I think I found the reason not to use override=True: if .env takes precedence, then we can't set environment variables in docker-compose.

docker-compose itself has an env_file attribute, and it supports the same KEY=value format as django-dotenv. So, maybe the better option for compose, is to not load the .env file in-container, and let compose inject those envvars itself?

We'll have to do something similar in a production environment. We don't want to bake in env-specific .env files at image build time, and we won't be able to map in a .env at runtime, so the app will have to support just having those envvars already there at container start. I think that's fine in the current logic, since it doesn't fail if neither .env nor environment.json are present.

Also, is the plan to update .env_SAMPLE to the KEY=value style syntax? Or do we need it to be an actual shell script for other purposes?

I've removed override=True, so variables set via Docker, Linux, Compose, or tox will always take precedence.

chosak · 2019-07-25T16:06:08Z

cfgov/manage.py

+    envfile_path = os.path.join(this_dir, '../.env')
+    environmentdotjson_path = os.path.join(this_dir, '../environment.json')
+    if os.path.exists(envfile_path):
+        dotenv.read_dotenv(envfile_path)


See above comment re override=True.

chosak · 2019-07-25T16:06:57Z

docker/drama-free-django/_test.sh

@@ -45,6 +45,8 @@ export DJANGO_SETTINGS_MODULE=cfgov.settings.production
 # commands do nothing.
 mkdir -p static.in/0/fonts

+export DJANGO_STATICFILES_IN=/tmp/current/static.in


Is this a legacy addition that can now be removed since we're using local static.in?

chosak · 2019-07-25T16:36:29Z

One other thought about this PR: if you were to remove these changes in _build.sh, and these changes in _test.sh, could this PR be merged as-is and have things continue to work using the existing master version of drama-free-django?

Essentially, the variables would get loaded again, with no change. That'd let us move ahead with this change even while the DFD changes are still under review.

Another thing: the docs [currently suggest using autoenv to load the variables, here and here. Can we change that if we want to move to auto-loading?

rosskarchner · 2019-07-25T16:39:04Z

This is only an annoyance but it sure feels like it could be a real annoyance for people running management commands constantly. Is there anything we can do about this?

Do you know what that's for?

We could:

find a way to eliminate that logic -- django-dotenv doesn't interpret bash files, it expects every line to just be a variable.
preprocess the .env file, reading it into memory, stripping out the invalid lines, and then using parse_dotenv instead of read_dotenv.

chosak · 2019-07-25T16:45:06Z

Do you know what that's for?

Yes. It's because when running using a local virtualenv, the database host and Elasticsearch host are both localhost, but in the existing docker-compose setup, these are postgres and elasticsearch, respectively. Perhaps there's some better way of eliminating that logic, like defaulting them to localhost but letting docker-compose override them?

wpears · 2019-07-25T16:47:58Z

Those commands (specifically activate-virtualenv.sh) could just be moved into a separate file that gets invoked. .env was just being overloaded and I agree that it's probably better to keep it as a pure envvar file.

I agree with andy that defaulting them to localhost is probably the right path

rosskarchner · 2019-07-25T17:00:14Z

One other thought about this PR: if you were to remove these changes in _build.sh, and these changes in _test.sh, could this PR be merged as-is and have things continue to work using the existing master version of drama-free-django?

Essentially, the variables would get loaded again, with no change. That'd let us move ahead with this change even while the DFD changes are still under review.

One wrinkle: these changes don't help with /venv/bin/django-admin collectstatic

That's kind of the reason my initial inclination was a seperate DFD pull request, just for adding the manage.py wrapper (that PR would need to be updated to use the runpy.run_path approach)

Another thing: the docs [currently suggest using autoenv to load the variables, here and here. Can we change that if we want to move to auto-loading?

I think so?

chosak · 2019-07-25T17:07:32Z

One wrinkle: these changes don't help with /venv/bin/django-admin collectstatic

Hm. Well, frankly, the original thought behind adding the collectstatic call to _test.sh was because I wanted to test collectstatic on Travis (#5110). But because of some bugginess of that approach, and a hesitancy to add it as a potential blocker to all cfgov-refresh PRs, that got rolled back (#5119). So that code isn't being invoked by anything right now.

So if it would make things simpler to just remove that call to collectstatic, maybe just for now, we could just do that here. I'll also take a look at your open DFD PR as well, of course.

rosskarchner · 2019-07-29T16:52:09Z

While chipping away at getting this working under mod_wsgi, I discovered that variables set via Apache's SetEnv were not available to the wsgi.py script. After tinkering with some work-arounds, I decide that a simpler solution was to just add a new, production WSGI file, that unambiguously sets DJANGO_SETTINGS_MODULE to cfgov.settings.production. Deployment will need to be updated to use cfgov/cfgov/production_wsgi.py. We are already working on changing that, since DFD will no longer include the current /current/wsgi.py

use django-dotenv to load environment variables

Co-Authored-By: Andy Chosak <[email protected]>

Also, remove some redundencies.

rosskarchner added the work in progress label Jul 23, 2019

rosskarchner requested review from chosak, wpears and hkeeler July 23, 2019 17:07

rosskarchner mentioned this pull request Jul 23, 2019

Remove runtime customizations cfpb/drama-free-django#25

Closed

1 task

hkeeler reviewed Jul 23, 2019

View reviewed changes

chosak reviewed Jul 23, 2019

View reviewed changes

chosak reviewed Jul 25, 2019

View reviewed changes

chosak mentioned this pull request Jul 26, 2019

Move root-required bits of drama-free-django build into Dockerfile #5145

Merged

11 tasks

rosskarchner added 7 commits July 30, 2019 13:26

use manage.py in docker/drama-free-django/test.sh

c2a7fec

first step to removing DFD runtime dependencies:

0bac8bb

use django-dotenv to load environment variables

adapt to DFD's magic-removal-branch

e0d7e63

restore dotenv to manage.py

e059b34

address PR feedback

b742ce0

remove commented line

6dfc1cd

implement DFD's json-reading behavior, if .env is not found

f9a697b

rosskarchner and others added 12 commits July 30, 2019 13:40

a more useful default STATIC_ROOT

3dcfcdd

make isort happy

32ce5ff

use map instead of list comprehension

ee15ba4

Co-Authored-By: Andy Chosak <[email protected]>

use override=True when calling dotenv.read_env

a05951d

Also, remove some redundencies.

move environment loading to settings, remove logic from .env_SAMPLE

be33dde

Try a gentler approach to magic removal

dc1e285

decided to just keep using dfd's magic-removal-branch

3230f3b

remove commented line

7aa3ac7

add a production_wsgi.py

6c40c7b

local is not production

bbec250

use DFD master

3ed7f0b

don't use override=True

1122b4b

rosskarchner force-pushed the magic-removal-branch branch from b8d11fd to 1122b4b Compare July 30, 2019 17:41

rosskarchner and others added 2 commits August 1, 2019 09:21

default DJANGO_SETTINGS_MODULE is now production

f502a5c

Merge branch 'master' into magic-removal-branch

b1ea93e

anselmbradford mentioned this pull request Aug 2, 2019

Update to [email protected] | cf-tables: Migrate table vars #5137

Merged

rosskarchner closed this Aug 15, 2019

cwdavies deleted the magic-removal-branch branch April 7, 2020 17:25

		this_dir = os.path.dirname(os.path.abspath(__file__))


		def initialize_environment():

add django-dotenv, and compensate for the removal of drama-free-django runtime features #5133

add django-dotenv, and compensate for the removal of drama-free-django runtime features #5133

Conversation

rosskarchner commented Jul 23, 2019 • edited Loading

Additions

Testing

Todos

Checklist

Testing checklist

Browsers

Accessibility

Other

hkeeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chosak commented Jul 24, 2019

rosskarchner commented Jul 25, 2019

rosskarchner commented Jul 25, 2019

chosak commented Jul 25, 2019

rosskarchner commented Jul 25, 2019

chosak commented Jul 25, 2019

rosskarchner commented Jul 25, 2019

chosak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosskarchner Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chosak commented Jul 25, 2019

rosskarchner commented Jul 25, 2019

chosak commented Jul 25, 2019

wpears commented Jul 25, 2019 • edited Loading

rosskarchner commented Jul 25, 2019

chosak commented Jul 25, 2019

rosskarchner commented Jul 29, 2019

rosskarchner commented Jul 23, 2019 •

edited

Loading

rosskarchner Jul 30, 2019 •

edited

Loading

wpears commented Jul 25, 2019 •

edited

Loading