Make Docker great again #163

lipemorais · 2017-05-10T18:36:41Z

This is in progress, I just created it earlier to have earlier feedbacks.

What is the purpose of this Pull Request?
The purpose of this PR is make easier contribute and develop for Jarbas using docker-compose and docker.

What was done to achieve this purpose?
Initially I fixed elm dockerfile to make docker work again for development with Jarbas. I have below a list of things to be done to make it ideal.

How to test if it really works?
make run.devel

Who can help reviewing it?
@gomex @Irio @anaschwendler @jtemporal @cuducos

What is out of scope
Is out of scope make this docker-compose ready for production environment, I'm planing to work on it in a next PR.

TODO

Fixes static files in rest framework front end
Create a jarbas-frontend image in docker hub
Update Jarbas image in docker hub
Improve documentation for Docker images
Update docker-compose.yml to use new images

…Jarbas

coveralls · 2017-05-10T18:40:08Z

Changes Unknown when pulling ba55e58 on lipemorais:master into ** on datasciencebr:master**.

cuducos · 2017-05-10T19:42:25Z

I really liked that:

Is out of scope make this docker-compose ready for production environment

Not because I'm kind of against Docker on production, but because… baby steps and focus!

But there's also something I don't like: make run.devel. Why? It leaves Windows users out. It's OK to have some shortcuts set but the documentation and public discussions should be Windows friendly too.

But so far so good ; )

Some questions:

Do we have a kind of diagnostic of why Docker is not working properly in development?
Changing yarn installation is just an enhancement or is it relevant to the purpose? I do not want to discourage enhancements, just want to learn from your PR ; )

lipemorais · 2017-05-10T21:45:57Z

But there's also something I don't like: make run.devel. Why? It leaves Windows users out. It's OK to have some shortcuts set but the documentation and public discussions should be Windows friendly too.

I am not a Windows user for a long time. Do you have any tip how could be it Windows friendly?

Do we have a kind of diagnostic of why Docker is not working properly in development?
Changing yarn installation is just an enhancement or is it relevant to the purpose? I do not want to discourage enhancements, just want to learn from your PR ; )

Both questions are related. There were a chain of problems in Dockerfile-elm. To isolate the problem I built Dockerfile-elm alone using docker build -f Dockerfile-elm -t jelm .

The first one was use sudo as in RUN curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add - there is no /usr/bin/sudo in the image node:6.9.1. So every place that had sudo was necessary remove it. To solve this I removed every sudo in this file. The error is below:

➜  jarbas git:(a2b2fe2) ✗ docker build -f Dockerfile-elm -t jelm .
Sending build context to Docker daemon 181.3 MB
Step 1/13 : FROM node:6.9.1
 ---> 00673888c33c
Step 2/13 : RUN curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add -
 ---> Running in 956480ca67dc
/bin/sh: 1: sudo: not found
curl: (23) Failed writing body (768 != 1369)
The command '/bin/sh -c curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | sudo apt-key add -' returned a non-zero code: 127

The following problem was that running apt-get update was not able to finding /usr/lib/apt/methods/https. Investigating a little more I found a solution that was like

// Dockerfile-elm
FROM node:6.9.1

RUN apt-get update
RUN apt-get install -y apt-transport-https
RUN curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | apt-key add -
RUN echo "deb https://dl.yarnpkg.com/debian/ stable main" | tee /etc/apt/sources.list.d/yarn.list
RUN apt-get install -y yarn
...

what I think that is not elegant, so I investigate a little more and found a second solution to install yarn using npm instead of apt-get that looks good enough, so I used it.

This is the error that I found:

➜  jarbas git:(a2b2fe2) ✗ docker build -f Dockerfile-elm -t jelm .
Sending build context to Docker daemon 181.3 MB
Step 1/13 : FROM node:6.9.1
 ---> 00673888c33c
Step 2/13 : RUN curl -sS https://dl.yarnpkg.com/debian/pubkey.gpg | apt-key add -
 ---> Using cache
 ---> ccc588b913d0
Step 3/13 : RUN echo "deb https://dl.yarnpkg.com/debian/ stable main" | tee /etc/apt/sources.list.d/yarn.list
 ---> Using cache
 ---> ccc30a8feaa4
Step 4/13 : RUN apt-get update
 ---> Running in 72d991e6c27e
