Skip to content
This repository was archived by the owner on May 5, 2022. It is now read-only.

Conversation

@iandees
Copy link
Member

@iandees iandees commented Feb 15, 2017

While trying to get tests to run for #523, I started putting together a Docker setup for the tests.

The Dockerfile.test file describes how Docker should build the image that will run the tests. The docker-compose.test.yml file describes how the test image should connect to a container running the postgis image.

Once Docker is installed, running the tests should be as simple as:

docker-compose -f docker-compose.test.yml run --rm machine-test python setup.py test

@iandees
Copy link
Member Author

iandees commented Feb 15, 2017

As it stands now, the tests don't pass for a variety of reasons:

  • openaddr/tests/dashboard_stats.py fails because it's looking for a role in postgres that's different from the role created in the setup for the postgis instance. We can make this work by adding a script that creates the role during the postgis image setup, but I'd prefer if the code looked for a environment variable for the dashboard postgres connection.
  • Several tests seem to fail because of missing files. The entire project should be in the docker image, so I'm not sure where that's coming from.

I'm leaving it in this state because I need to start working on dayjob stuff 😄. I'll come back to it later.

@migurski
Copy link
Member

Thanks Ian! I got some excellent Docker understanding help from Jamie yesterday that's making this easier to understand.

Copy link
Contributor

@jalessio jalessio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added my two cents based on having setup something similar in a few projects before.

Dockerfile.test Outdated

RUN apt-get update \
&& apt-get upgrade -y \
&& apt-get install -y libgdal1-dev gdal-bin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendations based on Docker's Best practices for writing Dockerfiles

  • Sort multi-line arguments
  • apt-get
    • You should avoid RUN apt-get upgrade or dist-upgrade, as many of the "essential" packages from the base images won’t upgrade inside an unprivileged container.
    • cleaning up the apt cache and removing /var/lib/apt/lists helps keep the image size down
