-
Notifications
You must be signed in to change notification settings - Fork 912
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
Add Docker Compose Support for Development Environment #2409
Conversation
@gravitystorm any thoughts on how to move this forward? I believe it's in a usable state and offers a quick way to get up and running with this project locally. I've been using it locally for over a week now without any issues. I could definitely benefit from a few people trying it out from scratch and providing feedback, but perhaps that's more likely to occur once it's on @zerebubuth / @Firefishy / @mmd-osm - would any of you be willing to give this branch a quick run-through locally since you were all active in the previous PR (#2406)? Thanks! |
@jalessio Maybe write an OSM diary entry (or similar) explaining what this is and how it would be useful to people - explain why would they benefit from having a local copy of this code, and what Docker is and how that helps them to do that? I appreciate that there's some of that at the top of #2272 , but a bit more info would also be useful. |
Test instructions are here: #2401 (comment) |
I tested the set up yesterday, and it looks good. It took forever to download phantomjs and I ended up removing it from the Dockerfile. phantomjs project is currently in some "suspended" status, not sure if we'd better replace it or not. @jalessio : as most of the editing API calls run via CGImap in production these days, we might consider (optionally) including CGImap https://github.com/zerebubuth/openstreetmap-cgimap/blob/master/Dockerfile as well, Some people claim is this notoriously difficult to set up, so adding this to docker compose seems worthwhile to me. |
I'll have another look later in the week - it looks like you've already taken care of a few other things that I was going to query.
No, please don't. I'm happy to see stuff in this repository to help with setting up development environments, but we deliberately leave production choices out of consideration. |
We'll add it to the CGImap repo then, no worries. I don't really care which repo hosts this config, as long as we offer this option at all. |
As a suggestion i would like to say that you could build your own image and publish at Docker Hub. So we would just run it. Maybe the database image and the website image. The only flaw I see is regarding the configuration file. The OSM guys could accept database configuration via environment variables and not only by YAML. I think the OSM WEBSITE is not very flexible and was build in a very |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great to me! Just a few changes required to bring this into line with INSTALL.md
Talking of which, there should be a paragraph added to the top of INSTALL.md to describe that you can use Docker to set up your development environment, just like you can with Vagrant.
Dockerfile
Outdated
RUN mkdir -p /usr/share/man/man1 && \ | ||
mkdir -p /usr/share/man/man7 | ||
|
||
# npm is not available in Debian repo so following official instruction [source: https://github.com/nodesource/distributions/blob/master/README.md#debinstall] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have more knowledge of Ubuntu than Debian, but as far as I can tell:
- ruby:2.5 is an alias for ruby:2.5-buster (https://hub.docker.com/_/ruby/)
- buster contains npm (https://packages.debian.org/buster/npm)
- but we don't need npm anyway, only
nodejs
andyarn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best to use FROM: ruby:2.5
rather than FROM: ruby:2.5-buster
... The images upstream will pull in changes.
Dockerfile
Outdated
|
||
# install npm packages | ||
RUN npm install -g --unsafe-perm \ | ||
phantomjs-prebuilt \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debian buster comes with a phantomjs package. Should we just install that instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gravitystorm I moved to installing phantomjs
via apt.
Dockerfile
Outdated
# install npm packages | ||
RUN npm install -g --unsafe-perm \ | ||
phantomjs-prebuilt \ | ||
yarn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find using npm to install yarn a bit of a roundabout way to go. Could we install the yarn package using the yarn repository instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gravitystorm Agreed! I'm now installing yarn
via apt.
DOCKER.md
Outdated
|
||
See `CONFIGURE.md` for information on how to manage users and enable OAuth for iD, JOSM etc. | ||
|
||
### Tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running the tests section should come before the loading an extract or additional configuration sections. This is because the goal for the user is to ensure their development environment is set up correctly, as validated by the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Done
DOCKER.md
Outdated
@@ -0,0 +1,100 @@ | |||
# Using Docker and Docker Compose to run OpenStreetMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Using Docker and Docker Compose to set up The Rails Port for development and testing" - based on the wording in the current INSTALL.md and to make it clear that these docker images are designed for development and testing, not necessarily for production.
DOCKER.md
Outdated
|
||
Run the Rails database migrations: | ||
|
||
docker-compose run --rm web rake db:migrate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency and reliability, we should have ... bundle exec rake db:migrate
here
DOCKER.md
Outdated
|
||
### Tests | ||
|
||
docker-compose run --rm web rake test:db |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bundle exec
Dockerfile
Outdated
dpkg-reconfigure --frontend=noninteractive locales && \ | ||
update-locale LANG=en_GB.UTF-8 | ||
|
||
ENV LANG en_GB.UTF-8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There needs to be a bit about bundle exec rake yarn:install
somewhere, either here in the docker config, or elsewhere in the manual installation notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not calling the yarn
installer during Docker image build.
What about config the database connection using environment vars? |
@icemagno What's your usecase? Would this be for your development environment, or for production deployments? |
@gravitystorm I'm developing a distributed software with about 50 microservices in Spring boot and must follow some Standards in the continuous delivery and infrastructure deployment. I already have it running and using a old OSM server I've created. Now I have a Docker conteiner running a very nice and rich styled OSM and want to create a software to create and edit OSM and custom features ( from the Brazilian Gov. Metadata Standard ). So I'm trying some options mostly (actualy a must) in Docker. The installation way I seen here is good and working but I need some more flexible because I need to document the installation process to the future and it must follow the current standard wich is one script for each container and must be possible to replace any container without affect others. So I need one script to build and run the database and another to do the same with the website. No docker compose is allowed even if it is the easy way to do it. Each build and run process must be documented alone. |
This blog post https://blog.codeship.com/running-rails-development-environment-docker/ should give you some idea how to set up database.yml to fetch database user and password from environment variables. Besides the requirements you describe seem rather out of scope for this pull request. |
@mmd-osm thanks. I'll give a look but you may consider use it anyway because this is the best way to offer a Docker image. Under my viewpoint it is not a good idea to create hard coded configurations in a Docker deploy. It is extremely boring editing files before building and sometimes I think the Docker premisse of "fire-and-go" is broken. And the user must be free to choose database names, hosts and etc. Almost forgot: congratulations. You guys are doing a very good work. |
FYI. A complete stack consisting of Rails port, Postgres db, cgimap and lighttpd is now described in more detail in zerebubuth/openstreetmap-cgimap#213. It leverages the Postgres and Rails port set up outlined in this issue, and extends docker-compose-yml by two additional services for cgimap and lighttpd. |
TODO - possibly switch to Debian Bionic base image to match other parts of this repo. See #2473 |
I just ran through the complete process of running this Docker environment, to test the user experience of a random passerby who’d like to contribute an edit to the site. @jalessio walked me through setting up Docker-Compose, initializing a clean new database, importing data using Osmosis (from a small PBF extract), and verifying that everything worked at I made the smallest change I could think of: updating the header text in a way that was obviously visible (history/schmistory, etc.). Rails hot-reloaded everything, and I was able to see my change immediately: As a Rails novice who doesn't expect to become an expert, this has been a super useful way to understand the moving parts of the website and test potential changes. I can imagine helping a designer implement an idea here and test it out live, useful to getting new ideas into OSM. I know that Docker configs can fall out of date over time, but as we’ve seen with openstreetmap/chef#266 tests for this can be added to our Travis configuration and images can be published to Docker Hub where a casual developer can try them out. |
docker-compose.yml
Outdated
POSTGRES_HOST_AUTH_METHOD: trust | ||
POSTGRES_DB: openstreetmap | ||
volumes: | ||
- ./docker-db-data:/var/lib/postgresql/data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should really be a data volume, rather than a file mapping.
See: https://docs.docker.com/compose/compose-file/compose-file-v2/#volume-configuration-reference
753b670
to
7dc7996
Compare
|
||
docker-compose run --rm web bundle exec rake test:db | ||
|
||
### Loading an OSM extract |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Dockerfile
Outdated
|
||
# Install NodeJS packages | ||
ADD package.json yarn.lock /app/ | ||
RUN yarn |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gravitystorm I realize you've been consistently asking for using the bundle exec
form of commands for consistency (which I appreciate), but I'll try to convince you of this exception...
I toiled with getting this to work via bundle exec rake db:migrate
, but I couldn't get it to run without adding a big chunk of the repository into the image during build. It ended up adding a bunch of extra files to the image just so that I could run bundle exec rake db:migrate
instead yarn
directly. I believe the result is the same in the end so I went with the simpler option.
That said, perhaps someone else could compile a small list of files necessary to successfully run rake
commands during the image build?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're already done bundle install
which should install everything bundle exec
might need - if you don't use bundle exec
then you have no idea if things are using the expected modules, or even if they will work at all.
I made a number of changes to this PR and I believe I have addressed all the comments / requests for changes (except one! see below). I have also rebased it against master. TODO
|
This reverts commit 20dbf89.
Co-authored-by: Tobias <t@tobiasjordans.de>
- Added db/functions/functions.sql to docker-compose DB image - Added `file`, `libgd-dev`, and `unzip` packages to help tests pass
d37f606
to
4fac47a
Compare
I’ve rebased this PR to keep it updated. @gravitystorm I’ve confirmed that the directory litter you pointed out in a previous review has been fixed since late last year. I believe we’ve addressed all feedback. |
Great, thanks @migurski and everyone else who contributed! I did find a first-run issue with the tests, involving i18n-js. But during my investigations (checking the web interface, running the tests again) everything worked, so something triggered the file generation. And now I can't recreate it, so I just mention it in case it comes up again for other people at some point. But I didn't think it was a showstopper, so I'm happy to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for submitting this after the PR has been merged but I hadn't realised the PR was ready for review.
- name: Start Docker-Compose | ||
run: | | ||
docker-compose up -d | ||
sleep 15 # let the DB warm up a little |
There was a problem hiding this comment.
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?
runs-on: ubuntu-20.04 | ||
steps: | ||
- name: Checkout source | ||
uses: actions/checkout@v1 |
There was a problem hiding this comment.
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.
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 |
There was a problem hiding this comment.
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
...
@@ -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. |
There was a problem hiding this comment.
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.
ports: | ||
- "54321:5432" | ||
environment: | ||
POSTGRES_HOST_AUTH_METHOD: trust |
There was a problem hiding this comment.
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?
psql -v ON_ERROR_STOP=1 -U "$POSTGRES_USER" <<-EOSQL | ||
CREATE USER openstreetmap PASSWORD 'openstreetmap'; | ||
GRANT ALL PRIVILEGES ON DATABASE openstreetmap TO openstreetmap; | ||
EOSQL |
There was a problem hiding this comment.
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.
unzip \ | ||
yarnpkg \ | ||
&& apt-get clean \ | ||
&& rm -rf /var/lib/apt/lists/* |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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...
|
||
# Install NodeJS packages | ||
ADD package.json yarn.lock /app/ | ||
RUN yarnpkg install |
There was a problem hiding this comment.
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.
adapter: postgresql | ||
database: openstreetmap | ||
username: openstreetmap | ||
password: openstreetmap |
There was a problem hiding this comment.
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.
Thanks @gravitystorm, so excited to see this merged! @tomhughes, I’ll follow up on your points in #3081 and I’ll continue to keep a close eye on any Docker-related issues that emerge in this repo. |
And I'm sorry for merging it without checking if you were happy with it. It's one of the downsides of PRs with 100+ comments, I guess - maybe we should get better at closing and reopening PRs after several rounds of changes. And I'm kicking myself about the |
This is a cleaned up version of PR #2406 which was built upon the work of @fazlerabbi37 in PR #2272.
docker-compose
for creating a development environment for this projectDOCKER.md
for Docker-specific installation instructionsAdds aMakefile
for providing shortcuts for running commondocker-compose
commandsThere is certainly still room for improvement here, including pruning unneeded dependencies in the top-level
Dockerfile
, but I propose merging this working version and iterating from that point (particularly since many of the changes will apply regardless of the installation method). I think we will benefit from feedback of a few people actually using this before making too many more changes.