Update Dockerizing guide to adhere to best practice #2293
Conversation
|
/ping @nodejs/docker |
|
|
||
| ```docker | ||
| CMD [ "npm", "start" ] | ||
| CMD [ "node", "server.js" ] |
There was a problem hiding this comment.
Nitpicking perhaps but maybe this should be script.js or index.js since not every node app is a server?
There was a problem hiding this comment.
Hi Laurent, thanks for the feedback. It should stay as-is because the guide, as it is written today, is a step-by-step guide that explicitly sets up a server and instructs the reader to create server.js. This PR is simply updating the pattern in the code without changing the scope of guide itself. Here's a link to the guide for reference.
| available from the [Docker Hub](https://hub.docker.com/): | ||
|
|
||
| ```docker | ||
| FROM node:8 | ||
| FROM node:10 |
There was a problem hiding this comment.
this could be node:lts instead of hard coding a version number, not sure if it should be recommended or not, though
There was a problem hiding this comment.
No it should not. It's problematic in the same way latest is.
There was a problem hiding this comment.
We're currently having this discussion in PR #2953. Can you elaborate on this? I clearly see there might be issues using the latest tag, but I don't understand in what way this might be problematic in the same way latest would be.
|
Is this good to go? 🚀 |
Hey y'all,
TL;DR - This PR updates the Dockerizing a Node.js web app guide to adhere to an official best practice of using
node server.jsinstead ofnpm startinside of a Docker container.Longer version:
The current Dockerizing a Node.js web app guide links to the official Node.js Docker Best Practices Guide that currently recommends avoiding the use of
npm startfor two reasons:SIGTERMandSIGINTto be received by the Node.js process instead of npm swallowing them.There are currently ~54,000 public
Dockerfiles on GitHub not following this practice.This PR updates the former guide to adhere to latter guide's best practice.
Additionally, the guide references "latest LTS" that points to
8, this updates that to10Additional Question:
How should I handle translations? The translation guide in this repo doesn't seem to apply to the guides pages, so I didn't update the corresponding paragraph of text for the locales that weren't already english.