Skip to content

Added Dockerfile and docker-compose.yml to run the project on Docker environment #23

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kciejek
Copy link

@kciejek kciejek commented May 16, 2019

Run with: docker-compose up

FROM ruby:latest

COPY . .
RUN apt-get update && apt-get install -y redis-server && gem install bundler -v '2.0.1' && bundle update --bundler
Copy link

@gautamkrishnar gautamkrishnar May 17, 2019

Choose a reason for hiding this comment

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

@kciejek
Can we move this to separate layer. Copying entire app files before installing dependencies is not a good approach.
Please see the best practices: https://medium.com/the-code-review/docker-copy-dockerfile-best-practices-503704bee69f

Copy link

@gautamkrishnar gautamkrishnar May 17, 2019

Choose a reason for hiding this comment

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

Ideally this should be the Dockerfile:

FROM ruby:latest
WORKDIR /app
COPY Gemfile Gemfile.lock ./
RUN apt-get update && apt-get install -y redis-server && gem install bundler -v '2.0.1' && bundle update --bundler
COPY . .
CMD eval 'redis-server &' && bundle install && bundle exec puma

Copy link
Author

Choose a reason for hiding this comment

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

@gautamkrishnar Pefrect, thanks.

@gautamkrishnar
Copy link

There's already a similar PR ref #7

@jeffque jeffque mentioned this pull request Dec 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants