X Tutup
The Wayback Machine - https://web.archive.org/web/20200917200646/https://github.com/nodejs/docker-node/pull/535
Skip to content

/ docker-node

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

Revert log level to npm's default level #535

Merged
merged 1 commit into from Oct 18, 2017
Merged

Revert log level to npm's default level #535

merged 1 commit into from Oct 18, 2017

Conversation

@LaurentGoderre
Copy link
Contributor

LaurentGoderre commented Oct 3, 2017

Fixes #528
@LaurentGoderre LaurentGoderre force-pushed the LaurentGoderre:loglevel branch from 08f8aa4 to f237a31 Oct 3, 2017
@nschonni
nschonni reviewed Oct 3, 2017
View changes
4.8/Dockerfile Outdated
@@ -40,7 +39,7 @@ RUN ARCH= && dpkgArch="$(dpkg --print-architecture)" \
&& rm "node-v$NODE_VERSION-linux-$ARCH.tar.xz" SHASUMS256.txt.asc SHASUMS256.txt \
&& ln -s /usr/local/bin/node /usr/local/bin/nodejs

ENV YARN_VERSION 0.24.4
ENV YARN_VERSION 1.1.0

This comment has been minimized.

Sign in to view
@nschonni

nschonni Oct 3, 2017 Member

Should the Yarn change with this?

This comment has been minimized.

Sign in to view
@chorrell

chorrell Oct 3, 2017 Contributor

Ah, yeah, that should not be updated (the update.sh script is probably to blame here :)

This comment has been minimized.

Sign in to view
@LaurentGoderre

LaurentGoderre Oct 3, 2017 Author Contributor

Yep it is....will fix that
@chorrell
chorrell suggested changes Oct 3, 2017
View changes
Copy link
Contributor

chorrell left a comment

Drop 7.10 to be in line with master
7.10/alpine/Dockerfile Outdated
@@ -0,0 +1,68 @@
FROM alpine:3.4

This comment has been minimized.

Sign in to view
@chorrell

chorrell Oct 3, 2017 Contributor

We dropped Node.js 7.10 awhile ago. I think your branch is out of sync with master.
@LaurentGoderre LaurentGoderre force-pushed the LaurentGoderre:loglevel branch 4 times, most recently from ba86e96 to a3fb16f Oct 3, 2017
@LaurentGoderre
Copy link
Contributor Author

LaurentGoderre commented Oct 3, 2017

@chorrell done! Sorry about that noise.
@chorrell
chorrell approved these changes Oct 3, 2017
View changes
@chorrell
Copy link
Contributor

chorrell commented Oct 3, 2017

Now worries. Looks good now!
@SimenB
SimenB requested changes Oct 4, 2017
View changes
Copy link
Member

SimenB left a comment •
edited

Needs to update the docs as well.

https://github.com/nodejs/docker-node#verbosity

(and a rebase is needed)
@LaurentGoderre
Copy link
Contributor Author

LaurentGoderre commented Oct 5, 2017

@SimenB @chorrell can we do the doc change in another PR. We need to discuss the wording etc.
@chorrell
Copy link
Contributor

chorrell commented Oct 5, 2017

Yeah, a follow-up PR for doc seems fine to me.
@LaurentGoderre LaurentGoderre force-pushed the LaurentGoderre:loglevel branch from a3fb16f to d851a14 Oct 5, 2017
@LaurentGoderre
Copy link
Contributor Author

LaurentGoderre commented Oct 5, 2017

Rebase done!
@SimenB
SimenB approved these changes Oct 5, 2017
View changes
Copy link
Member

SimenB left a comment

I don't understand why we should deploy something contradicting the documentation - there's no rush in landing this.

I won't block on it though
@chorrell
Copy link
Contributor

chorrell commented Oct 5, 2017

Well even if we land this, the images in the hub still have the npm logging noted in the current docs. I guess the docs will need to note that newer images will have default logging and older ones don’t.

I’m fine either way.
LaurentGoderre added a commit that referenced this pull request Oct 18, 2017
@LaurentGoderre
Updated documentation for new log level
9542e1a 
Completes #535
@LaurentGoderre
Copy link
Contributor Author

LaurentGoderre commented Oct 18, 2017

@chorrell can we merge this soon? Otherwise I will keep having to rebase
@LaurentGoderre
Revert log level to npm's default level
Loading status checks…
9861e1b 
Fixes #528
@LaurentGoderre LaurentGoderre force-pushed the LaurentGoderre:loglevel branch from d851a14 to 9861e1b Oct 18, 2017
@chorrell
Copy link
Contributor

chorrell commented Oct 18, 2017

Yes, but we still need an update for the docs to note that newer images will have default logging and older ones don’t. That update can either be in this PR or a new PR.
@LaurentGoderre LaurentGoderre mentioned this pull request Oct 18, 2017
Merged
@LaurentGoderre
Copy link
Contributor Author

LaurentGoderre commented Oct 18, 2017

@chorrell already done! #550
@chorrell
Copy link
Contributor

chorrell commented Oct 18, 2017

Perfect. Once Travis CI is green I'll merge and then do #550
@LaurentGoderre
Copy link
Contributor Author

LaurentGoderre commented Oct 18, 2017

We can discuss the docs wording further too
@chorrell chorrell merged commit c1f363a into nodejs:master Oct 18, 2017
1 check passed
1 check passed
Travis CI via nodejs-github-bot all tests passed
Details
@LaurentGoderre LaurentGoderre deleted the LaurentGoderre:loglevel branch Oct 18, 2017
LaurentGoderre added a commit that referenced this pull request Oct 20, 2017
@LaurentGoderre
Updated documentation for new log level
Loading status checks…
89e388f 
Completes #535
LaurentGoderre added a commit to LaurentGoderre/docker-node that referenced this pull request Oct 23, 2017
@LaurentGoderre
Updated documentation for new log level
6c84971 
Completes nodejs#535
SimenB added a commit that referenced this pull request Oct 24, 2017
@LaurentGoderre @SimenB
Updated documentation for new log level (#550)
5acf858 
Completes #535
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nschonni nschonni

@chorrell chorrell

@SimenB SimenB

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
@LaurentGoderre @chorrell @nschonni @SimenB
You can’t perform that action at this time.
X Tutup