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

Add Dockerfile #291

Merged
merged 5 commits into from
May 29, 2020
Merged

Conversation

groodt
Copy link
Contributor

@groodt groodt commented May 18, 2020

Adds a Dockerfile for running commuter in a production environment

There is an old PR still open for adding simple Docker support, but that appears more suitable for local development: #188

This PR is intended to build a Docker container suitable for publishing to Dockerhub (or other registry) and be run in a production environment such as Kubernetes.

It can be tested locally as follows:

checkout this branch

docker build --tag commuter:latest .

docker run \
--publish 4000:4000 \
--mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
--env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
commuter:latest

or (use my prebuilt image)

checkout commuter master branch

docker run \
--publish 4000:4000 \
--mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
--env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
groodt/commuter:latest

@groodt groodt marked this pull request as ready for review May 18, 2020 05:05
Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

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

Looks good overall! Can you add some docs on this in the README?

##################################
# Build
##################################
FROM node:14 as build
Copy link
Member

Choose a reason for hiding this comment

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

Curious: what does the as build syntax do here? I have some familiarity with Docker but haven't seen this before.

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 is for multi-stage builds. See: https://docs.docker.com/develop/develop-images/multistage-build/

These are useful for controlling the number of layers in the final image and also for controlling the contents of the image itself. Most of the time, the tools necessary at build time are not necessary at runtime. Using a multi-stage build allows us to leave the compiler toolchains behind and not include them in the final image.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I suspected as much. Thanks for the speedy response!

Dockerfile Show resolved Hide resolved
@mrtns
Copy link

mrtns commented May 22, 2020

@groodt Thank you for this!

I'm running into an issue configuring a non-default (non-4000) port.

For example, using port 4200, passing PORT does not seem to work:

docker run \
--publish 4200:4200 \
--mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
--env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
--env PORT=4200 \
groodt/commuter:latest

nor does passing COMMUTER_PORT:

docker run \
--publish 4200:4200 \
--mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
--env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
--env COMMUTER_PORT=4200 \
groodt/commuter:latest

but when I pass them in both it works:

docker run \
--publish 4200:4200 \
--mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
--env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
--env PORT=4200 \
--env COMMUTER_PORT=4200 \
groodt/commuter:latest

@groodt
Copy link
Contributor Author

groodt commented May 22, 2020

@mrtns My pleasure. I'm actually running this in production at the moment and it seems to work really well!

I've found the issue that requires 2 ports to be set:

This line:

config.port = process.env.PORT || process.env.COMMUTER_PORT || 4000;

combined with this line:
https://github.com/nteract/commuter/pull/291/files#diff-3254677a7917c6c01f55212f86c57fbfR77

will cause express to be running on port 4000.

I'll update this PR to not set the PORT.

I'll also raise a PR to simplify the PORT variable logic to a single variable.

@groodt
Copy link
Contributor Author

groodt commented May 24, 2020

@mrtns This PR now works when only setting COMMUTER_PORT. PORT should remain unset.

docker pull groodt/commuter:latest
docker run \
  --publish 4200:4200 \
  --mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
  --env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
  --env COMMUTER_PORT=4200 \
  groodt/commuter:latest

Or build the image yourself:

docker build --tag commuter:latest .

docker run \
  --publish 4200:4200 \
  --mount type=bind,source=/home/username/work/commuter/examples,target=/examples \
  --env COMMUTER_LOCAL_STORAGE_BASEDIRECTORY=/examples \
  --env COMMUTER_PORT=4200 \
  commuter:latest

@kokes
Copy link
Contributor

kokes commented May 26, 2020

Is there some reason for installing tini explicitly? Docker has had it for a few years now (PR 1, PR 2), so it's only a matter of --init.

@groodt
Copy link
Contributor Author

groodt commented May 26, 2020

Is there some reason for installing tini explicitly? Docker has had it for a few years now (PR 1, PR 2), so it's only a matter of --init.

For running in Kubernetes mainly.

@groodt
Copy link
Contributor Author

groodt commented May 29, 2020

@captainsafia I've added some notes to the README.md and Dockerfile for development purposes (if that's useful).

Do I need to do anything to merge this, or is this not going to be merged?

@captainsafia
Copy link
Member

It's good to merge. I was just waiting for the README updates.

@captainsafia captainsafia merged commit 3a7db09 into nteract:master May 29, 2020
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.

4 participants