-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Proposal to add Yarn to the existing images #337
Conversation
In general I'm pretty OK with this :) The one thing I would change is how yarn versions are handled. Currently the
I'm more inclined to be more conservative here and pin things to SemVer major. I'm assuming that at some point when yarn hits v1 there will be breaking changes. So something like |
Is there a use case where |
@daveisfera a lot of package install scripts depend on @chorrell I agree in general about SemVer-major pinning. We could probably use the NPM registry to resolve a SemVer selector to a specific version, with a query like https://registry.yarnpkg.com/yarn/0.20.x. There seem to be a few issues at the moment though:
We should probably ask the Yarn maintainers for advice as well. /cc @Daniel15 |
If you do want to pin the version number, the best solution right now is probably to hard-code it into your script or Docker file and manually update it for new releases. The version numbers are moving quickly so I don't know whether it's worth pinning it while we're still at 0.x releases. Maybe once 1.x comes out? We have one endpoint https://yarnpkg.com/latest-version to get the latest version number, perhaps we could add
Yeah I'm not quite sure what happened there. cc @bestander |
1+ to add |
Updated PR according to comments from @Daniel15 @chorrell @Starefossen in light of the info provided by @Daniel15 I think the best course for now is to keep using the latest version recommended by the Yarn team at https://yarnpkg.com/latest-version. After Yarn v1 is released we may switch to semver-major pinning. But it wouldn't work very well before that. |
Looks good to me. I'm good to merge this later today if no one objects :) |
@Starefossen @chorrell thanks for reviewing! I just added the auto-generated dockerfiles to the PR as well, to try and get a Travis result before merging (although it was timing out even before that). |
@pesho Cool. Are the timeouts an issue with nodejs-github-bot? |
@chorrell It looks like Travis is building Node.js from source? https://travis-ci.org/nodejs/docker-node/jobs/206192823 Not sure whats up with that... |
That is the Alpine images building from source... looks like they take a couple of minutes to complete |
@Starefossen Yep. But typically the build actually succeeds in Travis but the status on the PR never gets updated and shows pending. I was wondering if nodejs-github-bot was timing out after a set time? I guess longer term we need musl/Alpine binaries :) |
Actually that's what probably was happening. Not "timing out" as I said. |
Looks like we hit the same ha.pool.sks-keyservers.net timeout issue that's happening in #340 :( |
This update adds the latest version of Yarn (0.21.3) to all the Node.js Docker images and variants: - nodejs/docker-node#337 - https://yarnpkg.com/
This is awesome, also it broke my build on |
@terribleplan - Do you get an error? I would have thought that'd just override the |
|
Yes, it broke my builds as well, but once I removed the installation of yarn it worked just fine. |
Ah, good catch. I think that'd be specific to installing Yarn via npm, which is discouraged in the Yarn documentation so it shouldn't be commonly used. Anyone installing Yarn via the Linux packages would still be fine, they'll just end up with two versions of Yarn on the system. |
@Daniel15 Yeah, but if you are a nvm user, you just don't do apt-get install yarn. It feels very very wrong. |
This issue is specifically about Docker. Why would you use nvm in Docker? If you want to use multiple different Node.js versions, you'd probably have multiple different Docker images rather than using nvm within a Docker image. apt-get works fine if you're using nvm, you just need to use the |
I know. But why not do it in the docker container with |
I'd like to see this incorporated in the |
@williamboman the |
I don't think this was a good decision - yarn doesn't ship with node, so it shouldn't ship with an official node docker image. Conversely, if it is shipped with something official, it should ship with everything. This should be reverted. |
It adds 5.5 MB to the alpine images so less than 10% increase in the alpine image size (and significantly smaller for the other base images) and is extremely convenient for those using |
It may be convenient, but node does not ship yarn, and thus the official node docker image should not ship it. If the value is there, then node itself should ship with yarn before this docker image does. |
|
@daveisfera those are part of the base image, not something added by the node image. In the Alpine image where they are added on top of the base image, they are removed after the build. |
IMO there should be one variant with both npm and Yarn (for dev/builds), and one variant with neither npm nor Yarn (for production). You shouldn't need a package manager in your production Docker image. This would match what other ecosystems do - for example C# /ASP.NET Core has one Docker image with the SDK for building apps, and then a separate image with just the runtime for production. ASP.NET does this using a Docker multi-stage build: https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/docker/building-net-docker-images?view=aspnetcore-3.0 |
Disclaimer: This is a proposal PR which requires discussion. Please do not merge unless consensus has been established.
This PR adds Yarn as an alternative package manager to the existing images. It's added as a separate layer and the size increase is small (up to ~3.5 MB currently; for example the
7.6.0-alpine
tag increases in size from 56.2 MB to 59.7 MB). Theupdate.sh
script is updated to automatically detect the latest stable Yarn releaseusing the GitHub API.Adding Yarn has been by far the most upvoted request in our issue tracker (#243).
Notes:
npm install
) and the size increase is much smaller..js
file, GnuPG-check its signature, put it in/usr/local/bin
andchmod +x
it. This was chosen because of the small size (~3.2 MB), in contrast to all other available methods (tarball, DEB package, NPM) which are currently much larger when unpacked (at least ~14 MB). The-legacy
bundle variant is used, because the modern one does not support Node 4.x (which we do support).The GitHub APIhttps://yarnpkg.com/latest-version is used by theupdate.sh
script to automatically detect the latest release marked as stable by the Yarn team. Unstable pre-releases are not considered.gnupg
andcurl
are installed/uninstalled twice. This shouldn't be a big issue, and it helps that the installation method is mostly the same across all image variants../update.sh
invocation away.Related: #243 #260 yarnpkg/yarn#2721 nodejs/node#9161