Feature/js/setup dev environment #461

jisantuc · 2017-10-03T18:46:04Z

Overview

This PR takes steps toward having a working local instance of the application.

Its main actions are:

run the django server using gunicorn
remove unused dependencies
move files around as needed
move the django server into a container
move the database and redis into containers
upgrade a bunch of dependencies (django, redis, django-redis, postgres, postgis...)
use django tools that may not have existed in django 1.4 (like django.core.cache and the {% static ... %} interpolator)
upgrade django settings (include a CACHES section for the cache, e.g.)
modify url routing
kill a ton of trailing whitespace (but by no means all of it -- it's dead everywhere I visited I think but I probably forgot to do it in some places)

It's huge, and therefore pretty much impossible to test completely.

In the application's current state, you can hit the landing page and all of the links appear to work, but trying to do (basically) anything fails. Pretty much the only thing that works is that you can try to log in as a guest, and then the app will successfully tell you that you've failed to login (:fire:). This prevents exercising some of the application's features.

Notes

The "spread firefox support" button seems not to exist anywhere in mozilla anymore. I didn't remove it because I wasn't sure if we wanted to go hunting for a replacement.

There are a few things that I either haven't exercised or haven't fixed:

geoserver remains untested. I'll integrate the container into this work in when I get a PR up for it
gettext isn't working at all. That seems really bad! (Fix translations #459)
Something is generically wrong with basically all of the views, where they complain about No reverse lookup found or believe they have syntax errors in the templates or who knows what else. Digging through template inheritance seemed like a possible black hole (Dig through template inheritance to fix lookups #460)

Testing

docker-compose up
Test the cache connection:
- docker-compose exec django bash
- ./manage.py shell
- run this:

from django.core.cache import caches
cache = caches['default']
cache.get('foo')

you should get nothing instead of an error
Test the database connection
- from inside the django container, ./manage.py dbshell
- \d
- you should see a bunch of tables
- \d redistricting_district
- you should see something that looks like a reasonable data model
test the frontend
- navigate to localhost:8080
- the only error you should see in the console should be that the frontend doesn't know what gettext is

Closes (or at least convincingly starts on) #455
Closes #458

Needed to get around obscure Kombo errors when evaluating the settings file.

ddohler

docker-compose up threw errors for me that prevented me from moving further. Otherwise this looks fine (although I mainly stuck to reading the parts that looked hand-written and skimmed the rest of it).

ddohler · 2017-10-03T21:41:07Z

django/publicmapping/Dockerfile

+COPY requirements.txt /usr/src/app/
+RUN pip install --no-cache-dir \
+        numpy==$(grep "numpy" requirements.txt | cut -d= -f3) \
+        scipy==$(grep "scipy" requirements.txt | cut -d= -f3) \


On docker-compose up this is throwing an error:

Collecting numpy==1.6.2 Downloading numpy-1.6.2.tar.gz (2.6MB) Collecting scipy==0.11.0 Downloading scipy-0.11.0.tar.gz (6.8MB) Complete output from command python setup.py egg_info: Traceback (most recent call last): File "<string>", line 20, in <module> File "/tmp/pip-build-aIUO1B/scipy/setup.py", line 208, in <module> setup_package() File "/tmp/pip-build-aIUO1B/scipy/setup.py", line 145, in setup_package from numpy.distutils.core import setup ImportError: No module named numpy.distutils.core

Hm. I did not seem to run into this after running:

$ docker-compose build $ docker-compose up

I nuked every docker image on my machine and then rebuilt, and now it works. ¯_(ツ)_/¯

ddohler · 2017-10-03T21:44:49Z

django/publicmapping/publicmapping/settings.py

+# See https://docs.djangoproject.com/en/1.11/howto/deployment/checklist/
+
+# SECURITY WARNING: keep the secret key used in production secret!
+SECRET_KEY = '1%a60e98ywm#$gl4mcm%s#uxoppc3)t%w+an*gzmujo-v-i07^'


It might be good to change this or deliberately leave it out so that it doesn't slip through the cracks as we work on this further.

Yeah, I think that we can pull it from the environment such that it leaves this setting unset if we forget to populate it:

SECRET_KEY = os.getenv('DJANGO_SECRET_KEY')

ddohler · 2017-10-03T21:51:42Z

django/publicmapping/redistricting/urls.py

+
+urlpatterns = [
+    url(r'^plan/$', redistricting_views.viewplan, { 'planid': 0 }),
+    url(r'^plan/create/$', redistricting_views.createplan),


The removal of the name parameters here may be the source of the confusion with reverse() if we were relying on calling these views by name. In other words, this line may need to become url(r'^plan/create/$', redistricting_views.createplan, name='createplan').

I tried adding names to all of the url views including the login view with no change, though I may not have understood correctly what the names should have been

hectcastro · 2017-10-03T18:56:04Z

django/publicmapping/Dockerfile

@@ -0,0 +1,32 @@
+FROM python:2.7


This overlaps a good bit with what's in quay.io/azavea/django. I think that it would be good to use that as the base, or at least split that out as a separate task for the future since this PR is already pretty big.

See: https://github.com/azavea/docker-django/blob/master/Dockerfile-slim.template

hectcastro · 2017-10-04T12:50:00Z

django/publicmapping/publicmapping/settings.py

+# See https://docs.djangoproject.com/en/1.11/howto/deployment/checklist/
+
+# SECURITY WARNING: keep the secret key used in production secret!
+SECRET_KEY = '1%a60e98ywm#$gl4mcm%s#uxoppc3)t%w+an*gzmujo-v-i07^'


Yeah, I think that we can pull it from the environment such that it leaves this setting unset if we forget to populate it:

SECRET_KEY = os.getenv('DJANGO_SECRET_KEY')

hectcastro · 2017-10-04T12:55:29Z

docker-compose.yml

+      - POSTGRES_USER=district_builder
+      - POSTGRES_PASSWORD=district_builder
+    healthcheck:
+      test: ["CMD-SHELL", "pg_isready"]


We've seen instances of this failing when the service is up because of the missing -U argument to pg_isready. Error looks like the following in the logs:

FATAL: role "root" does not exist FATAL: role "root" does not exist FATAL: role "root" does not exist ...

I can confirm that I'm seeing this after doing docker-compose up.

I saw the error, but it didn't appear to affect anything -- I could still fire up a django shell and talk to the database so I thought of it as just an annoying thing. Fixing.

This doesn't appear to be resolved by including `"-U", "district_builder" in the healthcheck command, though it does show up every 15 seconds, matching the healthcheck interval

hectcastro · 2017-10-04T12:56:30Z

docker-compose.yml

+      retries: 3
+
+  redis:
+    image: redis:3.2.11-alpine


I think that we can shorten this to 3.2-alpine, although, would be great to get rid of Redis in the future if we can.

There's no need to use redis here I think, I just did because the project used redis previously. Do you mean it would be good to switch to memcached, or it would be good not to have to depend on a cache at all?

We're actually not using Redis as a cache, but rather as key/value storage for adjacency information, which is used in a single, and rarely used, calculator. If we can find an alternate efficient way to perform that calculation, then Redis can be eliminated as a dependency. At this stage, we can probably just ignore dealing with Redis completely if we just don't use that calculator in any testing.

My comment was mostly from the perspective of eliminating an architectural component. I'm cool with keeping it out for now based on Kenny's explanation.

hectcastro · 2017-10-04T13:09:36Z

This was a large set of changes, so possibly something to do as a follow-up task based on how many points were allocated, but I think that it would be good to get the STRTA setup going before other people start picking this up.

This commit is a good place to start from. It has way more that necessary, so I'd focus on only the Vagrant, Ansible, scripts parts that we're going to use in the immediate future.

ddohler · 2017-10-04T14:40:02Z

A couple more things that I noticed once I got the docker containers building:

It would be good to install ipython and django-extensions into the container so that we have access to ./manage.py shell_plus.
When I ran ./manage.py dbshell I got a warning about Postgresql client / server version mismatches: WARNING: psql major version 9.4, server major version 9.5. Some psql features might not work.

jisantuc · 2017-10-05T13:49:29Z

I think we talked about not doing the Vagrant + docker setup that we normally do to decrease friction with community contributors. Am I remembering that wrong?

jisantuc · 2017-10-05T15:13:13Z

The warning about client/server version mismatches from dbshell is resolved after switching the base image

kshepard · 2017-10-05T15:34:49Z

re: Docker only vs. Vagrant + Docker -- there was some discussion, but I don't think there was ever a definitive decision. We'll need to talk it through.

hectcastro · 2017-10-05T15:52:07Z

My vote would be to include the Vagrant based Docker setup with STRTA scripts for internal consistency. Others can choose to make use of that framework, or not. Even if they don't, I think it would be valuable to surface the steps we take to get a development environment going in code. I also think that including the scripts helps minimize the documentation burden (run setup vs. run migrations, load this data, build the containers, etc.).

Modern django makes a gitignore for you, but the application root gitignore didn't know about it, which is why there was so much in the original diff

jisantuc · 2017-10-05T20:08:28Z

Ok scripts are in a place where you can ./scripts/ your way through setup now, and there's a scripts/test that lints the scripts directory and does nothing else except hang out and wait for more interesting testing and a CI environment to run in

jisantuc · 2017-10-05T20:09:37Z

deployment/ansible/district-builder.yml

+    - { role: "district-builder.environment" }
+    - { role: "azavea.python-security" }
+    - { role: "district-builder.docker" }
+    - { role: "district-builder.aws-cli" }


My assumption is that we're going to include AWS integration in the repo, but if that's not the case and we're just going to have containers and handle deployment of those containers separately, this can be removed. Currently the aws cli isn't being used for anything so it's a no-cost deletion.

We can keep it out for now.

themightychris · 2017-10-05T21:41:05Z

Is STRTA some internal Azavea convention?

Is there any major work left before this considered just about merge ready? I'm eyeing a good point to take another stab at contributing a habitat plan. I think y'all will loooove what it does, it can render all the other debates moot.

jisantuc · 2017-10-05T21:45:05Z

It's not us, it's short for "scripts to rule them all": https://github.com/github/scripts-to-rule-them-all

hectcastro

A good start. Merging this should help unblock others to see about upgrading the other components.

hectcastro · 2017-10-05T22:02:56Z

deployment/ansible/district-builder.yml

+    - { role: "district-builder.environment" }
+    - { role: "azavea.python-security" }
+    - { role: "district-builder.docker" }
+    - { role: "district-builder.aws-cli" }


We can keep it out for now.

hectcastro · 2017-10-05T22:03:10Z

deployment/ansible/group_vars/all

@@ -0,0 +1,7 @@
+---
+
+aws_cli_version: "1.11.*"


Same with the removal of these two.

hectcastro · 2017-10-05T22:04:10Z

scripts/update

+#!/bin/bash
+set -e
+
+if [[ -n "${RF_DEBUG}" ]]; then


Stale variable name here.

hectcastro · 2017-10-05T22:04:48Z

scripts/test

+#!/bin/bash
+set -e
+
+if [[ -n "${RF_DEBUG}" ]]; then


Stale variable name here too.

hectcastro and others added 7 commits August 28, 2017 21:41

WIP

7798692

Upgrade Celery and django-celery

d9834d8

Needed to get around obscure Kombo errors when evaluating the settings file.

Add psycopg2 to requirements

138a8e4

Simplify OS package install process

6f3da82

Get django to the point where it wants to talk to the database

987ecd0

Fixup most staticfiles errors and remove more trailing spaces

ad7a3f7

Use django-redis and django caching for cache interaction

c8612bb

jisantuc requested review from hectcastro and ddohler October 3, 2017 18:46

ddohler suggested changes Oct 3, 2017

View reviewed changes

hectcastro reviewed Oct 4, 2017

View reviewed changes

jisantuc added 6 commits October 5, 2017 10:23

Use azavea docker-django image

4bd7bc2

Hide the secret key

e0b100a

Upgrade numpy and scipy because apparently we can

d28c4cd

Fix postgres healthcheck and more loosely specify redis

7d01d20

Include django-extensions for greater happiness

a794596

Include some STRTA

79ec1a0

jisantuc added 4 commits October 5, 2017 15:56

Include provisioning scripts, VM, and ansible roles

94c89d1

gitignore changes

1dee6b7

Modern django makes a gitignore for you, but the application root gitignore didn't know about it, which is why there was so much in the original diff

Update README

b94ec41

Include test script and clean up lint

1d7a10f

jisantuc commented Oct 5, 2017

View reviewed changes

hectcastro approved these changes Oct 5, 2017

View reviewed changes

Remove unused roles and update env variable names in STRTA

22fe021

ddohler approved these changes Oct 6, 2017

View reviewed changes

jisantuc merged commit 8e8f38b into develop Oct 6, 2017

jisantuc deleted the feature/js/setup-dev-environment branch October 6, 2017 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/js/setup dev environment #461

Feature/js/setup dev environment #461

jisantuc commented Oct 3, 2017

ddohler left a comment

ddohler Oct 3, 2017

hectcastro Oct 4, 2017

ddohler Oct 4, 2017

ddohler Oct 3, 2017

hectcastro Oct 4, 2017

ddohler Oct 3, 2017

jisantuc Oct 5, 2017

hectcastro Oct 3, 2017

hectcastro Oct 4, 2017

hectcastro Oct 4, 2017 •

edited

Loading

ddohler Oct 4, 2017

jisantuc Oct 5, 2017

jisantuc Oct 5, 2017

hectcastro Oct 4, 2017

jisantuc Oct 5, 2017

kshepard Oct 5, 2017

hectcastro Oct 5, 2017

hectcastro commented Oct 4, 2017

ddohler commented Oct 4, 2017

jisantuc commented Oct 5, 2017

jisantuc commented Oct 5, 2017

kshepard commented Oct 5, 2017

hectcastro commented Oct 5, 2017

jisantuc commented Oct 5, 2017

jisantuc Oct 5, 2017

hectcastro Oct 5, 2017

themightychris commented Oct 5, 2017

jisantuc commented Oct 5, 2017

hectcastro left a comment

hectcastro Oct 5, 2017

hectcastro Oct 5, 2017

hectcastro Oct 5, 2017

hectcastro Oct 5, 2017

Feature/js/setup dev environment #461

Feature/js/setup dev environment #461

Conversation

jisantuc commented Oct 3, 2017

Overview

Notes

Testing

ddohler 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

hectcastro Oct 4, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectcastro commented Oct 4, 2017

ddohler commented Oct 4, 2017

jisantuc commented Oct 5, 2017

jisantuc commented Oct 5, 2017

kshepard commented Oct 5, 2017

hectcastro commented Oct 5, 2017

jisantuc commented Oct 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

themightychris commented Oct 5, 2017

jisantuc commented Oct 5, 2017

hectcastro 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

hectcastro Oct 4, 2017 •

edited

Loading