Skip to content
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

Dockerfile for production #482

Merged
merged 2 commits into from
Jul 31, 2017
Merged

Dockerfile for production #482

merged 2 commits into from
Jul 31, 2017

Conversation

paroga
Copy link
Member

@paroga paroga commented Jul 26, 2017

No description provided.

Copy link

@dan-nl dan-nl left a comment

Choose a reason for hiding this comment

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

suggest creating 2 files for the moment; e.g. Dockerfile.prod and Dockerfile.dev and build the appropriate file. entrypoint file is a great idea.

@wvengen
Copy link
Member

wvengen commented Jul 31, 2017

It would be nice to generate a REVISION file (which is used here) when building a docker image. But this is a nice to have that can be added later.

@paroga
Copy link
Member Author

paroga commented Jul 31, 2017

I'd like to keep the main Dockerfile for production. We can add a Dockerfile.dev if we really want that later too.

Improving the .dockerignoresounds like a good idea, but is not mandatory to get it working again. There are additional changes too, which should be done to the Dockerfile to reduce the resulting size (unistall the dev-packages after bundler run).

I'll added generating the REVISION file.

# Run app and all commands as user 'app'. This avoids changing permissions
# for files in mounted volume.
RUN adduser --gecos GECOS --disabled-password --shell /bin/bash app
USER app
Copy link
Member

Choose a reason for hiding this comment

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

Wait a minute, does removing this mean that Foodsoft now runs as root? (Hope this is not a newbie Docker question.)

Copy link
Member Author

Choose a reason for hiding this comment

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

It's very uncommon to create a special user just for running the main entry point. Since it's a Docker container there are no "real root" rights in the container anyway and having an additional user does not offer a lot of additional security to the underlying system.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. Thanks for explaining!

@wvengen
Copy link
Member

wvengen commented Jul 31, 2017

Thanks! And looking good. Besides the run-as-question, ready to merge, I'd say.
I think we can still remove mailcatcher from docker-compose.yml for production.

Did you manage to re-setup the docker hub build?

@paroga
Copy link
Member Author

paroga commented Jul 31, 2017

Updating the docker-compose.yml file is a completely different task, since it's for development only. I personally prefere to get rid of it anyway and provide an example in the Wiki pages instead to show an example Docker setup for production. I also like to remove the doc folder and put that information in the wiki too. What do you think?

@paroga paroga merged commit 64bda5d into foodcoops:master Jul 31, 2017
@paroga paroga deleted the docker branch July 31, 2017 21:09
@wvengen
Copy link
Member

wvengen commented Jul 31, 2017

Great 👍

I know @dan-nl is working on a separate development and production docker setup, let's rework the rest with that.
Regarding the doc folder, I think it would be a good idea to have instructions on how to get it running in the source tree, so that it's there with the source code. Also, instructions may change over versions, keeping that in VCS allows one to have the right version (e.g. dependencies).

@wvengen wvengen added this to the 4.6 milestone Jul 31, 2017
@wvengen
Copy link
Member

wvengen commented Jul 31, 2017

One thing I didn't see yet: how about asset precompilation?

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