voxpupuli / puppet-nodejs

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

Fix default install as non-root user, #305 #363

Closed fnoop closed 4 years ago

fnoop commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

OK that failing check makes an incorrect assumption, removed

fnoop commented 6 years ago

Ooh, a green tick from Travis :)

ekohl commented 6 years ago

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

fnoop commented 6 years ago

@ekohl Yes, works fine.

juniorsysadmin commented 6 years ago

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 commented 6 years ago

@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

Dan33l commented 5 years ago

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