started working on dockerization #3028
Conversation
jessehorne
requested review from
astronaut-wannabe,
bookis,
eliduke,
evanphx,
guitsaru,
just1602 and
veganstraightedge
as code owners
Dockerfile
Outdated
| FROM ruby:3.1.3 | ||
| WORKDIR /crimethinc | ||
| COPY . . | ||
| RUN apt-get update -qq && apt-get install -y nodejs postgresql-client ruby-full git curl libssl-dev libreadline-dev zlib1g-dev autoconf bison build-essential libyaml-dev libreadline-dev libncurses5-dev libffi-dev libgdbm-dev |
There was a problem hiding this comment.
You probably don't need to install ruby-full since the base image is ruby:3.1.3. In fact, I'd double-check if you don't only need nodejs and postgresql-client.
There was a problem hiding this comment.
Oh hey, good catch! I'm really not sure how I ended up with all of those dependencies. Must have been while I was tinkering around trying to get this to work. You were right, we only need nodejs and postgresql-client apparently.
Dockerfile
Outdated
|
|
||
| RUN gem install bundler --conservative && gem install os && bundle install | ||
|
|
||
| CMD bundle exec rails db:create && bundle exec rails db:migrate && ./script/server |
There was a problem hiding this comment.
You probably don't want to run db:create each time you start the application. You should probably provide an entrypoint that point to bin/rails and set the command to bin/rails server. Since the entrypoint is gonna be bin/rails, we're gonna be able to do stuff like docker run crimethinc:latest db:create db:migrate.
Also, you can run multiple task as once, so you could do bundle exec db:create db:migrate in one command.
There was a problem hiding this comment.
This was great advice. It ended up being so cleaner and more usable.
| version: '3' | ||
| services: | ||
| db: | ||
| image: postgres |
There was a problem hiding this comment.
You probably want to specify a version of the postgres image since data format are not necessary compatible between version, so if you redeploy and a new version has been release it may not boot because the format have change, and we need to migrate the data first.
| db: | ||
| image: postgres | ||
| volumes: | ||
| - ./tmp/db:/var/lib/postgresql/data |
There was a problem hiding this comment.
I don't know if there's a way to specify this volume at runtime or set it to a more generic path, but we don't want production data to be keep in a tmp directory. The best place would probably be something like ~/crimethinc/data where assuming crimethinc has its own user on the host machine.
| volumes: | ||
| - ./tmp/db:/var/lib/postgresql/data | ||
| environment: | ||
| - POSTGRES_HOST_AUTH_METHOD=trust |
There was a problem hiding this comment.
It's how I setup my postgres database in local, but I don't know the best practice related to production and security. Myabe it's better to have a separate password for the database.
| web: | ||
| image: crimethinc:latest | ||
| ports: | ||
| - "3000:3000" |
There was a problem hiding this comment.
Here, if you don't setup another docker-compose service to front the Crimethinc. Web app like nginx, haproxy or anything else that can be used as a reverse proxy, you'll need to expose the port 3000 to the port 443 of the host machine. But if you do so, it'll also mean you need to handle SSL certificate.
docker-compose.yml
Outdated
| links: | ||
| - db | ||
| environment: | ||
| - RAILS_ENV=docker |
There was a problem hiding this comment.
For a first draft, this is gonna be fine, tho. But I think we really need to find a way to run every environment in docker. You should probably check this PR. In rails 7.1 that is gonna be release soon, DHH add the possibility to generate a default Dockerfile.
config/environments/docker.rb
Outdated
| @@ -0,0 +1,73 @@ | |||
| require 'active_support/core_ext/integer/time' | |||
There was a problem hiding this comment.
As mention later in another comment, we should probably find a way to run every environment in the docker container.
|
@jessehorne I'm so sorry that this PR has sat here for so long with no real response from me. (the past several months have been distracted) Thank you so much for putting in this effort on the project. 🎉 I'm gonna close this PR at this time though. This issue is only looking for a Docker setup that can be deployed. Not a setup for Docker based development. I'm still open to a PR that is scoped to just a deployable Dockerfile though. 🙂 |
How does this pull request make you feel (in animated GIF format)?
What are the relevant GitHub issues?
resolves Issue 1823
What does this pull request do?
This PR introduces three new files and some minor modifications which I believed necessary to a script or two...
Dockerfilewhich contains the image definition for the rails service itselfdocker-compose.ymlwhich contains the compose yaml for launching the rails service and the postgres instance separately.config/environments/docker.rbwhich is basically a clone ofdevelopment.rbbutdatabase.ymlspecifies the username for the database to bepostgres. This was the most straight forward way to do this without having to do complicated changes on dockers side.Another thing to note, if you see
docker-compose.ymlyou'll see an environment variable specified namedPOSTGRES_HOST_AUTH_METHODwhich is set totrust. This just makes it so we don't need to auth if we're connecting locally, from what I understand, but I truly don't fully understand this flag in its entirety so we probably shouldn't deploy this until we make sure we know what we're doing here. :PI've added instructions on how to use this in the README but basically you just need to run the following two commands and it should work, considering any previous setup necessary depending on your distro/os.
docker build -t crimethinc . docker compose upp.s
The database and rails service being separate is necessary. Docker containers are designed to run one thing at a time. It's not impossible to run the database and rails service together but it is not a good idea. :D
How should this be manually tested?
Depends on how we deploy but basically, everything should be tested as normal, manually. Probably scrape the site and make sure everything is accessible just the same. Should be straight forward. We can probably test it however you have tested it before.
Is there any background context you want to provide for reviewers?
See above. There's a bit to consider here but I got it working to its current state and looking forward to discussing what we could do to make it better.
Questions for the pull request author (delete any that don't apply):
Acceptance Criteria
These should be checked by the reviewers