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

Replace onbuild with alpine in Dockerfile example #471

Closed
wants to merge 2 commits into from

Conversation

chchrist
Copy link

Since onbuild is deprecated it should not be in the Dockerfile example.

Replacing onbuild with alpine from example as onbuild is deprecated
remove onbuild from example
@chorrell
Copy link
Contributor

I don't agree with choosing the alpine variant for the example. The reasoning is, more or less, spell out in https://github.com/nodejs/docker-node#nodealpine

It would also be better to encourage people to pin to a major or major-minor version, such as node:8 or node:8.2. Blindly pulling in the latest node image is not really a good idea.

Also, most of the example text for that section assumes the onbuild variant is being used so this would be inaccurate:

The image assumes that your application has a file named package.json listing its dependencies and defining its start script.

It would probably be better to replace the whole Create a Dockerfile in your Node.js app project section with a sane example that does similar things to what onbuild used to do.

@chorrell
Copy link
Contributor

And we'd want to make sure any doc changes also land on https://hub.docker.com/_/node/ via a PR to https://github.com/docker-library/docs

@SimenB
Copy link
Member

SimenB commented Jul 21, 2017

I don't agree with choosing the alpine variant for the example. The reasoning is, more or less, spell out in https://github.com/nodejs/docker-node#nodealpine

I have yet to hit anything using alpine that behaved differently. Why shouldn't the smallest image be recommended?

I suppose if it's aimed at beginners in Docker, but then I'd rather point to some other documentation (and document the caveats that come with using alpine, ref #384).

It would also be better to encourage people to pin to a major or major-minor version, such as node:8 or node:8.2. Blindly pulling in the latest node image is not really a good idea.

Agreed.

@Starefossen
Copy link
Member

Starefossen commented Jul 22, 2017

I suggest changing the example more in line of this one:

FROM node:8

WORKDIR /usr/src/app

ADD package.json /usr/src/app
RUN npm install
ADD . /usr/src/app

CMD ["node" "index.js"]

@Starefossen
Copy link
Member

Starefossen commented Jul 22, 2017

It would also be better to encourage people to pin to a major or major-minor version, such as node:8 or node:8.2. Blindly pulling in the latest node image is not really a good idea.

I do not agree with this statement. It is the Node.js Foundation's policy to strictly follow Semantic Versioning which means that feature and patch versions are safe with regards to incompatible or breaking changes.

Keep in mind that minor versions no longer receives patches when the next minor version is released.

@pesho
Copy link
Contributor

pesho commented Jul 22, 2017

feature and patch versions are safe with regards to incompatible or breaking changes

Usually yes, but there have been breakages in all components that we ship (Node, NPM, Yarn). I personally wouldn't base a production image on anything else than a strictly specified version (MAJOR.MINOR.PATCH).

@llb4ll
Copy link
Contributor

llb4ll commented Jul 31, 2017

How about using latest with a comment to change it to a specific major.minor version: #478

@llb4ll
Copy link
Contributor

llb4ll commented Aug 1, 2017

Pull request that specifies the current LTS as base image in the Dockerfile example: #482

@SimenB
Copy link
Member

SimenB commented Aug 1, 2017

Closed in #482 (although we should expand on the instructions, as mentioned there. PR welcome!)

@SimenB SimenB closed this Aug 1, 2017
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.

None yet

6 participants