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

[Tom Hughes]

@tomhughes commented on this pull request.

Apologies for submitting this after the PR has been merged but I hadn't realised the PR was ready for review.

> +    runs-on: ubuntu-20.04
+    steps:
+    - name: Checkout source
+      uses: actions/checkout at v1
+    - name: Poke config
+      run: |
+        cp config/example.storage.yml config/storage.yml
+        cp config/docker.database.yml config/database.yml
+        touch config/settings.local.yml
+    - name: Build Docker Image
+      run: |
+        docker-compose build
+    - name: Start Docker-Compose
+      run: |
+        docker-compose up -d
+        sleep 15 # let the DB warm up a little

That's a failure waiting to happen... Is there really not a way to actually detect when it's ready instead of guessing?

> @@ -0,0 +1,36 @@
+name: Docker
+on:
+  - push
+  - pull_request
+jobs:
+  test:
+    name: Docker
+    runs-on: ubuntu-20.04
+    steps:
+    - name: Checkout source
+      uses: actions/checkout at v1

This is out of date - there is a v2 now.

> +      run: |
+        docker-compose up -d
+        sleep 15 # let the DB warm up a little
+    - name: Prepare Database
+      run: |
+        docker-compose run --rm web rake db:migrate
+        docker-compose run web bundle exec rake i18n:js:export
+        docker-compose run --rm web osmosis --rx docker/null-island.osm.xml --wd host=db database=openstreetmap user=openstreetmap password=openstreetmap validateSchemaVersion=no
+    - name: Test Basic Website
+      run: |
+        curl -siL http://127.0.0.1:3000 | egrep '^HTTP/1.1 200 OK'
+        curl -siL http://127.0.0.1:3000 | grep 'OpenStreetMap is the free wiki world map'
+        curl -siL http://127.0.0.1:3000/api/0.6/node/1 | grep 'Null Island'
+    - name: Test Complete Suite
+      run: |
+        docker-compose run --rm web bundle exec rails test:db

I always use `rake test:db` but I admit to not being able to keep up with the constant changes about what is run via `rails` and what is run via `rake`...

> @@ -1,4 +1,4 @@
 ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../Gemfile", __dir__)
 
 require "bundler/setup" # Set up gems listed in the Gemfile.
-require "bootsnap/setup" # Speed up boot time by caching expensive operations.
+require "bootsnap/setup" if ENV.fetch("ENABLE_BOOTSNAP", "true") == "true" # Speed up boot time by caching expensive operations.

I'm sorry but there has to be a better solution to the bootsnap problem that polluting core rails setup files with this nonsense.

> +      - "3000:3000"
+    environment:
+      # https://github.com/Shopify/bootsnap/issues/262
+      ENABLE_BOOTSNAP: 'false'
+    command: bundle exec rails s -p 3000 -b '0.0.0.0'
+    depends_on:
+      - db
+
+  db:
+    build:
+      context: .
+      dockerfile: docker/postgres/Dockerfile
+    ports:
+      - "54321:5432"
+    environment:
+      POSTGRES_HOST_AUTH_METHOD: trust

If you're doing trust auth why did you put a password in the database configuration?

> +      libgd-dev \
+      libmagickwand-dev \
+      libpq-dev \
+      libsasl2-dev \
+      libxml2-dev \
+      libxslt1-dev \
+      locales \
+      nodejs \
+      postgresql-client \
+      ruby2.7 \
+      ruby2.7-dev \
+      tzdata \
+      unzip \
+      yarnpkg \
+ && apt-get clean \
+ && rm -rf /var/lib/apt/lists/*

Why is this removing part of apt's internal state?

> +      libxml2-dev \
+      libxslt1-dev \
+      locales \
+      nodejs \
+      postgresql-client \
+      ruby2.7 \
+      ruby2.7-dev \
+      tzdata \
+      unzip \
+      yarnpkg \
+ && apt-get clean \
+ && rm -rf /var/lib/apt/lists/*
+
+# Install current Osmosis
+RUN curl -OL https://github.com/openstreetmap/osmosis/releases/download/0.47.2/osmosis-0.47.2.tgz \
+ && tar -C /usr/local -xzf osmosis-0.47.2.tgz

Hard coding an omsosmis version number is a bad idea as it will need updating when osmosis changes. Why are we wasting time downloading osmosis anyway? How many people will use it? It's not like it even works very well for loading an API database which I assume is what you have it here for...

> + && tar -C /usr/local -xzf osmosis-0.47.2.tgz
+
+ENV DEBIAN_FRONTEND=dialog
+
+# Setup app location
+RUN mkdir -p /app
+WORKDIR /app
+
+# Install Ruby packages
+ADD Gemfile Gemfile.lock /app/
+RUN gem install bundler \
+ && bundle install
+
+# Install NodeJS packages
+ADD package.json yarn.lock /app/
+RUN yarnpkg install

This should be done as `bundle exec rake yarn:install` and not by running yarn directly.

> @@ -0,0 +1,20 @@
+# This configuration is tailored for use with docker-compose. See DOCKER.md for more information.
+
+development:
+  adapter: postgresql
+  database: openstreetmap
+  username: openstreetmap
+  password: openstreetmap

If you're using trust auth (which you appear to be) then you shouldn't need a password.

> @@ -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

As you appear to be using trust auth there shouldn't be any need to set a password and it would also be best not to set superuser unless have to - the only thing that is likely to be needed for installing the extension which os done as the postgres user anyway.

> @@ -0,0 +1,7 @@
+FROM postgres:11
+
+# Add db init script to install OSM-specific Postgres functions/extensions.
+ADD docker/postgres/openstreetmap-postgres-init.sh /docker-entrypoint-initdb.d/
+
+# Custom database functions are in a SQL file.
+ADD db/functions/functions.sql /usr/local/sbin/osm-db-functions.sql

I'm sorry but I agree that `/usr/local/sbin` is daft - it's not an executable and even if it was why sbin and not bin? I'd suggest lib or share for this.

-- 
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-582662631
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20210203/f109ae2e/attachment.htm>