-
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
Enable the app to be built using Docker #1290
Conversation
👍 nice! |
Do all those files really have to be in the top level directory? I'd really rather avoid polluting that with lots of noise if we can avoid it... |
Also if I'm reading that right it's relying on a prebuilt image rather that using a stock image and checking out the code? So anybody that uses this is going to start with a snapshot of the code from heaven knows when rather than something current? |
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 should be a COPY or ADD instruction to get the local directory to the docker instance running the rails port. I'm not sure on the semantics of copy vs add, so I don't know which is preferred.
- Because the configuration docs require running a rails console the docker instructions should mention how to do that, as well as how to access the database directly or with psql, which are needed for populating with osmosis or general debugging.
- I don't feel qualified to review the docker-compose stuff
|
||
# Install gems | ||
ADD Gemfile /app/Gemfile | ||
ADD Gemfile.lock /app/Gemfile.lock |
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 adding Gemfile
and Gemfile.lock
, but shouldn't they come over when you bring all of the files from the local directory into docker?
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.
These are required so that bundler can install all the gem dependencies required for the app. They have to be manually added so that they are available during the image build process.
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.
The added benefit is that gems will only be reinstalled when Gemfile*
changes, rather than when anything else in the source tree does.
Dockerfile
Outdated
libmagickwand-dev \ | ||
postgresql-client \ | ||
nodejs \ | ||
file |
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 should add && apt-get clean && rm -rf /var/lib/apt/lists/
to avoid including cache files in the docker image, inflating its size.
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.
👍
INSTALL.md
Outdated
@@ -3,7 +3,7 @@ | |||
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 following the instructions below. Alternatively there are guides to use either [Vagrant](VAGRANT.md) or [Docker](DOCKER.md). |
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 would still call running software directly on your machine the traditional and probably best-supported approach.
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.
👍
cp config/example.database.yml config/database.yml | ||
``` | ||
|
||
Set `username` to postgres and `host` to db leave the password blank |
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.
Can this be automated, since we always know what the DB info is when running in docker?
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.
The guides I've read keep this kind of configuration separate. I would suggest leaving it out for now and it could be reviewed later.
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.
All of the configuration variables can be set using the environment: OSM_...
Defaults could be set in the Dockerfile
and overwritten with -e
or --env-file
or using equivalent docker-compose.yml
configuration.
Dockerfile.postgres
Outdated
|
||
RUN apt-get update && apt-get install -y make \ | ||
postgresql-server-dev-all \ | ||
build-essential |
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.
Add cleaning commands as in the Dockerfile
# Install gems | ||
ADD Gemfile /app/Gemfile | ||
ADD Gemfile.lock /app/Gemfile.lock | ||
RUN bundle 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.
In other images I've seen a CMD set so you get a meaningful console when you access it, e.g. CMD ["python2"]
. Is it possible to do the same here to get a console? bundle exec rails console
is the command used in CONFIGURE.md
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.
The CMD
is actually declared in the docker-compose.yml
as:
./script/rails s -p 3000 -b '0.0.0.0'
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 script/rails
is kind of ancient history - it should really have been deleted long since I think. A newly created rails app wouldn't have it.
I think something like bundle exec rails s -p 3000 -b '0.0.0.0'
is more like what you should be doing these days.
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 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.
Including puma
or similar would be nice if this image were also going to be used for "production" purposes (e.g. POSM, where we currently use puma
).
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.
(Commenting here because it's the end of the file.)
COPY . /app
should be included if this image is intended to be used independently of a local git clone, otherwise none of the source files will be available in the image.
(This can co-exist happily with mapping $(pwd)
to /app
as a volume; files present in the image will be shadowed by local paths.)
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.
Also consider adding -j $(nproc)
as an argument to speed up gem installation (it will run a number of processes matching the cores available on the system doing the building).
Dockerfile
Outdated
ENV REFRESHED_AT 2016-09-15 | ||
|
||
# Install packages | ||
RUN apt-get update && apt-get install -y --no-install-recommends \ |
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.
These are different than the listed minimum requirements. I can understand ruby being different since it's coming from a ruby image and the lack of postgresql, but are there reasons for the other differences?
Dockerfile
Outdated
libxml2-dev \ | ||
imagemagick \ | ||
libmagickwand-dev \ | ||
postgresql-client \ |
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.
Is this needed for anything other than debugging? I'm okay with keeping it, but if it's only for debugging we should add a comment.
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 pretty sure there was a reason to add postgresql-client
while I was getting this all up and running but I can't remember precisely off the top of my head. I'm happy to try removing it and rebuilding to see.
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.
OK I tried removing it and when running the migrations it bails out because it can't execute:
pg_dump -s -x -O -f /app/db/structure.sql openstreetmap
This is run because of the setting in application.rb
:
config.active_record.schema_format = :sql
After reviewing everything I think I understand everything, but take it with a grain of salt because I don't fully understand docker-compose. When docker builds the image it copies the Gemfile and Gemfile.lock and does a I think this works because if the gemfiles are touched it invalidates that layer of the build and requires it to be rebuilt, while the next steps ( If this works it means that you can make changes and test them without having to do a new |
If it's copying in the code then what is in the |
Having said that I can't obviously find that in the registry so maybe I am misunderstanding? |
The docs say it's what compose names the built image |
Hey guys lot's of comments, thanks. I can't address them all right at the moment but to pick one:
This isn't required as the This means you can make changes locally and see them reflected when you refresh the browser. This would obviously be different you wanted to use Docker to deploy to a production server where it would need to clone down the repo. Hopefully that makes sense and apologies if i've misunderstood! |
So I think I've figured it out now and the images from the hub it is using are:
Which seems reasonable I guess. |
@tomhughes that's exactly right. It's very easy to switch those if you prefer other versions. For instance, it would probably be a good idea to match the production server environment. |
4407104
to
b213afb
Compare
OK so I think I've addressed the comments.
The app boots and it appears to load ok, but I'm not familiar with it at all. There are 4 specs that either error or fail locally. The output is below. Does anyone know what might be causing those?
|
I'm not familiar enough with the osm-website code but for any encoding, language, or locale issues on Docker I tend to suspect the absurdly minimal set of locales the images come with. |
The first three are apparently because the languages table isn't loaded properly. I saw something similar myself the other day after merging @gravitystorm's factory changes but I couldn't reproduce it again so decided it was a transient glitch but maybe not. The last one is not something I've seen before and looks very odd to me as it appears that |
So I was going to try this, but it turns out I don't have a |
Found it - there's a separate |
So the first time I ran it the docker-compose bombed out building the web site image due to a random HTTP failure talking to the debian repos and now I can't start it because every attempt bombs out with:
Presumably because the database image from the first attempt is still running, but I can't work out how to stop it - a I can't help feeling it all seems very fragile... |
Hmm no apparently that is because it is trying to bind port 5432 on the host which, unsurprisingly, is already in use by my host system's postgres daemon! Shouldn't it be using a private network or something to connect the two containers? |
Hey @tomhughes. I'm definitely not an expert on the inner workings of Docker, but as I understand it the two containers defined in the This is possible because
The port Hope this makes sense. |
Yes, it should. |
b213afb
to
b1f90c5
Compare
OK I spotted the path to the db functions was incorrect in I think the three |
RUN apt-get install -y --no-install-recommends file | ||
RUN apt-get install -y --no-install-recommends postgresql-client | ||
RUN apt-get install -y --no-install-recommends locales | ||
RUN 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.
Standard practice is to have all the apt-get stuff on one line
@Firefishy where are we at with the improvements you were working on in Brussels so we can merge this? |
I was working with docker for something else and files mentioned in the Dockerfile need to be in |
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.
Can the sed
command be bypassed by setting this before packages are installed? Reconfiguring locales isn't fast, so doing that earlier would make it quicker to iterate when using this for development purposes.
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 those new to Docker, each Docker "command" creates a filesystem layer. Filesystem layers are (typically) cached, allowing changes to later parts of the process to skip installation steps that were already taken care of (that were also defined earlier in the Dockerfile
).)
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.
(Changes to files COPY
'd or ADD
ed into an image invalidate those layers (and subsequent ones), similar to make
.)
The general idea is that an image is created (using this
It would be possible for the image to check out a current version of the code each time it's run, but that runs counter to the "immutable infrastructure" philosophy that Docker espouses (and enables). |
|
||
# Setup app location | ||
RUN mkdir -p /app | ||
WORKDIR /app |
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.
Also consider creating/running as an unprivileged user.
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.
(Sorry, didn't mean to trigger a review on this.)
# Install gems | ||
ADD Gemfile /app/Gemfile | ||
ADD Gemfile.lock /app/Gemfile.lock | ||
RUN bundle 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.
(Commenting here because it's the end of the file.)
COPY . /app
should be included if this image is intended to be used independently of a local git clone, otherwise none of the source files will be available in the image.
(This can co-exist happily with mapping $(pwd)
to /app
as a volume; files present in the image will be shadowed by local paths.)
# Install gems | ||
ADD Gemfile /app/Gemfile | ||
ADD Gemfile.lock /app/Gemfile.lock | ||
RUN bundle 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.
Also consider adding -j $(nproc)
as an argument to speed up gem installation (it will run a number of processes matching the cores available on the system doing the building).
@mojodna I don't think there's any intention for this to be published to the registry, and it's certainly not something we're expecting people to use for production use. The goal here is to make it easier for people wanting to work on the code to get a development environment. |
Cool, that makes total sense. |
Dockerized version of osm-website would be probably useful for me, what are the chances to have this code merged? |
Yes, it would be terrific to have this - about once every two months I want to get openstreetmap-website up and running to patch something, but struggle and eventually fail (usually due to Mac peculiarities and Ruby conflicts). @paulsturgess , @mojodna , is there anything anyone can do to help this move along? |
Unfortunately I don't have the time to work further on this or check if it still works but obviously happy for someone to take this. Looking at my last comment over a year ago, it appeared to be done but there were some intermittent failures occurring on Travis but no-one was able to shed any light on those. I'm not entirely sure on the resistance to merging it and iterating on it as it has no impact on the production code. |
Isn't vagrant enough for local development? |
It looks like Vagrant is just a starting command - you have to have all the VMs already. |
No, it downloads a clean ubuntu image and runs this script, so basically you do a |
I've checked and it works with Vagrant, I was able to use the website locally reading docs. I have also found that cruise.openstreetmap.org server mentioned in CONTRIBUTING.md is unavailable (no DNS record), but that's a different issue. So Docker version might be just not needed. |
Docker is still relevant, and many developers are used to it. Could we reopen this please, even if it stays unresolved a few more months? This could probably benefit from using the same or similar files to vagrant's. |
Hi, is there any update or progress on this? I am getting started with docker technology, and I was hoping to build on top of this PR. What do you guys think about it? |
@fazlerabbi37 Well you sounds like an excellent candidate to implement docker support then, because none of us uses it - of course ideally we'd likely somebody that will actually maintain the support because otherwise it is likely to bit rot if none of us are using it. |
@tomhughes great! I will get started 😄 . I was thinking of creating a new PR. Would that be ok? |
Yes of course. |
There has been some recent docker and osm website work over on the OpenHistoricalMap side of things on this which might help your PR @fazlerabbi37 https://github.com/OpenHistoricalMap/openhistoricalmap-docker . One note for example is the conversion of the postgres C functions to SQL versions so it can work with cloud sql instances. |
That appears to be entirely unrelated to whether or not the site is running in docker though... |
@timwaters I was hoping to focus first on make the dev setup easier through docker. Once that is done, we can surely move on to make things production ready. What do you think? @tomhughes . Thanks for the OpenHistoricalMap link BTW, I will take a look at it soon 😄. |
Hi, I saw your request for help on this app and I thought maybe getting it working via Docker might make local dev more appealing.
This is a work in progress PR, not ready for merging just yet. I thought it would be good to open it to get some early feedback.
TODO: