-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
Update Dockerizing guide to adhere to best practice #2293
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,11 +80,11 @@ touch Dockerfile | |
Open the `Dockerfile` in your favorite text editor | ||
|
||
The first thing we need to do is define from what image we want to build from. | ||
Here we will use the latest LTS (long term support) version `8` of `node` | ||
Here we will use the latest LTS (long term support) version `10` of `node` | ||
available from the [Docker Hub](https://hub.docker.com/): | ||
|
||
```docker | ||
FROM node:8 | ||
FROM node:10 | ||
``` | ||
|
||
Next we create a directory to hold the application code inside the image, this | ||
|
@@ -134,17 +134,16 @@ EXPOSE 8080 | |
``` | ||
|
||
Last but not least, define the command to run your app using `CMD` which defines | ||
your runtime. Here we will use the basic `npm start` which will run | ||
`node server.js` to start your server: | ||
your runtime. Here we will use `node server.js` to start your server: | ||
|
||
```docker | ||
CMD [ "npm", "start" ] | ||
CMD [ "node", "server.js" ] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpicking perhaps but maybe this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
``` | ||
|
||
Your `Dockerfile` should now look like this: | ||
|
||
```docker | ||
FROM node:8 | ||
FROM node:10 | ||
|
||
# Create app directory | ||
WORKDIR /usr/src/app | ||
|
@@ -162,7 +161,7 @@ RUN npm install | |
COPY . . | ||
|
||
EXPOSE 8080 | ||
CMD [ "npm", "start" ] | ||
CMD [ "node", "server.js" ] | ||
``` | ||
|
||
## .dockerignore file | ||
|
@@ -195,7 +194,7 @@ $ docker images | |
|
||
# Example | ||
REPOSITORY TAG ID CREATED | ||
node 8 1934b0b038d1 5 days ago | ||
node 10 1934b0b038d1 5 days ago | ||
<your username>/node-web-app latest d64d3505b0d2 1 minute ago | ||
``` | ||
|
||
|
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 could be
node:lts
instead of hard coding a version number, not sure if it should be recommended or not, thoughThere 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.
No it should not. It's problematic in the same way
latest
is.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.
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 waylatest
would be.