Fix default install as non-root user, #305

[fnoop]

npm.pp currently sets /root as the environment HOMEDIR. This breaks using nodejs::npm as a non-root user, as per #305. nodejs::npm::home_dir parameter can be set to override this, but is not intuitive, documented or automatic.

Instead, unset $home_dir parameter by default and only use it to set HOMEDIR environment variable if populated. If not set, npm will use the default home directory for the specified user.

[fnoop]

Hmm, well travis fails because of:
manifests/npm.pp:11:double_quoted_strings:WARNING:double quoted string containing no variables

However if you set default to undef ( String $home_dir = undef,) then you get a syntax error:

Error: Evaluation Error: Error while evaluating a Resource Statement, Nodejs::Npm[vue install as user mav]: parameter 'home_dir' expects a String value, got Undef (file: /srv/maverick/software/maverick/manifests/maverick-modules/maverick_network/manifests/init.pp, line: 16) on node dom-ubuntu.home

The obvious fix is to set $home_dir parameter without a type:
$home_dir = undef
But this doesn't follow good practice of setting parameter types..

[fnoop]

This is now failing spec tests, which just completely baffle me:

  1) nodejs::npm on centos-6-x86_64  with package set to express and target set to /home/npm/packages the environment variable HOME should be /root
     Failure/Error: is_expected.to contain_exec('npm_install_express').with('environment' => 'HOME=/root')
       expected that the catalogue would contain Exec[npm_install_express] with environment set to "HOME=/root" but it is set to nil
     # ./spec/defines/nodejs_npm_spec.rb:57:in `block (5 levels) in <top (required)>'

[fnoop]

OK that failing check makes an incorrect assumption, removed

[fnoop]

Ooh, a green tick from Travis :)

[ekohl]

Does this work? AFAIK HOME is not set in exec so does npm resolve this itself?

[fnoop]

@ekohl Yes, works fine.

[juniorsysadmin]

Hmm, can we go a bit further and add environment as a parameter since there may be other environment variables that one may wish to pass onto npm? (See https://docs.npmjs.com/misc/config#environment-variables)

Perhaps something like:

Parameters:
$home_dir => undef,
$environment => undef,

if !environment and !home_dir {
  $real_environment = "HOME=${home_dir}"
} else {
  $real_environment = $environment

[fnoop]

@juniorsysadmin Not convinced that it's a good idea to encourage passing/using environment variables for npm config. Better to write manifests or use .npmrc?:
https://docs.npmjs.com/misc/config#npmrc-files

[igalic]

looks good!
here's some of my opinions…

[igalic]

while we're at it, we could just as well give home_dir an Optional[Stdlib::AbsPath] type.

[igalic]

i think a selector is easier to read?

$_homedir = $home_dir? {
  undef => undef,
  default => "HOME=${home_dir}",
}

[Dan33l]

Instead of removing the test, why not modify it ?

[Dan33l]

Hi @fnoop do need some help to move forward about this PR ?

[ghoneycutt]

This changes the default behavior of the module and would likely need a major version bump.