search

voxpupuli / puppet-nodejs

Puppet module to install nodejs and global npm packages
https://forge.puppet.com/puppet/nodejs
Apache License 2.0
113 stars 247 forks source link

Add an option to install npm deps from package.json #300

Closed poikilotherm closed 7 years ago

poikilotherm commented 7 years ago

Dear maintainers of voxpopuli,

I found #154 and #167 and needed the functionality for myself (installing a Mathoid server, which is based on NodeJS).

Thus I added the necessary bits and would be glad if this would be merged upstream. From my point of view, the code should be idempotent.

Thanks in advance!

Cheers, Oliver

poikilotherm commented 7 years ago

Hi @all,

I just rebased... Do you prefer a single commit for all changes? (I personally prefer to seperate code, tests and doc changes in different commits.)

Cheers, Oliver

juniorsysadmin commented 7 years ago

Do you prefer a single commit for all changes

I do. But CONTRIBUTING.md just says "squash your commits down into logical components"

poikilotherm commented 7 years ago

Squashed as requested. Please merge :-)

Sorry, overlooked your comment about regsubst...

wyardley commented 7 years ago

@poikilotherm @juniorsysadmin I think a single commit is fine. The logical components bit is mostly just saying that it's Ok to break up the commits if they're done in a logical way. What vox mostly tries to avoid is

So, squashing to a single commit is usually appropriate for smaller changes. If it's a really big changeset, I have found that breaking it up into chunks can make it easier for the reviewer. However, it can make rebasing a pain sometimes too.

juniorsysadmin commented 7 years ago

@poikilotherm Merge, or don't merge just yet?

poikilotherm commented 7 years ago

I'm ok with the / and \ thing as it is right now. If your can live with it, please go ahead and merge... :-)

bastelfreak commented 7 years ago

Thanks for the PR @poikilotherm!