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

kizzie · 2017-09-20T09:33:42Z

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 · 2017-09-20T09:48:46Z

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 · 2017-09-20T09:59:27Z

There we go, working when you pass in

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

if nothing else

juniorsysadmin

Some tests to cover the change would be super as well, as well as some changes to the README

juniorsysadmin · 2017-09-24T06:06:40Z

manifests/init.pp

@@ -24,8 +24,20 @@
 $repo_proxy_username = $nodejs::params::repo_proxy_username,
 $repo_url_suffix = $nodejs::params::repo_url_suffix,
 Array $use_flags = $nodejs::params::use_flags,
+ $mac_provider = undef,


This isn't the right way to do it. For consistency, the best approach is to create a new variable in params.pp $darwin_package_provider = 'macports' then do $darwin_package_provider = $nodejs::params::darwin_package_provider in init.pp

Oooh good point, I was having issues with the provider not overriding when the package statement was in the params as well, but actually that could be moved to init and the default still set in params.

juniorsysadmin · 2017-09-24T06:08:52Z

manifests/install.pp

@@ -60,9 +60,15 @@
 }
 }

+ if $::osfamily == 'Darwin' {
+ $path = '/var/root'


Given $path is a banned keyword now I think, consider something like $root_npmrc_path = '/var/root/.npmrc' instead

Oh dear, that's going to cause some fun for me (just starting to move things from puppet 3 to 4 and must have missed this one!)

kizzie · 2017-09-25T08:56:55Z

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 · 2017-10-15T08:24:34Z

@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 · 2017-10-15T08:44:38Z

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 :)

bastelfreak · 2017-10-15T08:58:29Z

manifests/install.pp

@@ -60,9 +60,15 @@
 }
 }

+ if $::osfamily == 'Darwin' {


please use the new facts hash here:
$facts['os']['family']

bastelfreak · 2017-10-15T08:58:35Z

manifests/init.pp

 ) inherits nodejs::params {

+ if $::osfamily == 'Darwin' {


please use the new facts hash here:
$facts['os']['family']

bastelfreak · 2017-10-15T08:58:52Z

manifests/init.pp

@@ -24,8 +24,13 @@
 $repo_proxy_username = $nodejs::params::repo_proxy_username,
 $repo_url_suffix = $nodejs::params::repo_url_suffix,
 Array $use_flags = $nodejs::params::use_flags,
+ $darwin_package_provider = $nodejs::params::darwin_package_provider,


please add a puppet4 datatype for this new parameter

kizzie · 2017-10-15T09:12:00Z

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 · 2017-10-15T09:49:15Z

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

0511a52, 0276ab1 and 5911eb3 can be squashed into f290be1

bastelfreak · 2017-10-15T09:51:46Z

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

kizzie · 2017-10-15T09:55:53Z

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

kizzie · 2017-10-16T14:17:59Z

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 · 2017-10-16T21:20:18Z

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

kizzie · 2017-10-17T09:13:35Z

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

kizzie · 2017-12-18T07:56:27Z

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

ekohl · 2017-12-18T11:37:07Z

manifests/params.pp

@@ -57,6 +57,8 @@
 $npm_path = '/usr/bin/npm'
 $repo_class = '::nodejs::repo::nodesource'
 }
+
+ $package_provider = 'apt'


I'd prefer to set this and others (yum, zypper etc) to undef unless it's non-standard (macports? chocolatey?).

Changed it over

ekohl · 2017-12-19T10:48:43Z

manifests/install.pp

@@ -18,7 +18,9 @@
 }
 }