E: The method driver /usr/lib/apt/methods/https could not be found.
The command '/bin/sh -c apt-get update' returned a non-zero code: 100

But this doesn't make yarn available in the PATH so I was using yarn with the complete path /node_modules/yarn/bin/yarn.js everywhere. To solve this a created a symbolic link RUN ln -s /node_modules/yarn/bin/yarn.js /usr/bin/yarn.

After this tree step I achieved this PR.

cuducos · 2017-05-10T22:41:39Z

Wow… thank you so much for the explanation! So far so good ; )

cuducos · 2017-05-10T23:06:04Z

I am not a Windows user for a long time. Do you have any tip how could be it Windows friendly?

Me neither (since 2002 BTW) but I wrapping commands in a Makefile because they would only work in Linux and macOS. In sum make devel.run is just a shortcut for these commands that end up working in Windows too:

docker-compose up -d --build
docker-compose run --rm jarbas python manage.py ceapdatasets
docker-compose run --rm jarbas python manage.py migrate
docker-compose run --rm jarbas python manage.py collectstatic --no-input

We can keep the Makefile, but when offering instructions it's good to be more open ; )

lipemorais · 2017-05-11T20:02:53Z

I will take a look at this others PRs and see how I can use them to improve this one.

Does it worked for anyone? @cuducos @anaschwendler @jtemporal @Irio ?

cuducos · 2017-05-12T01:38:42Z

Does it worked for anyone?

I haven't tested it throughly (I mean, clean staticfiles/, edit Elm files etc.) but it looks like it works ; )

coveralls · 2017-05-12T03:29:07Z

Changes Unknown when pulling 15ab50e on lipemorais:master into ** on datasciencebr:master**.

lipemorais · 2017-05-12T03:45:20Z

I added one more change reusing the shared volume between the containers assets to make rest_framework assets available through NGINX. Fixing the first point of the TODO list.

If more explanation is needed to understand which was my approach to solve this issue please, just let me know.

…ocker image builds

coveralls · 2017-05-12T18:31:36Z

Changes Unknown when pulling 63529dd on lipemorais:master into ** on datasciencebr:master**.

…omated docker image builds" This reverts commit 63529dd.

coveralls · 2017-05-12T18:58:23Z

Changes Unknown when pulling ed7a711 on lipemorais:master into ** on datasciencebr:master**.

… of docker-compose will set the right configurarions

coveralls · 2017-05-12T19:49:34Z

Changes Unknown when pulling 40a939a on lipemorais:master into ** on datasciencebr:master**.

…more since environment variables are now given through docker-compose or sytem environment variables

coveralls · 2017-05-12T19:57:19Z

Changes Unknown when pulling 3b73256 on lipemorais:master into ** on datasciencebr:master**.

…g this elm container

…ompose is ran

coveralls · 2017-05-14T05:40:55Z

Changes Unknown when pulling fd09172 on lipemorais:master into ** on datasciencebr:master**.

lipemorais · 2017-05-14T15:45:44Z

I made a test to solve the TODO list points 2 and 3, about use images and not build it every time. To do this I made 3 docker images for Jarbas, Frontend and NGINX that were the images constructed after run docker-compose.

If you want to take a look:
https://hub.docker.com/r/felipedemorais/auto-jarbas/
https://hub.docker.com/r/felipedemorais/auto-jarbas-frontend/
https://hub.docker.com/r/felipedemorais/aut-jarbas-nginx/

The next step is have this 3 images in Data Science Brigade Docker Hub: https://hub.docker.com/u/datasciencebr/

A second point in this PR is move .env from jarbas dockerfile to docker-compose so the environment variables can be passed through docker-compose make the application adapt itself at each environments(development, production, ...).

cuducos · 2017-05-14T19:23:36Z

I'm getting ImportError: No module named 'whitenoise' — probably because the master branch when you forked was using WhiteNoise for static files. I reverted it and it fixed the issue — it's working! Can you merge ae6a32c into your branch?

lipemorais · 2017-05-14T20:20:26Z

I'm getting ImportError: No module named 'whitenoise' — probably because the master branch when you forked was using WhiteNoise for static files.

But I do not have any lead of whitenoise in my fork.
There is any way that I could reproduce this error?

I reverted it and it fixed the issue — it's working! Can you merge ae6a32c into your branch?

I can merge it, of course. How can I apply this changes?

