Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Docker Compose Support for Development Environment #2409

Merged
merged 31 commits into from Feb 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
c514fd6
Introduce docker-compose config for development
jalessio Oct 22, 2019
2eb8e76
Add Docker README and Makefile for helper commands
jalessio Oct 22, 2019
60a18ce
Update Docker instructions to avoid copying entire settings file
jalessio Oct 24, 2019
544daa0
Fix mismatched quote and pluralization
jalessio Oct 24, 2019
b5bf4e3
Don't try to introduce top-level Makefile at this time
jalessio Oct 26, 2019
db3cbf9
Docker readme updates
jalessio Oct 28, 2019
4b4217b
More DOCKER.md cleanup
jalessio Oct 28, 2019
d5fe598
Postgres is no longer starting
jalessio Feb 16, 2020
6624510
Simplify Dockerfile
jalessio Mar 2, 2020
5a431a5
Ignore additional 'large' directories during Docker build step
jalessio Mar 2, 2020
255ce87
put osmosis installation back
jalessio Mar 2, 2020
395300d
Clarifications to Docker instructions
jalessio Mar 2, 2020
6bcd214
Update INSTALL.md to reference Docker as an option
jalessio Mar 2, 2020
f07314c
Added new parallel test job for docker-compose
migurski Feb 17, 2020
b635cf6
Added API read to docker-compose test
migurski Feb 19, 2020
c17cb84
Replaced raw PGSQL testing file with Osmosis-driven XML file
migurski Mar 15, 2020
b71a6fd
Bumped Ruby version to 2.7 to match CI config
migurski Nov 13, 2020
188fc8f
Expanded yarn call to fix test failure
migurski Nov 14, 2020
1ab5e3b
Noted yarn step in Docker instructions
migurski Dec 12, 2020
c936063
Update INSTALL.md to restore original graf
migurski Dec 17, 2020
bc98021
Switched tests to "openstreetmap"
migurski Dec 17, 2020
810a941
Added minimal Docker workflow to test Github Actions
migurski Dec 20, 2020
1e5cc59
Extended Docker action to cover full setup and tests
migurski Dec 20, 2020
8baebab
Migrated from ruby:2.7 Docker image to ubuntu:20.04
migurski Dec 18, 2020
60f8fb9
Applied documentation suggestions from code review
migurski Dec 28, 2020
c66041c
Made Bootsnap required only when asked-for
migurski Dec 28, 2020
2da3ce6
Prevent node_modules from mounting to save rake yarn:install step
jalessio Dec 29, 2020
033c3b2
Added complete Rails test suite
migurski Dec 29, 2020
8c2db90
Switched to Docker volumn for DB data
migurski Dec 30, 2020
e19e58f
Create directories for traces and images
migurski Jan 2, 2021
4fac47a
Added /home/osm/traces and /home/osm/images to persistent Docker-comp…
migurski Jan 6, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .dockerignore
@@ -0,0 +1,6 @@
docker-db-data
log
tmp
docker-cache
.travis.yml
.git
36 changes: 36 additions & 0 deletions .github/workflows/docker.yml
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

- 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
Copy link
Member

Choose a reason for hiding this comment

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

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

- name: Prepare Database
run: |
docker-compose run --rm web rake db:migrate
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
docker-compose run --rm web rake db:migrate
docker-compose run --rm web rails db:migrate

Since we are on a fairly new version of Rails, this is the same and looks a bit nicer and is more up to date.

(I will comment only this one random instance, but there are a few of them)

Copy link
Contributor

Choose a reason for hiding this comment

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

I tested locally and this seems to work well! I’m hesitant to apply this change since we seem to have the “rake” and “Rakefile” naming convention throughout the repo. Maybe we can change this globally in another PR?

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
Copy link
Member

Choose a reason for hiding this comment

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

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

3 changes: 3 additions & 0 deletions .gitignore
Expand Up @@ -18,3 +18,6 @@ public/attachments
public/export
storage
tmp

# docker-compose database directory
docker-db-data
98 changes: 98 additions & 0 deletions DOCKER.md
@@ -0,0 +1,98 @@
# Using Docker and Docker Compose for Development and Testing

