<p></p>
<p><b>@tomhughes</b> commented on this pull request.</p>
<p>Apologies for submitting this after the PR has been merged but I hadn't realised the PR was ready for review.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569640220">.github/workflows/docker.yml</a>:</p>
<pre style='color:#555'>> + runs-on: ubuntu-20.04
+ steps:
+ - name: Checkout source
+ uses: actions/checkout@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
</pre>
<p>That's a failure waiting to happen... Is there really not a way to actually detect when it's ready instead of guessing?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569640584">.github/workflows/docker.yml</a>:</p>
<pre style='color:#555'>> @@ -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@v1
</pre>
<p>This is out of date - there is a v2 now.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569640907">.github/workflows/docker.yml</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>I always use <code>rake test:db</code> but I admit to not being able to keep up with the constant changes about what is run via <code>rails</code> and what is run via <code>rake</code>...</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569642965">config/boot.rb</a>:</p>
<pre style='color:#555'>> @@ -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.
</pre>
<p>I'm sorry but there has to be a better solution to the bootsnap problem that polluting core rails setup files with this nonsense.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569644006">docker-compose.yml</a>:</p>
<pre style='color:#555'>> + - "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
</pre>
<p>If you're doing trust auth why did you put a password in the database configuration?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569649638">Dockerfile</a>:</p>
<pre style='color:#555'>> + 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/*
</pre>
<p>Why is this removing part of apt's internal state?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569650242">Dockerfile</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p>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...</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569650686">Dockerfile</a>:</p>
<pre style='color:#555'>> + && 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
</pre>
<p>This should be done as <code>bundle exec rake yarn:install</code> and not by running yarn directly.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569653453">config/docker.database.yml</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>If you're using trust auth (which you appear to be) then you shouldn't need a password.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569648638">docker/postgres/openstreetmap-postgres-init.sh</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#discussion_r569647937">docker/postgres/Dockerfile</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p>I'm sorry but I agree that <code>/usr/local/sbin</code> 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.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2409#pullrequestreview-582662631">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLIORSJVX4LEUNFLS4LS5GK7DANCNFSM4JDURRXA">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AAK2OLLYTPZOQA6AIVMTHF3S5GK7DA5CNFSM4JDURRXKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEK5LTZY.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/2409#pullrequestreview-582662631",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2409#pullrequestreview-582662631",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>