cuducos · 2017-05-14T20:23:53Z

Wow… so I might have messed up the branches here. Never mind. Do you want access to our DockerHub or would you like to help me setting this up?

lipemorais · 2017-05-14T20:48:55Z

If prefer the second option of empower you, so more than people than me hold this knowledge. Sounds good to you @cuducos ?

cuducos · 2017-05-14T20:51:38Z

Hell yeah 🤘

cuducos

Added some inline comments. Hope they're helpful. Besides them se might delete staticfiles volume (lines 67-68) that seems unused…

cuducos · 2017-05-14T22:06:54Z

Dockerfile

@@ -3,7 +3,6 @@ COPY requirements.txt /requirements.txt
 RUN python -m pip install -U pip
 RUN python -m pip install -r requirements.txt
 RUN apt-get update && apt-get install -y postgresql postgresql-contrib


In theory we were installing PostgreSQL because pyscopg2 (PostgreSQL driver for Python) needs one or other library that comes with PostgreSQL. If this apt-get is coming after pip this might not be necessary anymore.

TLDR IMHO we can get rid of line 5

cuducos · 2017-05-14T22:11:17Z

docker-compose.yml

       - "./:/code"
+       - ".env:/code/.env"
    environment:
       - SECRET_KEY=lets-rock


It's extremely unsafe to use this for production. As the focus is development environment this is not a problem right now… but I decided to let this note here so we can (later) take care of that and of GOOGLE_STREET_VIEW_API_KEY ; )

anaschwendler · 2017-05-15T07:45:15Z

Hi @lipemorais !
Thanks for the contribuition :)
I'm only taking a look to this PR by now, but I'm not the best person to talk about Docker but @cuducos is updating us with everything and I will try to run this later and give you feedback :)

cuducos · 2017-05-16T13:42:59Z

Makefile

@@ -1,5 +1,5 @@
 run.jarbas:
-	docker-compose up -d --build
+	docker-compose up -d


We must reflect this change in README.md before merging

Fixed: 8b73b79

coveralls · 2017-05-16T14:11:00Z

Changes Unknown when pulling 8b73b79 on lipemorais:master into ** on datasciencebr:master**.

cuducos · 2017-05-16T14:13:35Z

I tested it more today. It works beautifully for development — hell yeah, many many thanks, @lipemorais!

Before merging we would like to see some bugs and issues squashed:

Google Street Views are not showing because I couldn't find a way to insert my API key (sensitive data): can we find a way to do it and mention it on the README.md
Say I just have a freshly reimbursements.xz that Rosie brewed this morning: how can I pass this file (originally saved in my local/host machine) to be processed by Django in the container (something like docker-compose run --rm jarbas python manage.py reimbursements -d /tmp/serenata-data/reimbursements.xz in which /tmp/serenata-data/refers to local/host file system)? This should be contemplated in the README.md too
Update images URLs to point to our images at Docker Hub

Is this feasible?

lipemorais · 2017-05-16T14:20:50Z

It's completely feasible.
I just did the third point right here: f60ea8e

I will work on the next points soon.

coveralls · 2017-05-16T14:25:43Z

Changes Unknown when pulling f60ea8e on lipemorais:master into ** on datasciencebr:master**.

…add the instructions in README

coveralls · 2017-05-17T13:12:33Z

Changes Unknown when pulling 1cdab6d on lipemorais:master into ** on datasciencebr:master**.

lipemorais · 2017-05-17T13:17:30Z

Theoretically is just export GOOGLE_STREET_VIEW_API_KEY=your-google-api-key, so docker compose will get it from envvar with the same name.
This is not a automatic way but in order to make it be explicit and safe I see this as the easiest way to handle it give that this is a sensitive data.

As I do not have a Api Key I'm just base in docker-compose documentation in: https://docs.docker.com/compose/environment-variables/#passing-environment-variables-through-to-containers

@cuducos, when you have break could you try it?

…e database

coveralls · 2017-05-17T14:51:13Z

Changes Unknown when pulling f2a6f31 on lipemorais:master into ** on datasciencebr:master**.

lipemorais · 2017-05-17T14:53:02Z

About the second point it's not a big change since the file is inside the project folder, everything works very similar. I added some a instruction in README.md just to make it clearer and more explicit.

I also exposed the port 5432 of db container to make possible connect in the database using a postgres client.

coveralls · 2017-05-17T14:54:35Z

