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.

Sign up for GitHub

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

Jump to bottom

Add Node.js v8.0.0 #411

Merged
merged 1 commit into from
May 31, 2017
Merged

Add Node.js v8.0.0 #411

merged 1 commit into from
May 31, 2017
+7 −7

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 30, 2017

#410 with node 7 removed. Unsure which of these (if any) you want 😀

@SimenB SimenB mentioned this pull request May 30, 2017
Closed
@chorrell
Copy link
Contributor

The Docker files should be in a 8.0/ directory.

@SimenB
Copy link
Member Author

SimenB commented May 30, 2017

The Docker files should be in a 8.0/ directory.

They are.
image

@chorrell
Copy link
Contributor

Ha! I missed that.

In that case LGTM

@SimenB
Copy link
Member Author

SimenB commented May 30, 2017

You should enable cancelling old builds on new pushes on travis. I force pushed a bit, and the queue is real right now 😭

https://blog.travis-ci.com/2017-03-22-introducing-auto-cancellation

@chorrell
Copy link
Contributor

I would if I had the access to do that ;-)

Someone from the build group should be able to enable that for us.

@SimenB
Copy link
Member Author

SimenB commented May 30, 2017

Should I close #410?

@chorrell
Copy link
Contributor

Sure.

@pouwerkerk pouwerkerk mentioned this pull request May 30, 2017
Closed
Starefossen

This comment was marked as off-topic.

Sign in to view

@pesho
Copy link
Contributor

pesho commented May 30, 2017

Wait, is 7.x a completely abandoned branch now?

@SimenB
Copy link
Member Author

SimenB commented May 30, 2017 •
edited
Loading

Wait, is 7.x a completely abandoned branch now?

https://github.com/nodejs/LTS/blob/4af640885c5888894348ace2b15942e8989494e4/README.md

An odd-numbered major release will cease to be actively updated when the subsequent even-numbered major release is cut.

Not really sure if that means "completely abandoned", though...

Btw, can one of you cancel some of the builds here? https://travis-ci.org/nodejs/docker-node/pull_requests (bonus is activating auto cancellation 🙂)

@pesho
Copy link
Contributor

pesho commented May 30, 2017

@SimenB thanks, it is indeed abandoned then.

I think the Yarn version should be updated too in this PR, according to the (not yet officially accepted) policy in #393

@chorrell
Copy link
Contributor

Were we also going to update Alpine for 8.x or was there a different plan for that? Either way, that can probably be handled separately.

@pesho
Copy link
Contributor

pesho commented May 30, 2017

Yes, Alpine should also be updated (probably to 3.6) according to the discussion in #399

@SimenB
Copy link
Member Author

SimenB commented May 30, 2017

I think the Yarn version should be updated too in this PR

I chose not to update it to keep it consistent between images. That doesn't really make sense though, the npm version is already different. Want me to update it?

@Starefossen
Copy link
Member

Starefossen commented May 30, 2017 •
edited
Loading

I'd vote we merge this PR as is and make a separate ones for upgrade of Alpine and Yarn.

@pesho
Copy link
Contributor

pesho commented May 30, 2017

I chose not to update it to keep it consistent between images.

Running ./update.sh 8.0 should "Do the right thing":tm: and only update it for 8.x.

@pesho
Copy link
Contributor

pesho commented May 30, 2017

I'm ok with doing Yarn and Alpine separately in this case, but in general think they should be handled in a single PR, atomically.

@SimenB
Copy link
Member Author

SimenB commented May 30, 2017

CI is green, but it doesn't seem connected to the latest commit in this PR... https://travis-ci.org/nodejs/docker-node/builds/237656473

@pouwerkerk
Copy link

@SimenB looks like the commit that CI built is the merge commit: 8e936c8

@olalonde
Copy link

💤

@xzyfer xzyfer mentioned this pull request May 31, 2017
Closed
6 tasks
@SimenB
Copy link
Member Author

SimenB commented May 31, 2017 •
edited
Loading

CI is confused, I force pushed the same commit now.

EDIT: Didn't help at all... I have no idea, just merge it? The same diff was green even if github/travis is a bit confused

@lostintangent lostintangent mentioned this pull request May 31, 2017
Merged
@Starefossen
Copy link
Member

I checked locally, it's all good. Merging.

@Starefossen Starefossen merged commit 82c21d3 into nodejs:master May 31, 2017
@SimenB SimenB deleted the node-8-del-7 branch May 31, 2017 06:04
@SimenB
Copy link
Member Author

SimenB commented May 31, 2017 •
edited
Loading

Great! Will this be pushed to the docker registry, or should yarn and alpine be updated first?

EDIT: PR for the former: #412, latter: #413

@Starefossen
Copy link
Member

Starefossen commented May 31, 2017 •
edited
Loading

Thanks a lot @SimenB. I'll merge them as soon as the builds finish. I have prepared a PR to Docker Hub. Hopefully will get this one out buy by the end of the day.

This was referenced May 31, 2017
Merged
Merged
tianon pushed a commit to docker-library/official-images that referenced this pull request May 31, 2017
Upgrade Node to 8.0.0
34aeba6 
PR-URL #2997

Related: nodejs/node#12220 nodejs/docker-node#411 nodejs/docker-node#412 nodejs/docker-node#113

Signed-off-by: Hans Kristian Flaatten <hans@starefossen.com>
@dy-dx dy-dx mentioned this pull request Jul 11, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Starefossen Starefossen Starefossen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@SimenB @chorrell @pesho @Starefossen @pouwerkerk @olalonde @analytically @LaurentGoderre