-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Improve the Docker image #403
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
Conversation
e8e53ae to
d84accc
Compare
entrypoint.sh
Outdated
| @@ -0,0 +1,8 @@ | |||
| #! /bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be inlined into Dockerfile or something? Seems a bit messy to have a file in root that just runs npm install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can run npm install "on building" of image by using the node:6.9.2-onbuild image instead.
I agree that the entrypoint.sh in the root of the project kinda bloats it. I somehow missed it before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This only helps during development - @parisk the on-build image flavor will still not work since we use volumes to mount the code inside the container.
We can just omit this and improve our docs - users should do docker-compose run --rm web npm install && docker-compose up the first time they want to spin up the project.
I'm 👍 with that, just let me know if you're Ok with this approach and I'll do a quick update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it work if we inlined the ENTRYPOINT into the Dockerfile?
ENTRYPOINT "[ ! -d node_modules ] && npm install; $@"Or something like the above?
d84accc to
5fefe2a
Compare
1. Bump Node version to 6.9 - the latest LTS 2. Include the `cpio` binary, used during building 3. Ignore node_modules and .git directories for faster builds 4. Run the tests and build during the image build, to make sure the image is not built if these break 5. Make npm run dev the default command 6. Add an entrypoint to automatically install Node modules if they do nott exist Signed-off-by: Antonis Kalipetis <[email protected]>
Signed-off-by: Antonis Kalipetis <[email protected]>
5fefe2a to
b2e257b
Compare
b3b49d0 to
a4c958c
Compare
|
Rebased with master and fixes an issue with EOL characters. |
|
LGTM, I'm no authority on docker though 😄 |
This allows for reproducible builds. Also, fix Dockerfile to first build and then run tests and a typo in the entrypoint. Thanks @BenHall for reporting the issue with the Docker build!
parisk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
cpiobinary, used during buildingSigned-off-by: Antonis Kalipetis [email protected]
Based on #402 - since it uses the
npm run devscript.