-
Notifications
You must be signed in to change notification settings - Fork 3
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 basic Dockerfile #4
Conversation
6494858
to
b055f57
Compare
Dockerfile
Outdated
|
||
EXPOSE 3000 | ||
|
||
ENTRYPOINT [ "tini", "--", "node", "./build/main.js" ] |
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 noticed that, when using ENTRYPOINT [ "node", "./build/main.js" ]
, I couldn't send a SIGINT
to the node process (CTRL+C
) successfully.
Hence wrapping it in tini
which handles signals and reaps zombie processes (if there ever are any zombie processes in the future)
0628202
to
3734a43
Compare
Tilt gives you a web interface with logs and live reloading docker containers on code change (and more) |
27279a8
to
05c87e5
Compare
Hi @rblaine95. Great additions 👍 We just need to make the PR to the I'm afraid that if we merge this to |
Sure thing @jakubkoci, I've updated the base of the PR and will do a quick rebase. |
* Add basic Dockerfile * Add `.tool-versions` for `asdf`/`rtx` * Extend `.gitignore` * Add vscode extensions recommendations * Add `.prettierignore` to ignore the `build/` directory * Build `node_modules` early in Docker image for more efficient caching * Use `tini` as entrypoint to catch signals * I noticed when not using `tini`, I couldn't `SIGINT` the node process asdfg
* Add `NODE_ENV` to Dockerfile * Remove build from docker-compose as Tilt was already building * Change docker entrypoint and command * Re-organize dependencies in `package.json` * Many dev dependencies are actually just dependencies * Reduce Docker image size by splitting Dev/Prod
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 just added some notes but LGTM overall. Good job 👍
FROM docker.io/node:18-alpine | ||
|
||
USER 0 | ||
RUN apk add --update --no-cache tini curl |
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 added a commit with signal handlers to this PR which is a good practice in Node.js apps in general. We shouldn't need tini
with them.
I usually tend to lower deps in Docker as much as possible, but if you feel it's still beneficial having tini
I'm happy with that :)
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.
Tini is exceptionally small (±68KB)
The main benefits of tini is that:
- Reaping of zombie processes (I've seen some docker images that benefit from this)
- Signal handling
- Fully transparent
krallin/tini#8 has a good writeup on benefits of using it.
{ | ||
"files.trimFinalNewlines": true, | ||
"files.insertFinalNewline": true | ||
} |
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 should be solved by the Prettier formatter. You can check and update the format with the following commands:
yarn format:check
yarn format:update
Doesn't it work for you? I'll add some pre-push hooks that will check the format and linter rules before each push to the repo.
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.
Prettier does work for me, but I like to add these settings with vscode as it executes whenever you save the file
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.
WRT git hooks, I've worked with Husky in the past
} | ||
) | ||
|
||
docker_compose('docker-compose.yaml') |
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 haven't heard of the Tilt yet. Sounds cool 👍
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.
Think docker compose up
but on steroids
.tool-versions
forasdf
/rtx
.gitignore
docker-compose.yaml
and TiltfileI tried to do a multi-stage Docker build, but the build output depends on the
node_modules/
directory.Partially resolves #1