Make repo_url_suffix to be usable for RH/CentOS

[tsde]

Hi,

This PR is intended to make the repo_url_suffix variable usable for RPM repos so that you can install NodeJS version 0.10 and above on RedHat based systems. The default value has been changed from "node_0.10" to "0.10". The suffix is then appended to repo URLs (node_suffix for Debian and pub_suffix for RedHat).

I also added some checks regarding the repo_url_suffix variable. There're still missing repos for some version of RedHat or Ubuntu on Nodesource, so I found it relevant to add these checks. Let me know if you find this addition useful or a pain to maintain ;) - I'll exclude this part from the PR in this case.

I've successfully tested this on Debian/Ubuntu and RedHat/CentOS.

[igalic]

what

[tsde]

Not sure what you meant by that. Unclear README info ? The main change is that repo_url_suffix now simply needs to reflect the version number without being preceded by "node_" which is only used by apt repos. For RPM repos, it's "pub_version_number". I tried to reflect all the valid settings in the README because some version of node are not yet available through nodesource repos for some OS releases. For example, there's still no 4.x and 5.x repo available at Nodesource for RedHat/CentOS 6. That's also the case, for some versions of Ubuntu.

Is there something wrong with the PR ?

[igalic]

i'm just wildly confused that 0.10 is the default anywhere…

[tsde]

Well, The current default behavior of the module is setting repo_url_suffix to "node_0.10" in params.pp. So, by default, it installs node version 0.10 on APT-based systems using the following repo URL schema: https://deb.nodesource.com/node_0.10.

Indeed you can change repo_url_suffix to "node_4.x" to install node version 4 on APT-based systems but what about RPM-based distros ? Currently, the RPM URL schema used in nodesource.pp is https://rpm.nodesource.com/pub/${dist_type}/${dist_version}/$basearch (for example https://rpm.nodesource.com/pub/el/7/x86_64 for RedHat 7). This endpoint only provides node version 0.10 which is why I proposed this PR so we can choose node version for RPM-based systems too. It simply uses pub_version instead of only pub.

Let me know

[juniorsysadmin]

@tsde Would you be able to fix up the linting and RSpec tests? You can probably leave the default version number, as we'll need to bump the module version when that happens.

i'm just wildly confused that 0.10 is the default anywhere…

Um yeah, 0.12 was new at the time, then things were uncertain with the fork off to iojs, ...

[juniorsysadmin]

Not strictly related but you might to double-check what happens with 4.x and the npm package and adjust the README accordingly (See #165 )

[tsde]

@juniorsysadmin Just fix the linting and added a check to ensure that $npm_package_ensure is set to absent when using Nodesource repositories. This should resolve #165. Also a default case clause has been added. Let me know if you find the changes acceptable.

I'm working at fixing rspec asap.

[juniorsysadmin]

Just fix the linting and added a check to ensure that $npm_package_ensure is set to absent when using Nodesource repositories. This should resolve #165.

Just looked at the #165 again - npm_package_ensure already defaults to false for most operating systems and this I'll need to investigate that one further, so perhaps leave off the enforcement of npm_package_ensure.

Also, would it be more maintainable to move this logic (or some of it) to nodesource.pp rather than init.pp?

[tsde]

so perhaps leave off the enforcement of npm_package_ensure.

Ok, I've just reverted my changes on this. I think, we face the same kind of issue when you try to set to present the nodejs-dev package while using the Nodesource repos for Debian. As stated here, they don't use different packages for nodejs, nodejs-dev and npm. Everything is bundled into one package (for RedHat, it seems it's not the case for the devel package). Indeed, this'll need to be investigated further through a different PR.

I propose that this PR stays focused only on the ability to choose the node version for RedHat-based systems. I'm still working on fixing rspec at the meantime.

Also, would it be more maintainable to move this logic (or some of it) to nodesource.pp rather than init.pp?

Maybe. I'm used to put all class parameters checks into the init.pp file so checks happen earlier in the process. Not sure what Puppet recommends about this ?

[tsde]

@juniorsysadmin Just fixed the rspec tests. Let me know if everything seems fine (move checks to nodesource.pp or not ?)

[juniorsysadmin]

Let me know if everything seems fine (move checks to nodesource.pp or not ?)

Up to you (could probably be done as another PR). You've updated some tests, and CI is green, so I'm okay with merging.

[tsde]

@juniorsysadmin Great ! I'm OK with merging now. As you said, we can rethink checks placement in another PR

[igalic]

aaand done! thanks @tsde!