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

npm is required on 16.04 #242

Closed gabriel403 closed 8 years ago

gabriel403 commented 8 years ago

On 16.04 we require npm to be installed along side nodejs for npm to be useable

bastelfreak commented 8 years ago

Hi, thanks for the patch. Do you think you can add tests somehow for this?

gabriel403 commented 8 years ago

@bastelfreak would love to, but I'm not really sure how to, could you point me in the right direction? I know without it on 16.04 you get

Warning: Scope(Class[Nodejs::Params]): The nodejs module might not work on Ubuntu 16.04. Sensible defaults will be attempted.

and npm isn't useable

Notice: /Stage[main]/Nodejs::Install/Package[nodejs]/ensure: ensure changed 'purged' to 'present'
Error: Command npm is missing
Error: /Stage[main]/Boxes::Packages/Package[bower]/ensure: change from absent to present failed: Command npm is missing
jackdpeterson commented 8 years ago

Even without this change ... if I have defined my deps as follows:

class { '::nodejs':
  nodejs_dev_package_ensure => 'present',
  npm_package_ensure        => 'present',
}

package { 'babel':
  ensure   => 'present',
  provider => 'npm',
}

package { 'webpack':
  ensure   => 'present',
  provider => 'npm',
}

package { 'browserify':
  ensure   => 'present',
  provider => 'npm',
}

I do get the complaining about possibly not working on 16.04 ... but everything (including the npm command) worked fine for me. Perhaps there's something else related too with respect to the provider for the 16.04 release of ubuntu.

gabriel403 commented 8 years ago

@jackdpeterson yeh so you're forcing the installation of npm with the npm_package_ensure param, but should npm be installed by default? If not then yeh doing it your way makes more sense than the pr. Thoughts @bastelfreak ?

juniorsysadmin commented 8 years ago

We don't have tests for defaults, just behaviour of parameters. Happy to merge this as is.