Improve setup docs and configs based on Jean's onboarding #687

jeancochrane · 2018-06-07T20:02:24Z

Overview

It took me two days to get DRIVER fully set up; most of that time was spent getting well enough acquainted with Vagrant/Ansible/VirtualBox/Docker/Angular so that I could debug routine errors in the toolchain, but some of it was genuine issues with the app setup that could be merged back in.

This PR attempts to pull out the useful changes I made during my onboarding and merge them back upstream.

Checklist

PR has a descriptive enough title to be useful in changelogs

Notes

For posterity, here are a few problems I ran into that turned out to require environment changes (i.e. changes to my setup and not the app itself):

My home directory lives in an encrypted drive on my Ubuntu 16.04 machine, and it seems that it's not possible to mount shared directories into a VM using NFS when the directories live on this kind of encrypted drive. I had to make sure that the DRIVER and ashlar repos lived outside of my encrypted drive in order to get the mount to work. The particular error I was getting was something along the lines of mount.nfs Error: /path/to/shared/folder is not a file or a directory.
Provisioning the app and celery VMs failed a few times due to routine config problems (e.g. I had a port collision on 5432 that I had to resolve by setting DRIVER_DATABASE_PORT_5432=5433). I wasn't aware that vagrant up and vagrant reload don't automatically rerun provisioning tasks; I spent a while trying to figure out why the loopback interface was denying my requests before I realized that large chunks of the app were missing because they still needed to be provisioned after I fixed the config errors. The solution to this is to run vagrant reload --provision or do as the docs say and manually retry vagrant provision <VM> when you're debugging provisioning problems.

Testing Instructions

Most of these changes are tiny, so I don't think they need real testing. If you want to verify my work, you could:

Load up the Angular apps and check to make sure that disabling HTML5 doesn't have downstream problems (shouldn't affect production anyway I don't think, since the edit is to the example config)
Check the styling of the schema editor forms, particularly the geography upload, to make sure that the buttons display appropriately
Try running a data import using a virtualenv, the scripts/requirements.txt file, and the updated docs in scripts/README.md

jeancochrane · 2018-06-07T20:04:48Z

deployment/ansible/group_vars/all.example

@@ -49,7 +49,7 @@ redis_bind_address: "redis.service.driver.internal"
 redis_port: 6379

 ## WEB SETTINGS
-js_html5mode: "true"
+js_html5mode: "false"


There was some discussion about this in the DRIVER Slack channel -- it seems like Tiago ran into a similar issue. I'm still not sure why it was a problem, but the app was failing to load with HTML5 enabled due to this issue: https://stackoverflow.com/questions/37034140/location-in-html5-mode-requires-a-base

Yeah, good call; this has tripped up several people recently; I don't know why it broke in the first place but our standard practice has become setting it to false, so it should be that way here too.

jeancochrane · 2018-06-07T20:05:16Z

schema_editor/app/scripts/views/geography/add-partial.html

@@ -56,7 +56,7 @@ <h2>Add New Geography</h2>
                </select>
            </div>
        </form>
-        <div class="save-area">
+        <div class="save-area col-sm-12">


Without assigned column widths, these buttons didn't appear for me in either Chrome or Firefox.

Haha, nice, they actually do show up but they're waaaay off to the right. Thanks for fixing this!

That's where they went! My screen must be too small to see them.

jeancochrane · 2018-06-07T20:06:06Z

scripts/README.md

@@ -1,13 +1,33 @@
-#Loading data for development
+# Loading data for development


I updated the data loading docs to provide a little more background on the requirements. Apologies for the ugly diff.

jeancochrane · 2018-06-07T20:07:37Z

scripts/requirements.txt

@@ -0,0 +1,3 @@
+python-dateutil
+pytz
+requests


Ideally these dependencies should be pinned, but I think it would make sense to wait to see if we want to put more effort into containerizing the test data ETL before doing that. My Python environment is substantially different such that I'm not confident the versions of these packages that I installed will generalize to all machines.

Hmm yeah, it might actually be best to hold off on this file altogether for the moment. If we add containerization it won't be necessary, and if this was what you needed to get things going for your own local machine, then it's likely to be incomplete because you probably had some stuff installed already. What might be good is to include these in the list of requirements you added to the README; then we can provide better context and explain that the listed requirements are not necessarily complete.

I like that solution -- much more clearly communicates the level of confidence I have in the completeness of the dependencies.

ddohler

Thanks for putting this together! This is looking pretty good, but it should be targeted against develop rather than master; we only merge to master when we're releasing a new version. I had a few other minor comments as well.

