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

WIP: Add templates for generating the variants #144

Merged
merged 1 commit into from
Apr 18, 2016

Conversation

LaurentGoderre
Copy link
Member

No description provided.

@LaurentGoderre
Copy link
Member Author

Here is the diff the the dockerfile generated by this
https://gist.github.com/LaurentGoderre/fbf5f44e1310a127ae6638e1d7548188

gpg --keyserver ha.pool.sks-keyservers.net --recv-keys "$key"; \
done

ENV NPM_CONFIG_LOGLEVEL info

This comment was marked as off-topic.

This comment was marked as off-topic.

@chorrell
Copy link
Contributor

I think this PR should be renamed since it's about adding templates for the various existing variants and not does not add an Alpine variant (which presumably will be a separate PR :)

@LaurentGoderre LaurentGoderre changed the title WIP: Add the alpine variant WIP: Add templates for generasting the variants Apr 11, 2016
@@ -0,0 +1,33 @@
FROM buildpack-deps:jessie-curl

This comment was marked as off-topic.

@chorrell
Copy link
Contributor

So I think it's time for me to review the 0.10 and 0.12 image with respect to how the slim variant is built an switch to the method used by 4.x and 5.x. That should help in terms if templating the updates.

@LaurentGoderre
Copy link
Member Author

I'm a bit confused what is needed to get this merge, especially since it's pending other decision. Should I separate the commit into the templates and the script so we could merge the template, keep working on it then update scripts when the template is finalized?

@chorrell
Copy link
Contributor

chorrell commented Apr 13, 2016

A couple of things need to happen outside of this PR:

  • A resolution to Excessive npm log output in 4.x #57. We need to either drop NPM_CONFIG_LOGLEVEL info or use it consistently with all the images. I get the sense we're leaning towards dropping it (edit: actually, I think most were for it as long as it was clearly documented?) but ultimately the @nodejs/docker needs to make the call here
  • A PR to make the slim variants of 0.10 and 0.12 consistent with 4.x and 5.x. I can work on that later this week for review.

Once the above two issues are resolve we should be able to move forward with this PR

@chorrell
Copy link
Contributor

chorrell commented Apr 13, 2016

PR to make the 0.10 and 0.12 slim variants consistent: #149

@chorrell
Copy link
Contributor

chorrell commented Apr 15, 2016

Also, to answer your previous question, I think it would be better to wait for the other issues to be resolved and then rebase this rather than split up the PR.

chorrell added a commit that referenced this pull request Apr 18, 2016
Use buildpack-deps:jessie-curl for 0.10 and 0.12 slim images. See #144
@chorrell chorrell changed the title WIP: Add templates for generasting the variants WIP: Add templates for generating the variants Apr 18, 2016
@chorrell
Copy link
Contributor

I just merged #149. In terms of #57 I think we should just modify the update.sh script with a sed line to remove ENV NPM_CONFIG_LOGLEVEL info for the 0.10 and 0.12 images until a decision is made.

I'll merge this now and work on a follow update for the update.sh script.

And thanks for this @LaurentGoderre !

@chorrell chorrell merged commit f5bcf5c into nodejs:master Apr 18, 2016
@LaurentGoderre LaurentGoderre deleted the alpine branch April 18, 2016 14:50
chorrell added a commit that referenced this pull request Apr 18, 2016
* Don't set npm log info for 0.10 and 0.12.

Reference: #57, #144
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.

2 participants