RUN apt-get update \
  && apt-get upgrade -y \
  && apt-get install -y \ 
        gdal-bin \
        libgdal1-dev && \
  rm -rf /var/lib/apt/lists/*

Also, I'm not sure I would apt-get upgrade in the Dockerfile -- I'd let the upstream python image take care of that, so I might change this to:

RUN apt-get update \
  && apt-get install -y \
        gdal-bin \
        libgdal1-dev && \
    rm -rf /var/lib/apt/lists/*

and if we really want to get picky, I find this style to be easier to read (possibly just personal preference?):

RUN apt-get update && \
    apt-get install -y \
      gdal-bin \
      libgdal1-dev && \
    rm -rf /var/lib/apt/lists/*

Dockerfile.test Outdated
RUN gdalinfo --version

RUN pip install cairocffi gdal==1.10.0 \
&& pip install -U .
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here on Dockerfile style guide. Maybe something like this:

RUN pip install \
      cairocffi \
      gdal==1.10.0 && \
    pip install -U .

@@ -0,0 +1,12 @@
machine-test:
build: .
dockerfile: Dockerfile.test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My hunch is that you could rename this file to docker-compose.yml, remove the Dockerfile line for the compose yml file, and not make the default CMD in the Dockerfile run the tests.

Instead, have the docker-compose.yml file look like this:

machine:
  build: .
  environment:
    - DATABASE_URL=postgresql://openaddr:openaddr@postgres/openaddr
  links:
    - postgres

postgres:
  image: mdillon/postgis
  environment:
    - POSTGRES_USER=openaddr
    - POSTGRES_PASSWORD=openaddr

and then run tests like this:

docker-compose run --rm machine python setup.py test

or maybe just

docker-compose run --rm machine ./setup.py test

Once you're there you could add a Makefile that turns that into a call to make test :)

@migurski
Copy link
Member

Thank you Jamie. I hope it's okay for this PR to hang out for a little while, along with #159? I am growing convinced that Docker support is necessary, but I want to eliminate Chef if/when Docker enters the project. This implies a change to how the project is deployed. #546 is also related.

@iandees
Copy link
Member Author

iandees commented Feb 16, 2017

The latest commit is based on @jalessio's feedback (but of course I forgot to alphabetize, woops). Using volume in the docker-compose seems to have helped with the tests getting farther, but I still fail on test_lake_man_gdb (openaddr.tests.conform.TestConformCli):

Traceback (most recent call last):
  File "/machine/openaddr/tests/conform.py", line 634, in test_lake_man_gdb
    rc, dest_path = self._run_conform_on_source('lake-man-gdb', 'gdb')
  File "/machine/openaddr/tests/conform.py", line 597, in _run_conform_on_source
    rc = conform_cli(source_definition, source_path, dest_path)
  File "/machine/openaddr/conform.py", line 1187, in conform_cli
    extract_to_source_csv(source_definition, source_path, extract_path)
  File "/machine/openaddr/conform.py", line 1134, in extract_to_source_csv
    ogr_source_to_csv(source_definition, ogr_source_path, extract_path)
  File "/machine/openaddr/conform.py", line 605, in ogr_source_to_csv
    in_layer = in_datasource.GetLayerByIndex(layer_id)
AttributeError: 'NoneType' object has no attribute 'GetLayerByIndex'

... I bet I'm missing something in OGR.

Also note that I removed the GRANTs from the schema.pgsql file to solve the problem with only having a single role created in the postgis container.

Note 3: I started COPYing individual files I needed during install but quickly needed to pull in the whole thing, basically, because setup.py is installing itself in addition to all the dependencies. So I COPY the whole current directory to /machine and when using the image via docker-compose I'm mounting the current directory to /machine, which overwrites the stuff copied when the image was built.

@migurski
Copy link
Member

You're missing GDAL 2.1.0 and GDB support. I'm working on a Dockerfile for this myself as part of the work I'm doing this week and next. Here's a working version I have going right now, a straightforward copy of the work that our Chef recipes are doing for us:

FROM ubuntu:14.04

RUN apt-get update -y && \
    apt-get install -y software-properties-common python-software-properties

ENV LC_ALL=C.UTF-8

# From chef/prereqs/recipes/default.rb
RUN add-apt-repository -y ppa:openaddresses/gdal2 && \
    apt-get update -y && \
    apt-get install -y python3-pip

# # Watch for compatibility between awscli, botocore, and boto3.
# RUN apt-get install -y libyaml-dev && \
#     pip3 install -U 'awscli == 1.11.50' 'botocore == 1.5.14'

# From chef/openaddr-prereqs/recipes/default.rb
RUN apt-get install -y python3-cairo libgeos-c1v5=3.5.0-1~trusty1 \
        libgdal20=2.1.0+dfsg-1~trusty2 python3-gdal=2.1.0+dfsg-1~trusty2 \
        python3-pip python3-dev libpq-dev memcached libffi-dev \
        gdal-bin=2.1.0+dfsg-1~trusty2 libgdal-dev=2.1.0+dfsg-1~trusty2

# From chef/openaddr/recipes/default.rb
COPY . /usr/local/src/openaddr
RUN cd /usr/local/src/openaddr && \
    pip3 install -U .

I'm reading back through @jalessio’s comments to figure out how to think about apt-get update, which I'm fairly certain is necessary for this.

@migurski
Copy link
Member

migurski commented Feb 22, 2017

I have committed the file above to a new branch, here: https://github.com/openaddresses/machine/blob/migurski/docker-docker-docker/Dockerfile

I’ll write more about what this is later on, but the short version is that I’m working on #559 and I view the Vagrant approach as a potential stepping stone on the way to a Docker setup. Right now, I’m doing some basic sanity checks on how large the built image is, how long it takes to build from scratch, and whether it can be used to run real OpenAddresses tasks. Answers so far:

@jalessio
Copy link
Contributor

If you move RUN add-apt-repository -y ppa:openaddresses/gdal2 to the top of the file you'd only have to run apt-get update once, which would save a bit of build time.

@migurski
Copy link
Member

I tried that first, but add-apt-repository isn’t available until software-properties-common python-software-properties have been added. Seems a bit circular.

@migurski migurski mentioned this pull request Feb 23, 2017
@migurski migurski mentioned this pull request Mar 24, 2017
5 tasks
@migurski
Copy link
Member

I’ve incorporated this work into #587, and merged changes to that branch. Closing this now to get us closer to a resolution on Docker things!

@migurski migurski closed this Apr 19, 2017
@migurski migurski deleted the docker_tests branch April 19, 2017 05:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants