Skip to content
This repository has been archived by the owner on May 5, 2022. It is now read-only.

Docker and Docker Compose support #34

Merged
merged 1 commit into from
Oct 6, 2017
Merged

Docker and Docker Compose support #34

merged 1 commit into from
Oct 6, 2017

Conversation

zfletch
Copy link
Contributor

@zfletch zfletch commented Oct 6, 2017

Add Docker and Docker Compose support by adding Dockerfiles and a docker-compose.yml.

The Docker setup is a little different for React/Webpacker apps and standard Rails app, so different files are copied depending on whether run with --webpack=react or not.

There’s a lot I don’t know about Webpacker and Docker, so it’s possible there are better ways to set it up.

Fixes #2

copy_file "templates/Dockerfile-assets", "Dockerfile-assets"
copy_file "templates/.env.docker/.env.docker-webpack", ".env.docker"
copy_file "templates/.env.docker-assets", ".env.docker-assets"
copy_file "templates/docker-compose.yml/docker-compose-webpack.yml", "docker-compose.yml"
Copy link
Contributor Author

@zfletch zfletch Oct 6, 2017

Choose a reason for hiding this comment

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

The way I organized the files that are different for React and non-React is:

templates/
  file_name.extension/
    file_name-standard.extension
    file_name-webpack.extension

I think this is reasonable, but I could change it if there's already a standard in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Granted, this is not reflected in the current directory structure of templates (as it started off flat), but I've been thinking of modifying it to group related elements together, like having a directory for ci, db, style, etc.

Perhaps we could start that by having a directory templates/docker that all of the docker files in this diff live in? Other than that, I'm fine with your organization.

Copy link
Contributor Author

@zfletch zfletch Oct 6, 2017

Choose a reason for hiding this comment

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

I think it makes sense to group the files, and to do that in another PR so the locations of all the files are consistent. I also think before doing that it would be good to figure out #16 since testing that everything works with and without Docker and with and without React takes a long time

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. #16 has been my white whale since starting the project. I agree with your thoughts though.

@@ -0,0 +1 @@
WEBPACKER_DEV_SERVER_HOST=0.0.0.0
Copy link
Contributor Author

@zfletch zfletch Oct 6, 2017

Choose a reason for hiding this comment

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

Using environment variables to configure the Webpack Dev server is a new feature merged just a few days ago. Previously you had to change the config/webpacker.yml file.

You still can't use command line arguments, because why would you need to use command line arguments when you can just edit a yaml file?

image: postgres:9.6.5
web:
build: .
command: bash -c 'rm -f tmp/pids/server.pid && bundle exec rails s --port 3000 --binding 0.0.0.0'
Copy link
Contributor Author

@zfletch zfletch Oct 6, 2017

Choose a reason for hiding this comment

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

The reason for the rm -f code is to avoid A server is already running. Check /app/tmp/pids/server.pid. errors when running docker-compose up. See this Docker Compose issue.

I looked at how some other applications solve the problem, and it looks like this works. Quoting mbarnett commenting on this pull request, linked from the GitHub issue:

Hacky, but what are you gonna do ¯\(ツ)

Copy link
Contributor

Choose a reason for hiding this comment

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

Smart. I'm still manually using docker-compose run web bash to run this when necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should be able to delete it locally by running rm tmp/pids/server.pid if you have volumes: - .:/app in your docker-compose.yml

@zfletch zfletch changed the title docker and docker compose support Docker and Docker Compose support Oct 6, 2017
@@ -0,0 +1 @@
WEBPACKER_DEV_SERVER_HOST=0.0.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the convention you have, should this be in the .env.docker directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are three .env files involved: .env.docker/.env-docker-standard, .env.docker/.env-docker-webpack, and .env-docker-assets.

The first two are copied as .env.docker in the created application. They need to have different values for the Webpack and non-Webpack cases.

The third one is copied as .env.docker-assets in the created application, but it's only copied when --webpack=react is passed in.

image: postgres:9.6.5
web:
build: .
command: bash -c 'rm -f tmp/pids/server.pid && bundle exec rails s --port 3000 --binding 0.0.0.0'
Copy link
Contributor

Choose a reason for hiding this comment

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

Smart. I'm still manually using docker-compose run web bash to run this when necessary.

version: '3'
services:
db:
image: postgres:9.6.5
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we targeting a specific release? Or, I guess I could see wanting to tie it to a specific release, but why not the latest (10)?

Update: Postgres 10 literally came out yesterday, so, that's a perfectly reasonable explanation as to why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It probably makes more sense here not to target a particular version of Postgres. I copied this from another project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it depends on if we want to standardize/lock in on a particular version until we all agree to upgrade, like a ruby version update. However, I'm not sure how much we should really care about what version of backing services we're using if we're targeting AWS RDS/Heroku?

Maybe @ngmaloney would have a better opinion on this than I would.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Once we create an application, it might make sense, but I don't think we want to be updating this repo every time a new version of Postgres comes out.

@@ -0,0 +1,20 @@
FROM ruby:2.4.2

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming you've tested this w/spinning up a postgres DB with some data in it and such with it working fine?

I only ask because EEC's Dockerfile has some stuff about setting up keys for postgres, which I can't claim to understand why it's needed, but figured I'd toss it out there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if what that code is for or whether it's currently necessary in EEC Web. I commented it out locally and was able to build and run the app successfully.

ADD Gemfile /app/Gemfile
ADD Gemfile.lock /app/Gemfile.lock
RUN bundle install

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried running acceptance tests with this? Again, just basing on EEC's file having various lines for chrome-driver setup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't tested Chrome driver. I think there's potentially enough work getting that running that it should be a separate issue.

copy_file "templates/Dockerfile-assets", "Dockerfile-assets"
copy_file "templates/.env.docker/.env.docker-webpack", ".env.docker"
copy_file "templates/.env.docker-assets", ".env.docker-assets"
copy_file "templates/docker-compose.yml/docker-compose-webpack.yml", "docker-compose.yml"
Copy link
Contributor

Choose a reason for hiding this comment

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

Granted, this is not reflected in the current directory structure of templates (as it started off flat), but I've been thinking of modifying it to group related elements together, like having a directory for ci, db, style, etc.

Perhaps we could start that by having a directory templates/docker that all of the docker files in this diff live in? Other than that, I'm fine with your organization.


```sh
docker-compose up
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the documentation. This has led me to create #35 to handle separately - which basically says we should take all this information and populate it in the README of the projects we're creating from this template as well, along with other setup info.


ADD package.json /app/package.json
ADD yarn.lock /app/yarn.lock
RUN yarn install
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this file should only be the above 3 lines (as everything else looks the same as the Dockerfile).

Then we could copy over Dockerfile twice - once for web, and another for assets. For the assets version, we can then use insert_into_file "Dockerfile-assets", before: " \ADD . /app\n" do...end, reading in and inserting these 3 lines here.

That way, if we need to change anything here, like the ruby version, we only have to do it once and it'll be used by both files for future apps generated from the template.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there could be a lot if churn in this file. In theory you shouldn't really need Ruby to run the development.

The way Rails and Webpacker is currently written, you have to have the full Rails app installed with all of its gems in order to call the dev server, which is a wrapper around a JavaScript library.

This makes a lot of sense when you're running the server locally -- you already have Rails and everything installed, but it doesn't work as well with Docker Compose. I can imagine a future where this could be changed to a nodejs image that doesn't have to know anything about Rails, but Webpacker is something I'm not that familiar with. At this point I don't know enough to know whether having the files completely separate or not will end up being easier to manage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough. Let's keep this as-is for now, get some experience with using it on new projects/various updates, and figure out what feels best in terms of managing it over time.

What's here is simple (in a good way) and that may very well be best.

Add Docker and Docker Compose support by adding `Dockerfile`s and a
`docker-compose.yml`.

The Docker setup is a little different for React/Webpacker apps and
standard Rails app, so different files are copied depending on whether
run with `--webpack=react` or not.
Copy link
Contributor

@kevin-j-m kevin-j-m left a comment

Choose a reason for hiding this comment

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

Thank you for adding this in. This'll be super helpful for all of our future projects!

@zfletch zfletch merged commit 8de5d13 into master Oct 6, 2017
@zfletch zfletch deleted the docker branch October 6, 2017 19:11
@zfletch zfletch mentioned this pull request Oct 9, 2017
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants