Description

fakewaffle

opened on Nov 17, 2015

Issue body actions

npm installs packages from a npm-shrinkwrap.json if one exists. They way in which the Dockerfile for onbuild is structured, npm will never install from npm-shrinkwrap.json.

https://github.com/nodejs/docker-node/blob/master/0.12/onbuild/Dockerfile#L6

Activity

1. pesho commented on Nov 17, 2015

   

   pesho

   on Nov 17, 2015

   Contributor

   More actions

   Indeed. @nodejs/docker does anyone remember why we copy only package.json first, before running npm install, and only then copy the rest?

   P.S. Ah yes, it's done in order to take advantage of the build-cache.
2. Starefossen commented on Nov 17, 2015

   

   Starefossen

   on Nov 17, 2015

   Member

   More actions

   This is similar to what is suggested here which says that there are no optional copy:

   ONBUILD COPY package.json npm-shrinkwrap.json /usr/src/app/
   

   This will fail for every project without an npm-shrinkwrap.json. This leaves us with three options:

   Option 1: Copy everything before installing:

   ONBUILD COPY . /usr/src/app/
   ONBUILD RUN npm install
   

   This will effectively disable Docker's build-cache and all modules will have to be re-installed for each change to the source code. The reason we use COPY package.json /usr/src/app/ is to only having to run npm install whenever package.json is changed.

   Option 2: Make an :onbuild-shrinkwrap Image

   ONBUILD COPY npm-shrinkwrap.json /usr/src/app/
   ONBUILD RUN npm install
   

   This will create more images that we will have to maintain and we already have quite a few:

   5 supported versions x 4 tags = 20 different images
   

   Option 3: Leave this up to those (few) who needs it

   The onbuild image variant is ment as a quickly get started with Dockerizing an application, and is not recommended by Docker Inc. for long term / production usage. There is an open issue #66 to discourage excessive use of the onbuild image.
3. Starefossen

   added

   bug

   question

   on Nov 17, 2015
4. retrohacker commented on Nov 18, 2015

   

   retrohacker

   on Nov 18, 2015

   Contributor

   More actions

   We do have a 4th option. Not saying that it is any good.

   Option 4: Use go's filepath.Match to add all json files.

   ONBUILD COPY *.json /usr/src/app/
   ONBUILD RUN npm install
   

   This has the disadvantage that all json files in the base directory of the project will be added, thus any change to any json file will trigger an npm install. It doesn't look like there is a way to do the equivalent of (package|npm-shrinkwrap).json.
5. retrohacker commented on Nov 18, 2015

   

   retrohacker

   on Nov 18, 2015

   Contributor

   More actions

   I'm in favor of Option 3.
6. fakewaffle commented on Nov 19, 2015

   

   fakewaffle

   on Nov 19, 2015

   Author

   More actions

   It's only awkward since one would assume that npm install would behave as expected - even though it doesn't since npm-shrinkwrap.json is never copied over.

   Option 3 is fine as long as there is that additional documentation.
7. chorrell commented on Nov 19, 2015

   

   chorrell

   on Nov 19, 2015

   Contributor

   More actions

   Yeah, Option 3 seems like the best way to go.
8. Starefossen

   added a commit that references this issue on Jan 13, 2016

   Onbuild does not install according to shrinkwrap

   This issue will close once commit ececf6c is merged into the 'master' branch.

   ececf6c
9. Starefossen

   closed this as completedin ececf6con Jan 14, 2016
10. openam commented on Aug 3, 2016

    

    openam

    on Aug 3, 2016

    More actions

    FWIW I think there is an option 5.

    Option 5: copy package.json and optionally the shrinkwrap

    ONBUILD COPY package.json *pm-shrinkwrap.json /usr/src/app/
    ONBUILD RUN npm install
    

    There would still a similar problem as with option 4, but the chance of having multiple files ending in pm-shrinkwrap.json is.

    The other thing I've noticed is if you have a .npmrc file it should be copied over too. Possibly add .npmr? to the list of things being copied over.

    Reacted by Muly Oved, Simen Bekkhus and André Werlang

Issue actions