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

Allow for the macports provider to be overridden on macos #321

Closed kizzie closed 6 years ago

kizzie commented 7 years ago

A vague pondering on how to fix this issue - not sure its 100% working or if my macvm is not happy about npm for other reasons. But any thoughts?

kizzie commented 7 years ago

Hmm, so the next issue with this is that the /root/.npmrc file can't be created for npm as that user doesn't exist for macos

kizzie commented 7 years ago

There we go, working when you pass in

class { 'nodejs' : mac_provider => 'homebrew' }

if nothing else

kizzie commented 7 years ago

I'll need to do some reading up on how to get travis to add tests for osx

I wouldn't bother with this for now.

juniorsysadmin commented 7 years ago

@kizzie I'm okay with merging minus the travis.yml changes, leaving matching rspec-puppet tests for another Pull Request.

I'll need to do some reading up on how to get travis to add tests for osx

I wouldn't bother with this for now.

kizzie commented 7 years ago

Okey dokey, shall remove the travis bits, as I never did get time to look at it - the rspec ones should at least be easier than trying to force beaker into working forit :)

kizzie commented 7 years ago

Do you have a preferred way to squash all the commits? Its starting to look a bit of a mess :) (and I'll have to work out what is up with that test when I'm back at work and have more tools with me than just atom and a command prompt - I think changing it from ::osfamily to the new fact style may have caused some interesting times for the tests that don't define it explicitly?)

bastelfreak commented 7 years ago

Personally I prefer a history with small commits, so this is in general okay. Can you rebase and get rid of 2c1ca6772d5ff6610bc93cdb08b6b1923b6736b1 and 96e53dc65c257b2601183652948bb1848ca3028f?

0511a52142343c51ed59ca0a92c863846a2d6725, 0276ab17728d649cdfd42138fbcd3ae936f47086 and 5911eb386f5d62ae5e3948175abaa380dde3e1f7 can be squashed into f290be1950fac5c3aed206843e0adabdd58b0d00

bastelfreak commented 7 years ago

Can you check the email address used in the first 6 commits? The used email address isn't associated with your github account.

kizzie commented 7 years ago

Oooh work laptop vs home computer. That's awkward. And annoyingly identifiable. Ho hum!

kizzie commented 7 years ago

So my initial guess was correct, the rspec tests don't like $facts['os']['family'] for anything but ubuntu based systems - it's happy with $facts['osfamily'] though - Not sure whether you want me to go through and work out how to change all the tests to work, or whether to use the older style fact hash?

bastelfreak commented 7 years ago

$facts['os']['family'] is probably not correctly mocked :(. Using the older $facts['osfamily'] is fine as well. It won't disappear soon.

kizzie commented 7 years ago

All fixed up - with some added tests and a bit in the read me :)

juniorsysadmin commented 6 years ago

@kizzie Did you still need this?

kizzie commented 6 years ago

Oh yes, we switched to doing something else internally (because even homebrew support wasn't giving us the version we were after) but will try and fix this up this week for your comments

kizzie commented 6 years ago

rebased to master now so hopefully should all be working! (edit: You try to add a single bit to the documentation and then everything has decided to commit strangely... not sure what I did there :/ will try and sort it out Monday as that rebase went very wrong)

kizzie commented 6 years ago

There we go. Different morning, different realisation that master may have updated. Hopefully all in order now.

kizzie commented 6 years ago

I have no idea why the build is failing to find bundle, any ideas? (it may be that bundler 1.16.1 was released yesterday and isn't quite making it to the build node?)

juniorsysadmin commented 6 years ago

@kizzie Thanks for this PR, I have just squashed commits and merged.

ekohl commented 6 years ago

Thanks for addressing all of my many comments @kizzie!