- Package { provider => $nodejs::package_provider }
+ if $nodejs::package_provider != undef {


Technically this is not needed. When undef it'll fall back on the default value from puppet.

ekohl · 2017-12-19T10:51:22Z

spec/classes/nodejs_spec.rb

@@ -18,7 +18,7 @@
 it 'the file resource root_npmrc should be in the catalog' do
 is_expected.to contain_file('root_npmrc').with(
 'ensure' => 'file',
- 'path' => '/root/.npmrc',
+ 'path' => '/.npmrc',


This is unexpected. We should fix a value for that fact. Could you add it to spec/default_facts.yml? I'll submit a PR to modulesync_config to ensure it's not overridden.

ekohl · 2017-12-19T10:52:28Z

spec/classes/nodejs_spec.rb

+ it 'the file resource root_npmrc should use /var/root for the path' do
+ is_expected.to contain_file('root_npmrc').with(
+ 'ensure' => 'file',
+ 'path' => '/.npmrc',


This doesn't match the description. Maybe this should be left out since it's already tested above.

We could either leave it out or override the fact for this test to be closer to the default that Darwin should come up with?

kizzie · 2017-12-22T12:05:18Z

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 · 2017-12-26T04:59:30Z

.travis.yml

@@ -7,6 +7,7 @@ before_install:
 - bundle -v
 - rm Gemfile.lock || true
 - gem update --system
+ - gem pristine bundler


Drop this line - after a rebase to master you should have a green build

juniorsysadmin · 2017-12-26T05:00:42Z

spec/default_facts.yml

@@ -12,3 +12,4 @@ concat_basedir: "/tmp"
 ipaddress: "172.16.254.254"
 is_pe: false
 macaddress: "AA:AA:AA:AA:AA:AA"
+root_home: /root


Add this to default_module_facts.yaml as well

If it's in default_module_facts.yaml then can't it be removed here? This file is modulesynced while the other isn't.

When I take it out of here then the tests start failing as its not being picked up by from default_module_facts.yaml

Then I'm not that clear of mind yet. Just leave it in and if it's an issue in the modulesync we'll fix it then.

…dden by the params class

… package providers

… to /var for Darwin

ekohl

Looks good, just some minor changes.

ekohl · 2018-01-02T15:31:19Z

spec/default_facts.yml

@@ -12,3 +12,4 @@ concat_basedir: "/tmp"
 ipaddress: "172.16.254.254"
 is_pe: false
 macaddress: "AA:AA:AA:AA:AA:AA"
+root_home: /root


If it's in default_module_facts.yaml then can't it be removed here? This file is modulesynced while the other isn't.

ekohl · 2018-01-02T15:32:04Z

manifests/install.pp

 file { 'root_npmrc':
 ensure => 'file',
- path => '/root/.npmrc',
+ path => "${root_npmrc_path}/.npmrc",


You can use "${facts['root_home']}/.npmrc" to remove the additional variable.

juniorsysadmin · 2018-01-04T11:36:05Z

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

ekohl · 2018-01-04T11:38:38Z

Thanks for addressing all of my many comments @kizzie!

kizzie mentioned this pull request Sep 20, 2017

Provide option to install node using Brew #236

Closed

kizzie force-pushed the feature/236-override-macports-provider branch from c2ad17a to c1b4424 Compare September 20, 2017 13:04

juniorsysadmin requested changes Sep 24, 2017

View reviewed changes

juniorsysadmin added needs-docs needs-tests needs-work not ready to merge just yet labels Sep 24, 2017

juniorsysadmin added the enhancement New feature or request label Sep 26, 2017

bastelfreak reviewed Oct 15, 2017

View reviewed changes

kizzie force-pushed the feature/236-override-macports-provider branch from 1daba1f to 3b250da Compare October 15, 2017 09:12

kizzie force-pushed the feature/236-override-macports-provider branch 2 times, most recently from e40c4b9 to b853d4a Compare October 16, 2017 13:52

kizzie force-pushed the feature/236-override-macports-provider branch from b853d4a to 4e6eb7d Compare October 17, 2017 09:01

juniorsysadmin removed needs-docs needs-tests needs-work not ready to merge just yet labels Oct 17, 2017

juniorsysadmin approved these changes Oct 17, 2017

View reviewed changes

kizzie force-pushed the feature/236-override-macports-provider branch from e9ef5ea to 05cc203 Compare December 18, 2017 07:55

ekohl requested changes Dec 18, 2017

View reviewed changes

ekohl reviewed Dec 19, 2017

View reviewed changes

kizzie force-pushed the feature/236-override-macports-provider branch 2 times, most recently from 890c9bb to b485c6f Compare December 22, 2017 12:02

juniorsysadmin requested changes Dec 26, 2017

View reviewed changes

kizzie force-pushed the feature/236-override-macports-provider branch from 7a76d31 to b485c6f Compare January 2, 2018 10:08

kizzie added 3 commits January 2, 2018 10:08

Allow for the macports provider to be overridden on macos

a59cccb

Fix .npmrc file going to a non existant directory on Darwin

d2ab52a

Updated readme with override for darwin_package_provider details

899cf99

kizzie force-pushed the feature/236-override-macports-provider branch from b485c6f to 0e57b70 Compare January 2, 2018 10:09

kizzie added 7 commits January 2, 2018 10:13

Rspec tests for darwin_package_provider

1540dcb

Package provider opton overridable for all operating systems

efefdf3

Use root_home fact for the root_npmrc file

1bcaa31

Update README to add package_provider parameter

5030afd

Set default package_provider to undef unless being explicitely overri…

864b359

…dden by the params class

Remove check for undef and rely on puppet defaulting back to standard…

522f478

… package providers

Add default fact for root_home set to /root for most OS and overriden…

bd02e1b

… to /var for Darwin

kizzie force-pushed the feature/236-override-macports-provider branch from 0e57b70 to bd02e1b Compare January 2, 2018 10:14

Add: Default module fact for root_home

059c41c

ekohl requested changes Jan 2, 2018

View reviewed changes

Removed extra variablem for root_home

c6b2439

ekohl approved these changes Jan 2, 2018

View reviewed changes

juniorsysadmin approved these changes Jan 4, 2018

View reviewed changes

juniorsysadmin merged commit e0b6645 into voxpupuli:master Jan 4, 2018

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

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

Conversation

kizzie commented Sep 20, 2017

kizzie commented Sep 20, 2017

kizzie commented Sep 20, 2017

juniorsysadmin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kizzie commented Sep 25, 2017 • edited by juniorsysadmin Loading

juniorsysadmin commented Oct 15, 2017

kizzie commented Oct 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kizzie commented Oct 15, 2017 • edited Loading

bastelfreak commented Oct 15, 2017 • edited Loading

bastelfreak commented Oct 15, 2017

kizzie commented Oct 15, 2017

kizzie commented Oct 16, 2017

bastelfreak commented Oct 16, 2017

kizzie commented Oct 17, 2017

kizzie commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kizzie commented Dec 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juniorsysadmin commented Jan 4, 2018 • edited Loading

ekohl commented Jan 4, 2018

juniorsysadmin left a comment •

edited

Loading

kizzie commented Sep 25, 2017 •

edited by juniorsysadmin

Loading

kizzie commented Oct 15, 2017 •

edited

Loading

bastelfreak commented Oct 15, 2017 •

edited

Loading

kizzie commented Dec 22, 2017 •

edited

Loading

juniorsysadmin commented Jan 4, 2018 •

edited

Loading