(MODULES-1637) Major refactor #103

juniorsysadmin · 2015-01-06T03:51:21Z

As mentioned in the Changelog and commit log, this Pull Request if commited will break a few things since major changes have been made to the API.

The code could probably be a bit better, so I have created this PR to get some feedback before I create a some acceptance tests.

@x3dfxjunkie - this PR potentially supercedes PR 102
@b4ldr - this PR potentially supercedes PR 99
@menssen - this PR potentially supercedes PR 96
@CpuID - this PR potentially supercedes PR 94
@aebm - this PR potentially supercedes PR 93
@wassimsalib - this might make your PR 89 redundant
@einyx - this PR potentially supercedes your PR 85
@jurgenweber, @brentax - this PR potentially supercedes PR 82
@jasperla - this PR potentially supercedes PR 80 - Are you perhaps able to check that it works nicely on FreeBSD?
@jlambert121 - this PR potentially supercedes PR 79 - Travis Puppet 2.7 tests have been removed, and I have attempted to use modulesync
@wenlock, @wknapik - this might make PR 70 redundant
@emonty, @rchrd2, @atrepca - this PR potentially supercedes PR 69
@mpdude - this PR potentially supercedes PR 66 - Are you perhaps able to confirm that the MacPorts default npm path is correct?

A few starting points of discussion:

It might be possible to modify this code to be backward-compatible with the current version, but it seems like a lot of work. If one follows the SemVer guidelines, breaking the API for 0.x releases is perfectly okay.
This code adds an explicit dependency on the treydock/gpg_key module for importing the YUM repository keys. I'm not sure if this is a something that is preferred for PuppetLabs modules in particular.
For whatever reason, Node.js often gets installed on a variety of operating systems, included 'bleeding edge' ones. The usual policy of failing hard when newer operating system versions are supported does
not seem to make sense for this module. Any strong objections?
The Travis CI Puppet 2.7 tests have been removed, because I errored out out with unusual messages on rspec tests, even though a real test on a Puppet 2.7 agent seemed to be successful.
These patches don't modify the npm (global) Puppet provider code, which can probably be left for a new PR. While this code forces the global config settings proxy and https-proxy to be applied first before any local npm packages are installed, it does not do something similar with the npm provider type.
There is a need for the npm_path parameter since Windows, Darwin and possibly others default to installing npm in various places. The npm defined types have been modified to accommodate this, but the provider has not.
NodeSource haven't released a binaries for Fedora 21, but I would think they are imminent.
I have updated the module version number to 0.7.0 in various places, and if this gets checked in the
tag on the master will need to be updated as well. This is rather optimistic, so I am happy to reverse these changes.

juniorsysadmin · 2015-01-06T11:25:56Z

@skpy This incorporates your previous PR on parameterizing package names slightly differently, so you may wish to comment as well.

jasperla · 2015-01-06T11:27:53Z

I'll check if this work on OpenBSD instead of FreeBSD as I've never done any work on this module for FreeBSD.

juniorsysadmin · 2015-01-06T12:35:54Z

@jasperla My mistake. I have just pushed through another commit. Can you confirm that the default npm path is correct on OpenBSD?

rnelson0 · 2015-01-06T14:14:22Z

README.md


