#2007 - Introduce nodejs module

#2007 - Introduce nodejs module

[mathias, 2017-07-09 07:26:50]

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
File modules/nodejs/manifests/init.pp (right):

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
modules/nodejs/manifests/init.pp:47: require => Apt::Source[$title],
Use arrow-notation to establish relationships, especially when using this merge
magic. Otherwise the configuration could overwrite your definitions by accident.

(BTW, this very relationship is already configured properly below.)

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
modules/nodejs/manifests/init.pp:60: ensure_resource('apt::key', $title, merge({
How is the user supposed to remove this one if the package is ensured to be
'absent'?

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
modules/nodejs/manifests/init.pp:65: ensure_resource('apt::source', $title,
merge({
How is the user supposed to remove this one if the package is ensured to be
'absent'?

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
File modules/nodejs/manifests/package.pp (right):

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:10: define nodejs::package (
Shouldn't there be some $ensure parameter in order to allow for un-installing an
NPM package?

https://codereview.adblockplus.org/29483578/diff/29483579/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:24: onlyif => "test ! -x /usr/bin/${title}",
Please use the 'creates' parameter instead.

[mathias, 2017-07-10 22:33:41]

https://codereview.adblockplus.org/29483578/diff/29485609/modules/nodejs/mani...
File modules/nodejs/manifests/init.pp (right):

https://codereview.adblockplus.org/29483578/diff/29485609/modules/nodejs/mani...
modules/nodejs/manifests/init.pp:44: $ensure = ensure_state($ensure) ? {
Please use actual if/else blocks. That way future changes introducing new
conditional parameter values do not need to perform the same decision.

https://codereview.adblockplus.org/29483578/diff/29485609/modules/nodejs/mani...
File modules/nodejs/manifests/package.pp (right):

https://codereview.adblockplus.org/29483578/diff/29485609/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:27: $creates = "/usr/bin/${title}"
Are you sure every package will create a file or executable in that location?

[mathias, 2017-07-11 18:24:43]

https://codereview.adblockplus.org/29483578/diff/29486573/modules/nodejs/mani...
File modules/nodejs/manifests/init.pp (right):

https://codereview.adblockplus.org/29483578/diff/29486573/modules/nodejs/mani...
modules/nodejs/manifests/init.pp:43: # Used as default $ensure parameter for
most resources below
That comment doesn't really fit here any more.

https://codereview.adblockplus.org/29483578/diff/29486573/modules/nodejs/mani...
File modules/nodejs/manifests/package.pp (right):

https://codereview.adblockplus.org/29483578/diff/29486573/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:34: $onlyif = shellquote("test", "!",
"`${check_command}`")
That doesn't make any sense: The backticks are escaped anyway. And they aren't
even necessary.

[mathias, 2017-07-11 22:28:03]

https://codereview.adblockplus.org/29483578/diff/29486585/modules/nodejs/mani...
File modules/nodejs/manifests/init.pp (right):

https://codereview.adblockplus.org/29483578/diff/29486585/modules/nodejs/mani...
modules/nodejs/manifests/init.pp:3: # Install nodejs package from source
This is not "from source", but "from NodeSource" or similar.

https://codereview.adblockplus.org/29483578/diff/29486585/modules/nodejs/mani...
File modules/nodejs/manifests/package.pp (right):

https://codereview.adblockplus.org/29483578/diff/29486585/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:16: $options = [],
Are you sure we still need that $options parameter?

https://codereview.adblockplus.org/29483578/diff/29486585/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:49: exec {"state_$title":
Not a good title, no association with the type definition or purpose of the
command. Please something like "nodejs package $title" or similar.

https://codereview.adblockplus.org/29483578/diff/29486585/modules/nodejs/mani...
modules/nodejs/manifests/package.pp:50: path => ["/usr/bin"],
Not sure but this path seems too short. I am sure it works, but I'd say without
explicit $PATH requirements it should always include /bin as well.

[mathias, 2017-07-12 16:04:49]

LGTM.