-
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
Multiple slim #850
Multiple slim #850
Conversation
a1de351
to
7a888d0
Compare
Hi @LaurentGoderre will this be fixed shortly, anything I can help with ? Thanks |
b29a16f
to
9fc3dab
Compare
The benefit of this is it creates the |
I'm not sure what caused this test to stall: https://travis-ci.com/nodejs/docker-node/jobs/143886958 Maybe we need to pin to a specific version on eclint? |
I restarted the test build. |
Maybe that failure was just an issue with the npm registry? Anyway, it's nice that we can restart builds now :-) |
9fc3dab
to
e58b0c0
Compare
@chorrell good catch! |
e58b0c0
to
204471d
Compare
56b248a
to
26fb32f
Compare
26fb32f
to
6d5a848
Compare
@chorrell @PeterDaveHello what do you think of my approach? |
6d5a848
to
06e58b1
Compare
I think I prefer separate templates. It will make the update script simpler and we can just delete the jessie template when we drop it. |
This bit from RUN set -ex; \
if ! command -v gpg > /dev/null; then \
apt-get update; \
apt-get install -y --no-install-recommends \
gnupg \
dirmngr \
; \
rm -rf /var/lib/apt/lists/*; \
fi |
@yosifkit wow that's even cleaner!!!! |
06e58b1
to
6dcc3b3
Compare
6dcc3b3
to
aeced95
Compare
LGTM. The only thing I’m not certain about are the architecture files |
This seems all correct to me. |
Sounds like we can merge this then do the Docker Hub PR for #921 I'm still sick, will most likely be taking the rest of the day off. If someone else can pick this up, that would be great. |
Yes, it's getting better.
Why do these -onboard images not support the other architectures? Are these still built from Jessie? (same comment for all of the other -onbuild images)
Do you know why this doesn't support the other architectures? Are these still built from Jessie? (same comment for all the other -slim variants)
You have 2 'slim' entries in this line.
|
@lag-linaro the two slim was fixed. As for what architecture are supported, I just preserved what was there before. |
Ah, so this change doesn't update the GitCommit tags. Is that already set as a separate piece of work? This library file is now referencing tags which are several months old. This misses out on the fact that the |
@lag-linaro perhaps but that should be looked at in a different PR I think |
I'm okay with that. In which case, if the duplicate LGTM |
@SimenB want to merge? |
I'm a bit hesitant to land this before the security releases on Tuesday... Thoughts on that? |
The security update will include stretch as the new default for the slim and base image. Landing this first would give people a Jessie option for slim. And we should do the pr for the recent update to v8.x.x before Tuesday |
Actually, I think you're right. This can wait until after the security update |
1fda969
to
fb54d5f
Compare
@chorrell @PeterDaveHello @SimenB rebased |
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'll still have node:10-slim
right?
Yep (via
|
No description provided.