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

Add git to the alpine image #367

Closed
wants to merge 1 commit into from

Conversation

LaurentGoderre
Copy link
Member

Fixes #366

@Starefossen
Copy link
Member

  1. What is the size implication on the final image?
  2. What is the practice for other alpine images regarding including git?

@chorrell
Copy link
Contributor

What does this add to the image size?

@deitch
Copy link

deitch commented Mar 28, 2017

What is the practice for other alpine images regarding including git?

Depends if it needs it. In the case of npm, I would argue it needs it, since npm includes support for git URLs as dependencies. See the related issue I opened #366

size

17.6 MB. I pulled 7.4-alpine, then made a simple Dockerfile extending it with just git:

node       7.4-alpine                      356564976a67        2 months ago        55.2 MB
node       7.4-with-git-alpine             3ecd850e6820        About a minute ago   72.8 MB

Now one could argue that the really right solution is for npm to access git natively, not call out to the external binary, but until that happens...

@Starefossen
Copy link
Member

17.6 MB is way to much for what is supposed to be the smallest Docker image possible.

@pesho
Copy link
Contributor

pesho commented Mar 28, 2017

17.6 MB is way to much for what is supposed to be the smallest Docker image possible.

I agree. I'd prefer documenting this as a common usecase instead of adding it to the image.

@deitch
Copy link

deitch commented Mar 28, 2017

I'd prefer documenting this as a common usecase instead of adding it to the image.

I am somewhat torn. Adding 1/3 to the image size is a lot. On the other hand, without it, a well-documented and heavily-used official standard npm use case is broken.

the smallest Docker image possible

If it doesn't support npm's documented features, is it the smallest possible? I don't know.

Coming back to where this started, I think the correct thing is fixing npm to support git natively (is npm itself in JS? Most langs have some form of native git support), but until then...

@chorrell
Copy link
Contributor

Yeah, that's a significant increase in size. 👎

The main use case for the Alpine image is to provide something really small that users can build upon. So for me the working assumption is yeah, you'll need to install things like git. The same applies to the slim variants.

