[openstreetmap/openstreetmap-website] Add Docker Compose Support for Development Environment (#2409)

[Andy Allan]

@gravitystorm requested changes on this pull request.

I've added the specific review points from my testing today. I also note that Firefishy raised an issue about data volumes, but I'm not enough of a Docker expert to know if it's a nice-to-have or a must-have.

The geckodriver issue raises the biggest point for me. The ruby images this Dockerfile are based on are themselves based on Debian, rather than Ubuntu. Our installation notes and Vagrantfile are both based on Ubuntu. I don't know if the geckodriver issue is easy to fix or not, but in the longer term, I'm concerned that there may be other packages that we use from Ubuntu that aren't available in Debian, or whichever debian release the ruby image of the day is based on. So I wonder if it's better to align these, and base this Dockerfile on an Ubuntu-based image instead?

> +      - cp config/example.storage.yml config/storage.yml
+      - touch config/settings.local.yml
+      - echo -e "---\nmemcache_servers:\n  - 127.0.0.1" > config/settings/test.local.yml
+      - bundle exec rake db:migrate
+      - bundle exec rake i18n:js:export
+      - bundle exec rake yarn:install
+    script:
+      - bundle exec rubocop -f fuubar
+      - bundle exec rake eslint
+      - bundle exec erblint .
+      - bundle exec brakeman -q
+      - bundle exec rake db:structure:dump
+      - sed -e "/idle_in_transaction_session_timeout/d" -e 's/ IMMUTABLE / /' -e "/^--/d" db/structure.sql > db/structure.actual
+      - diff -uw db/structure.expected db/structure.actual
+      - bundle exec rake test:db
+  - name: "Docker-Compose Configuration"

Unfortunately this is the trickiest bit to review today, since it looks like we're moving to github actions. I would suggest waiting and then reworking the PR if #3002 is merged

>  
 These instructions are based on Ubuntu 20.04 LTS, which is the platform used by the OSMF servers.
 The instructions also work, with only minor amendments, for all other current Ubuntu releases, Fedora and MacOSX
 
-We don't recommend attempting to develop or deploy this software on Windows. If you need to use Windows, then try developing this software using Ubuntu in a virtual machine, or use [Vagrant](VAGRANT.md).
+These instructions are based on Ubuntu 18.04 LTS, which is the platform used by the OSMF servers.
+The instructions also work, with only minor amendments, for all other current Ubuntu releases, Fedora and MacOSX.

There's an error here, since the proposed additions are just (out-of-date) copy+pastes of the two lines above.

If the intention is to suggest using Docker for development on Windows (I've no idea if this is a minefield or not), then best to add that to the existing instructions. Otherwise, I think we should keep the original paragraph.

> +development:
+  adapter: postgresql
+  database: openstreetmap
+  username: openstreetmap
+  password: openstreetmap
+  host: db
+  encoding: utf8
+
+# Warning: The database defined as 'test' will be erased and
+# re-generated from your development database when you run 'rake'.
+# Do not set this db to the same as development or production.
+test:
+  adapter: postgresql
+  database: osm_test
+  username: postgres
+  password:

The username and password here are different from the `openstreetmap` database, but it's not clear why. I suspect it's because the init script sets up the `openstreetmap` database user (role), but only gives it permissions on the `openstreetmap` database, whereas for tests we have other databases. Using the `postgres` role gives plenty of permissions!

Note that the tests will likely create multiple databases (e.g. `osm_test-0`, `osm_test-1` in order to run parallel tests.

My suggestion would be to create the `openstreetmap` user as a postgres superuser, as we do in the standard install notes and the vagrant provisioning script.

> @@ -0,0 +1,11 @@
+#!/bin/bash
+set -ex
+
+# Create 'openstreetmap' user
+psql -v ON_ERROR_STOP=1 -U "$POSTGRES_USER" <<-EOSQL
+    CREATE USER openstreetmap PASSWORD 'openstreetmap';
+    GRANT ALL PRIVILEGES ON DATABASE openstreetmap TO openstreetmap;
+EOSQL

This is where the `openstreetmap` role is created - see my other review note.

> +# Install system packages
+RUN apt-get update && \
+    apt-get install --no-install-recommends -y \
+      build-essential \
+      imagemagick \
+      libarchive-dev \
+      libffi-dev \
+      libmagickwand-dev \
+      libpq-dev \
+      libsasl2-dev \
+      libxml2-dev \
+      libxslt1-dev \
+      locales \
+      nodejs \
+      default-jre-headless \
+      phantomjs \

We don't use phantomjs any more, see #2544 and #2598 

Without geckodriver, the test suite doesn't work. I've had a look at packages, but I could only find [a request on debian to include geckodriver](https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=8742070 and it hasn't had much activity. Perhaps someone with better search-fu can find a debian package for geckodriver?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2409#pullrequestreview-553821838
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20201216/587b7c50/attachment-0001.htm>