Updated settings and requirements to fix deployment issues #447

blarghmatey · 2018-02-23T20:20:50Z

What are the relevant tickets?

What's this PR do?

The file parsing available in uWSGI has a hardcoded line limit which was causing problems with managing the certificate strings via the env file. Added a block to read in the env file and set environment variables automatically as part of settings.py to work around that issue.

Removed the -e flags from the git dependencies so that they get installed to the dist-packages directory instead of the home directory of the user that runs the pip command. This was causing issues with being able to import those packages at runtime.

Added the option to set the celery broker separately from the results backend in order to allow for using RabbitMQ in place of Redis.

How should this be manually tested?

Ensure that the app will start properly with an env file formatted as ENV_VAR=value with no export prefix.

Where should the reviewer start?

odl_video/settings.py and requirements.txt

What GIF best describes this PR or how it makes you feel?

mbertrand · 2018-02-23T20:57:54Z

odl_video/settings.py

+    with open(f'{BASE_DIR}/.env') as envsettings:
+        for line in envsettings:
+            k, v = line.rstrip('\n').lstrip('export ').split('=', maxsplit=1)
+            os.environ.setdefault(k=v)


This throws an error: TypeError: setdefault() got an unexpected keyword argument 'k'

This works: os.environ.setdefault(k, v)

codecov-io · 2018-02-23T22:45:17Z

Codecov Report

Merging #447 into master will decrease coverage by 0.03%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master     #447      +/-   ##
==========================================
- Coverage   95.27%   95.24%   -0.04%     
==========================================
  Files         136      136              
  Lines        4633     4643      +10     
  Branches      191      192       +1     
==========================================
+ Hits         4414     4422       +8     
- Misses        179      181       +2     
  Partials       40       40

Impacted Files	Coverage Δ
odl_video/envs.py	`96.72% <75%> (-3.28%)`	⬇️
odl_video/settings.py	`82.82% <75%> (+0.21%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cadbcb3...3c5b5c0. Read the comment docs.

mbertrand · 2018-02-26T14:12:27Z

odl_video/settings.py

@@ -488,7 +497,7 @@
    },
    'redis': {
        "BACKEND": "django_redis.cache.RedisCache",
-        "LOCATION": CELERY_BROKER_URL,
+        "LOCATION": REDIS_URL,


CELERY_BROKER_URL will default to REDIS_URL if the env. variable isn't set, so maybe it might be best to not change this line, @noisecapella do you want to weigh in? That seems to be the pattern used in open-discussions & micromasters as well:

CELERY_BROKER_URL = get_string("CELERY_BROKER_URL", REDIS_URL)

It looks like Giovanni added that line in a commit to add mandatory settings, I don't think REDIS_URL has any special meaning to Celery or django-redis. I think the change here makes sense to keep the celery redis broker separate from the redis cache Django

@mbertrand if we leave it as is and the CELERY_BROKER_URL gets set to something other than a Redis protocol then that would break the cache config. That's why I broke them out to be separate settings.

OK, sounds good to me then

noisecapella · 2018-02-26T15:14:02Z

odl_video/settings.py

@@ -21,6 +21,14 @@

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

+try:


Can we move this into a function and write tests for it?

noisecapella · 2018-02-26T16:07:57Z

odl_video/settings.py

@@ -488,7 +497,7 @@
    },
    'redis': {
        "BACKEND": "django_redis.cache.RedisCache",
-        "LOCATION": CELERY_BROKER_URL,
+        "LOCATION": REDIS_URL,


It looks like Giovanni added that line in a commit to add mandatory settings, I don't think REDIS_URL has any special meaning to Celery or django-redis. I think the change here makes sense to keep the celery redis broker separate from the redis cache Django

noisecapella · 2018-02-26T20:36:48Z

odl_video/envs_test.py

+
+
+def test_parse_env():
+    testpath = 'testenv.txt'


Can you use a TemporaryFile here or put it into a temporary directory? If it's not done with a context manager or something we should also put the os.remove in a finally block

noisecapella · 2018-02-26T20:37:26Z

odl_video/envs_test.py

+def test_parse_env():
+    testpath = 'testenv.txt'
+    with open(testpath, 'w') as testfile:
+        testfile.write('FOO_VAR=bar_var\nFOO_NUM=42')


Can you change one of the lines to include an "export ", and have one of the values include its own = sign?

mbertrand · 2018-02-27T16:38:49Z

odl_video/envs.py

+    duplicating across multiple locations and not requiring explicit
+    sourcing.
+
+    :param env_file: path to a file consisting of key=value pairs


@blarghmatey The typical format looks like this, can you change it? Otherwise looks good!

The file parsing available in uWSGI has a hardcoded line limit which was causing problems with managing the certificate strings via the env file. Added a block to read in the env file and set environment variables automatically as part of settings.py to work around that issue. Removed the `-e` flags from the git dependencies so that they get installed to the `dist-packages` directory instead of the home directory of the user that runs the pip command. This was causing issues with being able to import those packages at runtime. Added the `no-binary` flag for psycopg2 due to issues with how the wheel is linked to OpenSSL for Debian 9 (unbit/uwsgi#1569) Added the option to set the celery broker separately from the results backend in order to allow for using RabbitMQ in place of Redis.

blarghmatey added Needs Review DevOps labels Feb 23, 2018

blarghmatey requested review from mbertrand and noisecapella February 23, 2018 20:20

mbertrand self-assigned this Feb 23, 2018

blarghmatey force-pushed the celery_broker_update branch 2 times, most recently from a04410a to c37d9ef Compare February 23, 2018 20:41

mbertrand requested changes Feb 23, 2018

View reviewed changes

blarghmatey force-pushed the celery_broker_update branch from c37d9ef to 841ea84 Compare February 23, 2018 22:45

mbertrand requested changes Feb 26, 2018

View reviewed changes

mbertrand added Waiting on Author and removed Needs Review labels Feb 26, 2018

noisecapella self-assigned this Feb 26, 2018

noisecapella suggested changes Feb 26, 2018

View reviewed changes

mbertrand approved these changes Feb 26, 2018

View reviewed changes

blarghmatey force-pushed the celery_broker_update branch 5 times, most recently from 648d89f to f55619d Compare February 26, 2018 20:35

noisecapella suggested changes Feb 26, 2018

View reviewed changes

blarghmatey force-pushed the celery_broker_update branch 5 times, most recently from e2c2359 to 1aa25c5 Compare February 27, 2018 15:29

mbertrand reviewed Feb 27, 2018

View reviewed changes

blarghmatey force-pushed the celery_broker_update branch from 1aa25c5 to 3c5b5c0 Compare February 27, 2018 16:47

blarghmatey merged commit c14046b into master Feb 27, 2018

blarghmatey deleted the celery_broker_update branch February 27, 2018 16:49

mbertrand mentioned this pull request Mar 14, 2018

Upgrade to Django 1.11 #465

Merged

mbertrand mentioned this pull request Apr 23, 2018

Collections Pagination #554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated settings and requirements to fix deployment issues #447

Updated settings and requirements to fix deployment issues #447

blarghmatey commented Feb 23, 2018

mbertrand Feb 23, 2018

codecov-io commented Feb 23, 2018 •

edited

Loading

mbertrand Feb 26, 2018

noisecapella Feb 26, 2018

blarghmatey Feb 26, 2018

mbertrand Feb 26, 2018

noisecapella Feb 26, 2018

noisecapella Feb 26, 2018

noisecapella Feb 26, 2018

noisecapella Feb 26, 2018

mbertrand Feb 27, 2018

		@@ -21,6 +21,14 @@

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

		try:

Updated settings and requirements to fix deployment issues #447

Updated settings and requirements to fix deployment issues #447

Conversation

blarghmatey commented Feb 23, 2018

What are the relevant tickets?

What's this PR do?

How should this be manually tested?

Where should the reviewer start?

What GIF best describes this PR or how it makes you feel?

Choose a reason for hiding this comment

codecov-io commented Feb 23, 2018 • edited Loading

Codecov Report

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

codecov-io commented Feb 23, 2018 •

edited

Loading