Changes Unknown when pulling f2a6f31 on lipemorais:master into ** on datasciencebr:master**.

cuducos · 2017-05-17T19:37:27Z

Sweet! Many many thanks for all that, @lipemorais!

gomex · 2017-05-18T05:20:08Z

I loved the PR! Thanks @lipemorais

fixes elm docker file to make docker work again for development with …

ba55e58

…Jarbas

cuducos added enhancement infra labels May 10, 2017

cuducos added the work in progress label May 10, 2017

This was referenced May 10, 2017

Setup a new way to compose containers #137

Closed

Introducing docker prod environment #129

Closed

cuducos mentioned this pull request May 11, 2017

Painful development with docker #98

Closed

makes rest_framework statics available trough nginx

15ab50e

removes .env from .gitignore, so jarbas will able to have automated d…

63529dd

…ocker image builds

Revert "removes .env from .gitignore, so jarbas will able to have aut…

ed7a711

…omated docker image builds" This reverts commit 63529dd.

moves environment variables from .env to docker-compose so the runner…

40a939a

… of docker-compose will set the right configurarions

remove the copy of .env in Dockerfile because it is not necessary any…

3b73256

…more since environment variables are now given through docker-compose or sytem environment variables

Felipe Morais added 2 commits May 14, 2017 02:26

uses best practice to handle CMD in Docker to do not surprises runnin…

246e87f

…g this elm container

uses images from docker hub instead of build every time that docker-c…

fd09172

…ompose is ran

cuducos reviewed May 14, 2017

View reviewed changes

lipemorais mentioned this pull request May 16, 2017

Automate deploy with Docker #53

Closed

cuducos reviewed May 16, 2017

View reviewed changes

fixes command to run docker-compose on README

8b73b79

change beta images for oficials from datasciencebr Docker Hub

f60ea8e

makes GOOGLE_STREET_VIEW_API_KEY work easily with docker compose and …

1cdab6d

…add the instructions in README

Felipe Morais added 2 commits May 17, 2017 11:42

exposes the database port to make it easy to connect

6d01828

adds instructions to seed database with a new reimbursements.xz in th…

f2a6f31

…e database

cuducos merged commit c224085 into okfn-brasil:master May 17, 2017

cuducos mentioned this pull request May 17, 2017

Update docs (after Docker) #168

Merged

cuducos changed the title ~~[WIP] Make Docker great again~~ Make Docker great again May 17, 2017

cuducos removed the work in progress label May 17, 2017

Make Docker great again #163

Make Docker great again #163

Conversation

lipemorais commented May 10, 2017 • edited by cuducos Loading

coveralls commented May 10, 2017

cuducos commented May 10, 2017

lipemorais commented May 10, 2017

cuducos commented May 10, 2017

cuducos commented May 10, 2017

lipemorais commented May 11, 2017 • edited Loading

cuducos commented May 12, 2017

coveralls commented May 12, 2017

lipemorais commented May 12, 2017 • edited Loading

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 12, 2017

coveralls commented May 14, 2017

lipemorais commented May 14, 2017

cuducos commented May 14, 2017

lipemorais commented May 14, 2017

cuducos commented May 14, 2017

lipemorais commented May 14, 2017

cuducos commented May 14, 2017

cuducos left a comment

Choose a reason for hiding this comment

cuducos May 14, 2017

Choose a reason for hiding this comment

cuducos May 14, 2017

Choose a reason for hiding this comment

anaschwendler commented May 15, 2017

cuducos May 16, 2017

Choose a reason for hiding this comment

lipemorais May 16, 2017

Choose a reason for hiding this comment

coveralls commented May 16, 2017

cuducos commented May 16, 2017 • edited Loading

lipemorais commented May 16, 2017

coveralls commented May 16, 2017

coveralls commented May 17, 2017

lipemorais commented May 17, 2017 • edited Loading

coveralls commented May 17, 2017

lipemorais commented May 17, 2017

coveralls commented May 17, 2017

cuducos commented May 17, 2017

gomex commented May 18, 2017

lipemorais commented May 10, 2017 •

edited by cuducos

Loading

lipemorais commented May 11, 2017 •

edited

Loading

lipemorais commented May 12, 2017 •

edited

Loading

cuducos commented May 16, 2017 •

edited

Loading

lipemorais commented May 17, 2017 •

edited

Loading