-Debian users may want to use apt::pin to pin package installation priority on squeeze. See [puppetlabs-apt](https://github.com/puppetlabs/puppetlabs-apt) for more information.
+```puppet
+nodejs::npm { 'express|/opt/packages':


Describe the 'express|/opt/packages' string format.

x3dfxjunkie · 2015-01-06T16:54:59Z

@juniorsysadmin, that looks like it would make my PR 102 irrelevant (not that I'm complaining).

sfc-gh-eraigosa · 2015-01-06T21:03:21Z

@juniorsysadmin indeed that would do it. +1 ty!

juniorsysadmin · 2015-01-07T01:58:29Z

@jasperla - have fixed that in last commit
@rnelson0 - have added some documentation

rnelson0 · 2015-01-07T03:05:48Z

@juniorsysadmin I think the nodejs::npm docs at the top look better and it doesn't feel too verbose to me, just enough.

juniorsysadmin · 2015-01-17T12:18:19Z

Rebased. Does anyone have any more feedback to add?

juniorsysadmin · 2015-01-21T05:52:57Z

Rebased. Optimistic proposed version number changed to 0.8.0.

underscorgan · 2015-01-21T22:25:37Z

CHANGELOG.md

@@ -1,3 +1,26 @@
+##2015-01-21 0.8.0


That date's a little optimistic, as a release date. ;)

juniorsysadmin · 2015-01-26T02:22:22Z

Rebased and some changes made based on feedback.

juniorsysadmin · 2015-01-26T07:27:09Z

NodeSource haven't released a binaries for Fedora 21, but I would think they are imminent.

NodeSource have released Fedora 21 binaries today on their repository.

igalic · 2015-01-27T17:28:43Z

general comments: i remain unhappy with node::npm { 'foo|/target/dir': ... }
i would've thought that in a major refactor we would've changed this to target => /target/dir, but i guess the problem here is that we'll run into title uniqueness issues.

nibalizer · 2015-01-28T11:26:22Z

I tend to agree with @igalic that the syntax used to hack in composite namvars is startling and my initial reaction to it is negative.

That being said, composite namevars have always been a 'maybe next year' so hacking it in with pipes in the node module, hey why not. The more I look at it, the more excited about the prospect I get.

I haven't been able to review the entire patch despite several attempts. This patch is super huge which is a problem for any reviewer. I hate to be "that guy" but could this be broken up into a few logical chunks?

This seems like it is the best way forward for a real future for this module. @juniorsysadmin do you anticipate being able to take on primary ownership for this module going forward?

To respond directly to the conversation starters:

semver does state that you can YOLO on the entire 0.x lifetime. I think its great to keep it in 0.x so that we can massage this new syntax a bit. I think you should have no fear of breaking compat with the old version.
A lot of modules do hardcode in the gpg keys. Rabbitmq does this. I don't have a preference, if a module makes this easier then I'm ok with it.
Not sure I understand this. Are you saying when an unrecognized operating system is detected it should fail, but you've wired it to not fail and keep on chugging? I suppose I think thats okay.
Puppet 2.7 should diaf.
Largely the node::npm defined type is replacing the provider for the package resource. I think we can let the ruby provider lag in feature set until it's discovered what the utility of the new syntax for packages is figured out.
Sounds like this resolved itself.
Sounds good.

igalic · 2015-01-28T12:11:29Z

manifests/npm.pp

+    $package_string = $npm_package
+  }
+  elsif $ensure == 'absent' {
+    $package_string = $npm_package


these two branches can be collapsed.

juniorsysadmin · 2015-01-28T12:13:07Z

I haven't been able to review the entire patch despite several attempts. This patch is super huge which is a problem for any reviewer. I hate to be "that guy" but could this be broken up into a few logical chunks?

I could try and do this, but it's going to be really, really hard - the provider is the only code part that escaped unscathed, and that's because I don't have very strong knowledge of Puppet provider types.
It's a bit dishonest to call this a Pull Request - it's really just a fork masquerading as one. So you're better off just looking at the forked code rather than diffs.

Not sure I understand this. Are you saying when an unrecognized operating system is detected it should fail, but you've wired it to not fail and keep on chugging? I suppose I think thats okay.

What my convoluted sentence was trying to say was:

This module is not sticking with long term releases as the only 'supported' versions
EOL operating system versions are 'not supported'
Anything newer than a long term release it will chug away with a warning
That's the idea anyway.

This seems like it is the best way forward for a real future for this module. @juniorsysadmin do you anticipate being able to take on primary ownership for this module going forward?

I can try - my knowledge of Puppet providers is not great though. The current version looks like an absolute nightmare for anyone who needs to merge PRs, I would think this version would make things a lot easier for whoever needs to merge PRs in future.

igalic · 2015-01-28T12:14:04Z

manifests/npm.pp

+    $package_string = "${npm_package}@${npm_tag}"
+  }
+  elsif $ensure == 'latest' {
+    $package_string = "${npm_package}@latest"


if that was the case, we could, again, collapse these two branches, i.e.: if ensure matches latest, or a version string, just append it.
in that case, it would make sense to make this the default, last case. i.e.: present, absent, etc.. have already been handled, now we just give up and decide it's a version number (or latest)

nibalizer · 2015-01-28T19:24:14Z

@hlindberg courtesy ping so you can look at the '|' namevar syntax.

hlindberg · 2015-01-28T23:03:05Z

@nibalizer From a language perspective, the title is any valid string and it is up to the type to do what it pleases with it. From a resource perspective it has to be unique. In practice, there are several types where a compound namevar is wanted and it would be a shame if it is done differently in different types.

My natural tendency would be to use URI syntax for this sort of thing - i.e a path within a domain. Protocol is not needed since that is really the type.

juniorsysadmin · 2015-02-01T08:27:32Z

Based on feedback (especially about the namevar ugliness), the last commit:

Allows an arbitary value for the namevar for the nodejs:npm defined type except if one wants to be lazy and not specify a package name
Adds a user parameter to allow npm install to be run as a specific user

When ensure => latest is set, the installcheck will always fail and run npm install package@latest - I am not sure if there is much that can be done about this without using a Custom Type. Longer term, a better direction for this defined type is probably a wrapper which calls a Custom Type, similar to the what is done in puppetlabs-mysql etc.

juniorsysadmin · 2015-02-04T13:41:21Z

Alignment issues in the RSpec files of the last commit have been fixed.

igalic · 2015-02-06T10:42:35Z

👍 for merge

eljohnson92 · 2015-02-12T20:21:15Z

When are you planning on merging this? we would like support for updated node

igalic · 2015-02-16T13:28:56Z

@cyberious, @mhaskel: ping

nibalizer · 2015-02-16T16:22:59Z

ya lets merge this +1

This set of patches includes multiple changes and is not backward-compatible. In summary this code: - Supercedes PR 99 (MODULES-1075), 97, 96, 94, 93, 85, 82, 80, 79, 51, 69, 66 and 102 - Explicitly makes Debian Squeeze unsupported - Makes nodejs, nodejs-dev and nodejs-debug package names parameters - Defaults to using the NodeSource repositories where possible, but allows native packages to be installed when appropriate parameters are set - Introduces a parameter repo_class, which allows one to use alternative repositories like EPEL for the Node.js packages. - Adds Windows installation support via Chocolatey - Adds FreeBSD and OpenBSD support - Changes the format of the nodejs::npm defined type to be module|path rather than path:module for the name - Attempts to support all of the npm install <package> forms, except for version ranges. Tags are supported with a separate parameter, and the version parameter has been removed (Version numbers can be passed to the ensure parameter instead) - Adds a defined type nodejs::npm::global_config_entry, which allows one to set and delete global npm config options. The proxy parameter in nodejs:npm has been removed since this can now be done with global_config_entry. - Adds a number of RSpec unit tests - Adds a Beaker acceptance test skeleton

juniorsysadmin · 2015-02-19T05:42:45Z

I have fixed two major issues in nodejs::npm - ensure => absent should now work properly, and default npm paths are now correct on Windows. (The commits have been squashed). I have also done a fair bit of testing of the npm defined type on Linux and Windows, so I'm reasonably satisfied that it works as advertised.

daenney · 2015-02-19T07:56:07Z

This code adds an explicit dependency on the treydock/gpg_key module for importing the YUM repository keys. I'm not sure if this is a something that is preferred for PuppetLabs modules in particular.

This shouldn't really be needed. All it takes is to drop one file, the key file, in the right directory: /etc/pki/rpm-gpg/RPM-GPG-KEY-$name.

(MODULES-1637) Major refactor

nibalizer · 2015-02-19T18:28:52Z

\o/

igalic · 2015-02-19T19:36:45Z

it's done!

thank you everyone for reviewing, and thank you so much @juniorsysadmin!

mmarseglia · 2015-02-19T22:57:09Z

Thank you @juniorsysadmin!

rnelson0 reviewed Jan 6, 2015
View reviewed changes

jasperla mentioned this pull request Jan 21, 2015

Add support for OpenBSD #80

Closed

underscorgan added backwards-incompatible enhancement New feature or request labels Jan 21, 2015

underscorgan reviewed Jan 21, 2015
View reviewed changes

CHANGELOG.md

@@ -1,3 +1,26 @@

##2015-01-21 0.8.0

Copy link

Contributor

underscorgan Jan 21, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That date's a little optimistic, as a release date. ;)

igalic reviewed Jan 28, 2015
View reviewed changes

igalic mentioned this pull request Feb 6, 2015

added support for installing latest nodejs for Enterprise Linux from nod... #109

Closed

juniorsysadmin mentioned this pull request Feb 12, 2015

Update init.pp #110

Closed

hunner added a commit that referenced this pull request Feb 19, 2015

Merge pull request #103 from juniorsysadmin/major-changes

5d09187

(MODULES-1637) Major refactor

hunner merged commit 5d09187 into voxpupuli:master Feb 19, 2015

This was referenced Feb 19, 2015

Fix for 3.7.3 (parser=future) Invalid relationship #102

Closed

If using standard ubuntu repos you need to install npm separately #99

Closed

Don't install nodejs-dev for Ubuntu dev repo #96

Closed

juniorsysadmin deleted the major-changes branch February 20, 2015 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(MODULES-1637) Major refactor #103

(MODULES-1637) Major refactor #103

juniorsysadmin commented Jan 6, 2015

juniorsysadmin commented Jan 6, 2015

jasperla commented Jan 6, 2015

juniorsysadmin commented Jan 6, 2015

rnelson0 Jan 6, 2015

x3dfxjunkie commented Jan 6, 2015

sfc-gh-eraigosa commented Jan 6, 2015

juniorsysadmin commented Jan 7, 2015

rnelson0 commented Jan 7, 2015

juniorsysadmin commented Jan 17, 2015

juniorsysadmin commented Jan 21, 2015

underscorgan Jan 21, 2015

juniorsysadmin commented Jan 26, 2015

juniorsysadmin commented Jan 26, 2015

igalic commented Jan 27, 2015

nibalizer commented Jan 28, 2015

igalic Jan 28, 2015

juniorsysadmin commented Jan 28, 2015

igalic Jan 28, 2015

nibalizer commented Jan 28, 2015

hlindberg commented Jan 28, 2015

juniorsysadmin commented Feb 1, 2015

juniorsysadmin commented Feb 4, 2015

igalic commented Feb 6, 2015

eljohnson92 commented Feb 12, 2015

igalic commented Feb 16, 2015

nibalizer commented Feb 16, 2015

juniorsysadmin commented Feb 19, 2015

daenney commented Feb 19, 2015

nibalizer commented Feb 19, 2015

igalic commented Feb 19, 2015

mmarseglia commented Feb 19, 2015

(MODULES-1637) Major refactor #103

(MODULES-1637) Major refactor #103

Conversation

juniorsysadmin commented Jan 6, 2015

juniorsysadmin commented Jan 6, 2015

jasperla commented Jan 6, 2015

juniorsysadmin commented Jan 6, 2015

rnelson0 Jan 6, 2015

Choose a reason for hiding this comment

x3dfxjunkie commented Jan 6, 2015

sfc-gh-eraigosa commented Jan 6, 2015

juniorsysadmin commented Jan 7, 2015

rnelson0 commented Jan 7, 2015

juniorsysadmin commented Jan 17, 2015

juniorsysadmin commented Jan 21, 2015

underscorgan Jan 21, 2015

Choose a reason for hiding this comment

juniorsysadmin commented Jan 26, 2015

juniorsysadmin commented Jan 26, 2015

igalic commented Jan 27, 2015

nibalizer commented Jan 28, 2015

igalic Jan 28, 2015

Choose a reason for hiding this comment

juniorsysadmin commented Jan 28, 2015

igalic Jan 28, 2015

Choose a reason for hiding this comment

nibalizer commented Jan 28, 2015

hlindberg commented Jan 28, 2015

juniorsysadmin commented Feb 1, 2015

juniorsysadmin commented Feb 4, 2015

igalic commented Feb 6, 2015

eljohnson92 commented Feb 12, 2015

igalic commented Feb 16, 2015

nibalizer commented Feb 16, 2015

juniorsysadmin commented Feb 19, 2015

daenney commented Feb 19, 2015

nibalizer commented Feb 19, 2015

igalic commented Feb 19, 2015

mmarseglia commented Feb 19, 2015