These instructions are designed for setting up The Rails Port for development and testing using [Docker](https://www.docker.com/). This will allow you to install the OpenStreetMap application and all its dependencies in Docker images and then run them in containers, almost with a single command. You will need to install Docker and Docker Compose on your development machine:

- [Install Docker](https://docs.docker.com/install/)
- [Install Docker Compose](https://docs.docker.com/compose/install/)

The first step is to fork/clone the repo to your local machine:

git clone https://github.com/openstreetmap/openstreetmap-website.git

Now change working directory to the `openstreetmap-website`:

cd openstreetmap-website

## Initial Setup

### Storage

cp config/example.storage.yml config/storage.yml

### Database

cp config/docker.database.yml config/database.yml

## Prepare local settings file

This is a workaround. [See issues/2185 for details](https://github.com/openstreetmap/openstreetmap-website/issues/2185#issuecomment-508676026).

touch config/settings.local.yml

## Installation

To build local Docker images run from the root directory of the repository:

docker-compose build

If this is your first time running or you have removed cache this will take some time to complete. Once the Docker images have finished building you can launch the images as containers.

To launch the app run:

docker-compose up -d

This will launch one Docker container for each 'service' specified in `docker-compose.yml` and run them in the background. There are two options for inspecting the logs of these running containers:

- You can tail logs of a running container with a command like this: `docker-compose logs -f web` or `docker-compose logs -f db`.
- Instead of running the containers in the background with the `-d` flag, you can launch the containers in the foreground with `docker-compose up`. The downside of this is that the logs of all the 'services' defined in `docker-compose.yml` will be intermingled. If you don't want this you can mix and match - for example, you can run the database in background with `docker-compose up -d db` and then run the Rails app in the foreground via `docker-compose up web`.

### Migrations

Run the Rails database migrations:

docker-compose run --rm web bundle exec rake db:migrate

### Tests

Run the test suite by running:

docker-compose run --rm web bundle exec rake test:db

### Loading an OSM extract
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since loading an extract via osmosis stopped working after PR #2432 I'm inclined to remove this section until I can offer a working solution.

Copy link
Contributor

@mmd-osm mmd-osm Mar 2, 2020

Choose a reason for hiding this comment

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

There’s a work around available: #2543 (comment)

Just add it to the description. You would need to call psql in any case to correct the various Postgres sequences, so the additional pain to add and drop a column should be close to zero.

Copy link
Contributor

@migurski migurski Mar 2, 2020

Choose a reason for hiding this comment

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

It’s a super-ugly workaround, almost to the point that I think it’d be harmful just to recommend that people monkey with the schema pre- and post-import. Let’s remove the section for now?

Copy link
Contributor

Choose a reason for hiding this comment

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

It’s as ugly as having to tweak sequences. My point was, it was already ugly before, we’re not adding much to that by this workaround.

Copy link
Contributor

Choose a reason for hiding this comment

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

Osmosis 0.47.1 now correctly writes to the DB so this section should work fine.


This installation comes with no geographic data loaded. You can either create new data using one of the editors (Potlatch 2, iD, JOSM etc) or by loading an OSM extract. Here an example for loading an OSM extract into your Docker-based OSM instance.

For example, let's download the District of Columbia from Geofabrik or [any other region](https://download.geofabrik.de):

wget https://download.geofabrik.de/north-america/us/district-of-columbia-latest.osm.pbf

You can now use Docker to load this extract into your local Docker-based OSM instance:

docker-compose run --rm web osmosis \
-verbose \
--read-pbf district-of-columbia-latest.osm.pbf \
--write-apidb \
host="db" \
database="openstreetmap" \
user="openstreetmap" \
validateSchemaVersion="no"

Once you have data loaded for Washington, DC you should be able to navigate to [`http://localhost:3000/#map=12/38.8938/-77.0146`](http://localhost:3000/#map=12/38.8938/-77.0146) to begin working with your local instance.

### Additional Configuration

See [`CONFIGURE.md`](CONFIGURE.md) for information on how to manage users and enable OAuth for iD, JOSM etc.

### Bash

If you want to get into a web container and run specific commands you can fire up a throwaway container to run bash in via:

docker-compose run --rm web bash

Alternatively, if you want to use the already-running `web` container then you can `exec` into it via:

docker-compose exec web bash

Similarly, if you want to `exec` in the db container use:

docker-compose exec db bash
50 changes: 50 additions & 0 deletions Dockerfile
@@ -0,0 +1,50 @@
FROM ubuntu:20.04

ENV DEBIAN_FRONTEND=noninteractive

# Install system packages
RUN apt-get update \
&& apt-get install --no-install-recommends -y \
build-essential \
curl \
default-jre-headless \
file \
firefox-geckodriver \
imagemagick \
libarchive-dev \
libffi-dev \
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/*
Copy link
Member

Choose a reason for hiding this comment

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

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


# 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
Copy link
Member

Choose a reason for hiding this comment

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

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


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
Copy link
Member

Choose a reason for hiding this comment

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

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

6 changes: 5 additions & 1 deletion INSTALL.md
Expand Up @@ -3,7 +3,11 @@
These instructions are designed for setting up The Rails Port for development and testing.
If you want to deploy the software for your own project, then see the notes at the end.

You can install the software directly on your machine, which is the traditional and probably best-supported approach. However, there is an alternative which may be easier: Vagrant. This installs the software into a virtual machine, which makes it easier to get a consistent development environment and may avoid installation difficulties. For Vagrant instructions, see [VAGRANT.md](VAGRANT.md).
You can install the software directly on your machine, which is the traditional and probably best-supported approach. However, there
are two alternatives which make it easier to get a consistent development environment and may avoid installation difficulties:

* **Vagrant** This installs the software into a virtual machine. For Vagrant instructions see [VAGRANT.md](VAGRANT.md).
* **Docker** This installs the software using containerization. For Docker instructions see [DOCKER.md](DOCKER.md).

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
Expand Down
2 changes: 1 addition & 1 deletion config/boot.rb
@@ -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.
Copy link
Member

Choose a reason for hiding this comment

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

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

20 changes: 20 additions & 0 deletions config/docker.database.yml
@@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

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: openstreetmap
password: openstreetmap
host: db
encoding: utf8
40 changes: 40 additions & 0 deletions docker-compose.yml
@@ -0,0 +1,40 @@
version: "3"

services:
web:
build:
context: .
volumes:
- .:/app
# Prevent these directories from mounting so they're not shared between host OS and Docker
- /app/node_modules/
- /app/tmp/
# Mount these upload directories so they persist between runs
- web-traces:/home/osm/traces
- web-images:/home/osm/images
ports:
- "3000:3000"
environment:
# https://github.com/Shopify/bootsnap/issues/262
ENABLE_BOOTSNAP: 'false'
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see where ENABLE_BOOTSNAP would be used right now. Did you maybe forget to check in your changes to config/boot.rb here?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is me chasing down some Ruby 2.5/2.7 vs. Ubuntu 18/20 weirdness when migrating away from the ruby:2.7 base image per @gravitystorm’s recommendation above. I might be able to back this out if the latest Ubuntu 20.04 + Ruby 2.7 commit builds cleanly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Some details on why Bootsnap. Without these references in docker-compose.yml, we see this output:

+ docker-compose run --rm web bundle exec rake db:migrate
Starting openstreetmap-website_db_1 ... done
Errno::ENOENT: No such file or directory - bs_fetch:atomic_write_cache_file:chmod
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/compile_cache/iseq.rb:30:in `fetch'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/compile_cache/iseq.rb:30:in `fetch'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/compile_cache/iseq.rb:47:in `load_iseq'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:23:in `require'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:23:in `block in require_with_bootsnap_lfi'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/loaded_features_index.rb:92:in `register'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `require_with_bootsnap_lfi'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:31:in `require'
/var/lib/gems/2.7.0/gems/activesupport-6.0.3.4/lib/active_support/dependencies.rb:324:in `block in require'
/var/lib/gems/2.7.0/gems/activesupport-6.0.3.4/lib/active_support/dependencies.rb:291:in `load_dependency'
/var/lib/gems/2.7.0/gems/activesupport-6.0.3.4/lib/active_support/dependencies.rb:324:in `require'
/var/lib/gems/2.7.0/gems/bootstrap-4.5.3/lib/bootstrap.rb:61:in `register_rails_engine'
/var/lib/gems/2.7.0/gems/bootstrap-4.5.3/lib/bootstrap.rb:11:in `load!'
/var/lib/gems/2.7.0/gems/bootstrap-4.5.3/lib/bootstrap.rb:75:in `<main>'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:23:in `require'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:23:in `block in require_with_bootsnap_lfi'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/loaded_features_index.rb:92:in `register'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:22:in `require_with_bootsnap_lfi'
/var/lib/gems/2.7.0/gems/bootsnap-1.5.1/lib/bootsnap/load_path_cache/core_ext/kernel_require.rb:31:in `require'
/var/lib/gems/2.7.0/gems/bundler-2.2.3/lib/bundler/runtime.rb:66:in `block (2 levels) in require'
/var/lib/gems/2.7.0/gems/bundler-2.2.3/lib/bundler/runtime.rb:61:in `each'
/var/lib/gems/2.7.0/gems/bundler-2.2.3/lib/bundler/runtime.rb:61:in `block in require'
/var/lib/gems/2.7.0/gems/bundler-2.2.3/lib/bundler/runtime.rb:50:in `each'
/var/lib/gems/2.7.0/gems/bundler-2.2.3/lib/bundler/runtime.rb:50:in `require'
/var/lib/gems/2.7.0/gems/bundler-2.2.3/lib/bundler.rb:174:in `require'
/app/config/application.rb:19:in `<top (required)>'
/app/rakefile:4:in `require_relative'
/app/rakefile:4:in `<top (required)>'
/usr/share/rubygems-integration/all/gems/rake-13.0.1/exe/rake:27:in `<top (required)>'
(See full trace by running task with --trace)

With these lines included, rake db:migrate runs to completion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Clarification: the error output above happens on my local machine (MacOS, Docker 2.3). Same process seems happy on GH Actions: https://github.com/migurski/openstreetmap-website/runs/1618711125

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, on Github, it's pretty much a no-op, as this environment variable isn't being used anywhere b/c config/boot.rb isn't part of this pull request. I also haven't seen this before on Ubuntu 20.04 host.

Copy link
Contributor

Choose a reason for hiding this comment

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

Any advice? I don’t know what Bootsnap is or does, I’m just blindly searching error messages here.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the Github issue you've linked to, they also replaced one line in a file named config/boot.rb:

On my local box, there's one line to load the bootsnap/setup module, which is used to speed up Rails application loading:

require "bootsnap/setup" # Speed up boot time by caching expensive operations.

They replace that line by making the loading process depend on your new environment variable ENABLE_BOOTSNAP:

require 'bootsnap/setup' if ENV.fetch("ENABLE_BOOTSNAP", "true") == "true"

Copy link
Contributor

@migurski migurski Dec 28, 2020

Choose a reason for hiding this comment

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

That makes sense — I’ve added this check in config/boot.rb and it seems to now work both locally and on GH. Thanks!

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
Copy link
Member

Choose a reason for hiding this comment

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

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

POSTGRES_DB: openstreetmap
volumes:
# Mount the Postgres data directory so it persists between runs
- db-data:/var/lib/postgresql/data

volumes:
web-traces:
web-images:
db-data:
7 changes: 7 additions & 0 deletions docker/null-island.osm.xml
@@ -0,0 +1,7 @@
<?xml version='1.0' encoding='UTF-8'?>
<osm version="0.6" generator="Osmosis 0.47.2">
<bounds minlon="-180.00000" minlat="-90.00000" maxlon="180.00000" maxlat="90.00000" origin="Osmosis 0.47.2"/>
<node id="1" version="1" timestamp="1970-01-01T00:00:00Z" uid="1" user="nobody" lat="0" lon="0">
<tag k="name" v="Null Island"/>
</node>
</osm>
7 changes: 7 additions & 0 deletions docker/postgres/Dockerfile
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of comments:

  • /usr/local/sbin seems to be a somewhat unusual location for sql scripts.

  • Also, I'm not 100% sure if we need this script at all. At least maptile_for_point (for the /changes endpoint) and xid_to_int4 (for the old osmosis based replication) are likely candidates to be removed in the near future. tile_for_point functionality is probably part of the quad_tile, and might also no longer be needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have an alternate suggestion for a location?

Tests fail when these functions are excluded.

Copy link
Contributor

@mmd-osm mmd-osm Dec 29, 2020

Choose a reason for hiding this comment

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

Do you recall which test did in fact fail? My guess here would be the /changes endpoint stuff.

Regarding the location, can't you use /app/db/functions/functions.sql as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here’s the failing test suite that was fixed by adding functions.sql: https://github.com/migurski/openstreetmap-website/runs/1619876640?check_suite_focus=true

We don’t use /app in the DB container, so it’s not there by default.

Copy link
Contributor

@mmd-osm mmd-osm Dec 29, 2020

Choose a reason for hiding this comment

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

Ah, yes, right, the script needs to go to another container. Assumption confirmed, it was the /changes endpoint. I really want to get rid of this code, as it's outdated and not used by anyone except for some single unknown user. (-> re-opened #2486). Once this is done, we don't need the sql script anymore. For the time being we can leave it as-is, it's not worth fiddling much with it.

 Failure:
Api::ChangesControllerTest#test_changes_hours_valid [/app/test/controllers/api/changes_controller_test.rb:91]:
Expected response to be a <2XX: success>, but was a <500: Internal Server Error>
Response body: ActiveRecord::StatementInvalid: PG::UndefinedFunction: ERROR:  function maptile_for_point(integer, integer, integer) does not exist
LINE 1: SELECT COUNT(*) AS count_all, maptile_for_point(latitude, lo...

Copy link
Contributor

@mmd-osm mmd-osm Feb 4, 2021

Choose a reason for hiding this comment

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

That's a bit odd that you need those migrations on an empty database inside a container. I mean, we're starting fresh with a modern state of the database sql scripts, and skip any ancient stuff. Would we ever even run those ancient migrations inside the Docker container?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To answer your second question, yes, you run those old migrations - the instructions have docker-compose run --rm web bundle exec rake db:migrate

But the wider topic is whether we should keep those old migrations, whether we see the database defined by either a) the sum of all the migrations or b) the structure.sql file (opinions vary), and finally whether if we do drop old migrations should we have a 'fake' starter migration, or should we just keep a certain number of migrations and change all the documentation to use rake db:structure:load (or similar), and finally (finally) if we don't keep all the migrations, how do we ensure that the structure file matches the sum of the migrations that have been run production. So that's not a simple set of decisions.

Copy link
Contributor

@mmd-osm mmd-osm Feb 4, 2021

Choose a reason for hiding this comment

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

Somehow I assumed a new database would be set up using db/structure.sql, and then the contents of the schema_migrations table would define, which additional migrations would be needed, by comparing that table's contents against what's inside the db/migrate directory.

Maybe the mechanism works in a completely different way, but then I don't really know what's the purpose of db/structure.sql is in the first place (sorry, I know this all sounds much like noob questions).

Copy link
Contributor

@mmd-osm mmd-osm Feb 4, 2021

Choose a reason for hiding this comment

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

In any case, running the migration passes on a fresh install even without the db scripts, so I'm even more confused now why they would be needed. We don't really have any data to be backfilled on an empty database, don't we?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's discuss this on a new issue - otherwise this conversation will never be found!

14 changes: 14 additions & 0 deletions docker/postgres/openstreetmap-postgres-init.sh
@@ -0,0 +1,14 @@
#!/bin/bash
set -ex

# Create 'openstreetmap' user
psql -v ON_ERROR_STOP=1 -U "$POSTGRES_USER" <<-EOSQL
CREATE USER openstreetmap SUPERUSER PASSWORD 'openstreetmap';
GRANT ALL PRIVILEGES ON DATABASE openstreetmap TO openstreetmap;
EOSQL
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.


# Create btree_gist extensions
psql -v ON_ERROR_STOP=1 -U "$POSTGRES_USER" -c "CREATE EXTENSION btree_gist" openstreetmap

# Define custom functions
psql -v ON_ERROR_STOP=1 -U "$POSTGRES_USER" -f "/usr/local/sbin/osm-db-functions.sql" openstreetmap