Add an option to install npm deps from package.json #300
Conversation
manifests/npm.pp
Outdated
| ) { | ||
|
|
||
| validate_re($ensure, '^[^<>=]', "The ${module_name}::npm defined type does not accept version ranges") | ||
| validate_array($install_options) | ||
| validate_string($package) | ||
| validate_absolute_path($target) | ||
| validate_array($uninstall_options) | ||
| validate_bool($use_package_json) |
There was a problem hiding this comment.
If you rebase, this should now use data types instead (remove validate_bool, and update
$use_package_json = false,
to
Boolean $use_package_json = false,
e.g., https://github.com/voxpupuli/puppet-nodejs/blob/master/manifests/init.pp#L4
There was a problem hiding this comment.
True, but probably better to change all of these in a new PR, now this one
| } | ||
|
|
||
| $list_command = "${npm_path} ls --long --parseable" | ||
| $install_check = "${list_command} | ${grep_command} \"${target}${dirsep}node_modules${dirsep}${install_check_package_string}\"" |
There was a problem hiding this comment.
I have mixed feelings about a using a variable for a directory separator in a string - I personally prefer explicit versus implicit values for it.
There was a problem hiding this comment.
IMHO this makes the code more readable - the string just differ by the dir seps...
Maybe let's replace it with a call to regsubst on Windows to replace / with \\?
There was a problem hiding this comment.
Maybe let's replace it with a call to regsubst on Windows to replace / with \?
That's an idea. Not sure if there will be unintended side-effects to doing this though.
There was a problem hiding this comment.
Maybe this is not even necessary...
According to https://docs.puppet.com/puppet/5.2/lang_windows_file_paths.html#when-to-use-each-kind-of-slash, we can just use slashes.
I don't have a Windows Puppet at hands to prove this true or false - anything available on your side?
There was a problem hiding this comment.
@poikilotherm Er I'm not sure if I'm much help - I haven't tested Puppet on Windows with anything for...quite some some time. Looking at the docs in more detail though, it seems as though we do care about what type of slashes, because we're using cmd.exe to run the grep-alternative and cmd.exe requires backslashes.
There was a problem hiding this comment.
/ should just work, both in puppet and in node.
manifests/npm.pp
Outdated
| ) { | ||
|
|
||
| validate_re($ensure, '^[^<>=]', "The ${module_name}::npm defined type does not accept version ranges") | ||
| validate_array($install_options) | ||
| validate_string($package) | ||
| validate_absolute_path($target) | ||
| validate_array($uninstall_options) | ||
| validate_bool($use_package_json) |
There was a problem hiding this comment.
True, but probably better to change all of these in a new PR, now this one
|
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, |
I do. But CONTRIBUTING.md just says "squash your commits down into logical components" |
juniorsysadmin
added
enhancement
…s::npm * Extended defined type. * Added spec testing for new option to use package.json. * Updated docs in README.md about package.json usage.
|
Sorry, overlooked your comment about |
|
@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.
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. |
|
@poikilotherm Merge, or don't merge just yet? |
|
I'm ok with the |
| @@ -10,6 +10,7 @@ | |||
| Array $uninstall_options = [], | |||
| $home_dir = '/root', | |||
| $user = undef, | |||
| Boolean $use_package_json = false, | |||
| Uninstall dependencies from package.json: | ||
|
|
||
| ```puppet | ||
| nodejs::npm { 'serverapp': |
|
Thanks for the PR @poikilotherm! |
bastelfreak
changed the title
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