ddohler · 2018-06-08T11:45:45Z

scripts/README.md

+## Script and schema dependencies
+
+Certain ETL scripts and schemas in this repo depend on specific data
+sources to run properly. Retrieve these dependencies from the fileshare and


Outside users won't have access to our fileshare, so I don't think we should include this in the setup instructions. What might be nice is to add some of the files below to a sample_data directory so that they're in the repo. However, the incident data is from the Philippines and I think we should get authorization before including it. Perhaps we could just include these for now:

black_spots.json

interventions_sample_pts.geojson

That will at least allow users to generate a schema with black spots and interventions, and then perhaps manually generate some data using the web UI.

Sounds great! I'll create the sample_data directory. I think the dependency table is still helpful if you have access to the fileshare, since it's the only documentation that exists for matching scripts and schemas to the example data that they rely on, but I agree that it's weird to have open docs that reference a private data directory. Is it OK to leave the table here, or would it be better to move it to a README in the fileshare folder?

I think a README in the fileshare folder would be perfect, that's a great solution!

ddohler · 2018-06-08T11:51:21Z

scripts/requirements.txt

@@ -0,0 +1,3 @@
+python-dateutil
+pytz
+requests


Hmm yeah, it might actually be best to hold off on this file altogether for the moment. If we add containerization it won't be necessary, and if this was what you needed to get things going for your own local machine, then it's likely to be incomplete because you probably had some stuff installed already. What might be good is to include these in the list of requirements you added to the README; then we can provide better context and explain that the listed requirements are not necessarily complete.

ddohler · 2018-06-08T11:55:22Z

schema_editor/app/scripts/views/geography/add-partial.html

@@ -56,7 +56,7 @@ <h2>Add New Geography</h2>
                </select>
            </div>
        </form>
-        <div class="save-area">
+        <div class="save-area col-sm-12">


Haha, nice, they actually do show up but they're waaaay off to the right. Thanks for fixing this!

ddohler · 2018-06-08T11:56:48Z

deployment/ansible/group_vars/all.example

@@ -49,7 +49,7 @@ redis_bind_address: "redis.service.driver.internal"
 redis_port: 6379

 ## WEB SETTINGS
-js_html5mode: "true"
+js_html5mode: "false"


Yeah, good call; this has tripped up several people recently; I don't know why it broke in the first place but our standard practice has become setting it to false, so it should be that way here too.

…e data

jeancochrane added 4 commits June 7, 2018 11:12

Add col widths to schema editor form buttons

00871d7

Don't require HTML5 in config to avoid Angular error

87ff90b

Tighten up documentation

98618b4

Remove stray console log from the geography upload controller

f4683fe

jeancochrane commented Jun 7, 2018

View reviewed changes

jeancochrane changed the title ~~[ WIP ] Improve setup docs and configs based on Jean's onboarding~~ Improve setup docs and configs based on Jean's onboarding Jun 7, 2018

jeancochrane mentioned this pull request Jun 7, 2018

Review Ashlar / DRIVER changes to get DRIVER dev environment running azavea/grout-2018-fellowship#1

Closed

ddohler self-assigned this Jun 8, 2018

ddohler suggested changes Jun 8, 2018

View reviewed changes

jeancochrane changed the base branch from master to develop June 8, 2018 13:59

jeancochrane added 2 commits June 8, 2018 10:56

Add sample data and update docs for test data

3ee2f38

Clean up docs for loading sample data and remove references to privat…

6f360fc

…e data

ddohler merged commit bc4e417 into WorldBank-Transport:develop Jun 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve setup docs and configs based on Jean's onboarding #687

Improve setup docs and configs based on Jean's onboarding #687

jeancochrane commented Jun 7, 2018

jeancochrane Jun 7, 2018

ddohler Jun 8, 2018

jeancochrane Jun 7, 2018

ddohler Jun 8, 2018

jeancochrane Jun 8, 2018

jeancochrane Jun 7, 2018

jeancochrane Jun 7, 2018

ddohler Jun 8, 2018

jeancochrane Jun 8, 2018

ddohler left a comment

ddohler Jun 8, 2018

jeancochrane Jun 8, 2018

ddohler Jun 8, 2018

ddohler Jun 8, 2018

ddohler Jun 8, 2018

ddohler Jun 8, 2018

		@@ -1,13 +1,33 @@
		#Loading data for development
		# Loading data for development

Improve setup docs and configs based on Jean's onboarding #687

Improve setup docs and configs based on Jean's onboarding #687

Conversation

jeancochrane commented Jun 7, 2018

Overview

Checklist

Notes

Testing Instructions

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

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