A lot of npm modules will fail to install because the build-base package is not installed (see for instance #355). If we gone down this path then we'll end up with another bloated image like noda:latest (yay buildpack-deps:jessie).

I think we need to document the above (that it's a minimal image and you will need git and build-base etc.) and also put it in some GitHuB issues and pr templates.

And really we need to probably document some very strict criteria for adding things to the slim and alpine images.

@deitch
Copy link

deitch commented Mar 28, 2017

And really we need to probably document some very strict criteria for adding things to the slim and alpine images.

Yeah. The docs probably should say something like, "contains only node and npm, period. If you try to install an npm package with additional dependencies like git or python or... it will fail. Extend the image and make your own with those dependencies installed."

@yosifkit
Copy link
Contributor

You mean like the description on the Docker Hub:

To minimize image size, it's uncommon for additional related tools (such as git or bash) to be included in Alpine-based images. Using this image as a base, add the things you need in your own Dockerfile (see the alpine image description for examples of how to install packages if you are unfamiliar).
- https://github.com/docker-library/docs/tree/f9cee39d06180d293190189d4455a2613ce11213/node#nodealpine

@chorrell
Copy link
Contributor

Ha!

@chorrell
Copy link
Contributor

We should probably update the README to also mention build-base.

@deitch
Copy link

deitch commented Mar 29, 2017

You mean like the description on the Docker Hub:

it's uncommon for additional related tools (such as git or bash) to be included

"some stuff like xx is not included" is not quite the same as "the following basic npm features require additional installs: ____"

@LaurentGoderre
Copy link
Member Author

LaurentGoderre commented Mar 29, 2017

I'm really on the fence with this one....I want to keep the size to a minimum but the Node package should work out of the box and since npm is packaged with it, that also should work. That part of the docs, I interpret as saying that alpine shouldn't include "helpful tools" such as bash which is easier to use than the builtin sh. However, you could argue that in this case, git is a dependency of npm.

@deitch
Copy link

deitch commented Mar 29, 2017

@LaurentGoderre : you said that much more eloquently than I did! :-)

Any idea why:

a- git is so darn big? checking on my Mac, it is all compiled binaries
b- why npm doesn't have a minimal subset? After all, it only needs to do some basic git pull stuff.

@Starefossen
Copy link
Member

One could also argue that npm is strictly not a part of node, providing it with the official image is only done as a service to our users (it is provided with the official nodejs distribution).

@deitch
Copy link

deitch commented Mar 29, 2017

One could also argue that npm is strictly not a part of node

I think it is inseparable at this point. In the early days, when they were separate installs, maybe. At this point, it is a single install, and just about everything is packaged via npm and required to run almost any programs.

@Starefossen Starefossen requested review from Starefossen and removed request for Starefossen March 29, 2017 12:35
@LaurentGoderre
Copy link
Member Author

@deitch I think a big chunk of the size is merge tools

@pesho
Copy link
Contributor

pesho commented Mar 29, 2017

However, you could argue that in this case, git is a dependency of npm.

More precisely, git is an optional dependency of npm. Many applications (probably most) don't really need it.

I admit though that the lines are blurry especially after we already included Yarn in the Alpine image. Still, I think there were two factors which made it a bit more suitable for inclusion than git:

  • The size increase due to Yarn was considerably smaller.
  • Yarn was considerably harder to install in an optimal way than git.

I still think the best approach would be to document the most common use cases. Need to install modules from git? Do this. Need native modules? Do that. Need Flowtype? Do that.

@LaurentGoderre
Copy link
Member Author

Perhaps we'd need a whole separate document dedicated to alpine, we could document ndoe-gyp, this, FFlow-type etc..

@deitch
Copy link

deitch commented Mar 29, 2017

npm seems to centralize git actions here https://github.com/npm/npm/blob/latest/lib/utils/git.js which just spawns the expected-to-exist git binary. Is there a native JS git client port?

@EnzoMartin
Copy link

EnzoMartin commented Mar 30, 2017

We ran into this issue for building, and ended up splitting our image so we have a build image and a runtime image because we wanted to keep our runtime alpine image as tiny as possible.

The build image has git, build tools, and runs npm install etc. to leverage docker layer caching to reduce build times as much as possible. We then copy out the node_modules from the build image and push it into the runtime image, which has no NPM operations and minimal layers.

Simplified version of our images: https://gist.github.com/EnzoMartin/6b40242e38b6ae232c9d6c990871b454

@deitch
Copy link

deitch commented Mar 30, 2017

@EnzoMartin funny, that is precisely what I do with go, java, and any other lang that has a compile-time environment. I never really thought of node in that light.

I guess you could argue that a truly lightweight node image would have just node, not even npm, as npm is part of the build-time. But I don't see the world going that way; try to remove npm from this image and see what happens. :-)

@EnzoMartin
Copy link

EnzoMartin commented Mar 30, 2017

Agreed. I would not want to see git or anything more added to the image. Since if we argue that git is needed, then the next argument will be that it should support building C++ dependencies, which requires a LOT more OS dependencies.

I would actually love to see npm removed from it as it's not needed for running a node service and an alpine image should not be responsible for building out of the box as it runs counterpoint to it's reason for existing IMO (having the tiniest possible image).

@LaurentGoderre
Copy link
Member Author

So can we close this?

@LaurentGoderre
Copy link
Member Author

@EnzoMartin thanks so much for sharing that build thing, I had never seen this in Docker and I can find quite a bit of use for that?

@EnzoMartin
Copy link

EnzoMartin commented Mar 30, 2017

You'll find similar set up for using languages that generate an artifact, like for Java or Go services that have an executable file that gets compiled. You don't want the runtime to be responsible for building as that generates a lot of layers and dependencies that are simply not needed for the actual runtime and breaks the docker layer caching that would otherwise speed up the container build times.

It's just not something that's been documented/talked about much in the Node ecosystem yet

@chorrell chorrell mentioned this pull request Mar 30, 2017
@EnzoMartin
Copy link

These upcoming Docker changes would make most of this obsolete also: http://blog.alexellis.io/mutli-stage-docker-builds/

@macta
Copy link

macta commented May 16, 2017

I've found this issue after discovering that my build was failing on gitlab using an alpine node image due to a valid git+ package specification. I wouldn't expect a node image to not support this, so it was very surprising. I hadn't initially spotted this as Gitlab was caching my node_modules directory.

I agree with the proposer, if it doesn't work with valid package.json files, then its not really a node image.

@EnzoMartin
Copy link

Then you should use the full NodeJS image, not the alpine image.

@LaurentGoderre
Copy link
Member Author

All of this might get resolved with the new builder pattern that Docker released. We might be able to have a build and a production image.

@james-gardner
Copy link

What base images would we use for a multi stage build?

@LaurentGoderre
Copy link
Member Author

@james you can use any of them really. Alpine is the smallest but requires more work sometimes to setup

@SalathielGenese
Copy link

I inclined for RUN apk --no-cache add git in my Dockerfile

@SalathielGenese
Copy link

Some are suggesting multistage build is a solution but I'm not sure how a requirement for when npm install actually run will turn into a stage approach. I'm really unable to figure out how.
Would you elaborate, please ?

@LaurentGoderre
Copy link
Member Author

@SalathielGenese you would run npm in the first stage (where git would be installed) and copy the installed modules to the second stage (without git)

@SalathielGenese
Copy link

SalathielGenese commented Aug 6, 2018

Malin le gringo,
Très très malin,

Thanks @LaurentGoderre, much thanks !!!

@AlekKras
Copy link

AlekKras commented Oct 9, 2018

Just add

RUN apk update && \
    apk upgrade && \
    apk add git

In your Dockerfile and you should be good

@SalathielGenese
Copy link

SalathielGenese commented Oct 17, 2018

It is important to keep the image as slim as possible (even in multiple stage build)... So am I using --no-cache.

RUN apk --no-cache add git

May I note here that what I first missed is that git was needed by npm during package installation.

@LaurentGoderre LaurentGoderre deleted the add-alpine-git branch October 18, 2018 15:02
Wilfred added a commit to Wilfred/wikig that referenced this pull request Dec 8, 2019
express-cache-middleware depends on express-mung, which is pulled
directly from git. See nodejs/docker-node#367
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.