Enhance distro support #342
Conversation
| @@ -25,7 +26,7 @@ | |||
| if $::operatingsystemrelease =~ /^6\.(\d+)/ { | |||
| fail("The ${module_name} module is not supported on Debian Squeeze.") | |||
| } | |||
| elsif $::operatingsystemrelease =~ /^[78]\.(\d+)/ { | |||
| elsif $::operatingsystemrelease =~ /^[789]\.(\d+)/ { | |||
There was a problem hiding this comment.
Wouldn't versioncmp($::operatingsystemmajrelease, '7') >= 0 be better and more future proof?
There was a problem hiding this comment.
I think that would incorrectly catch the Ubuntu versions as well wouldn't it?
There was a problem hiding this comment.
This will likely break for the next version of Debian (10) as it will start to conflict with Ubuntu 10.x, logic should be broken off separately for Debian and Ubuntu.
There was a problem hiding this comment.
You're right. Early morning code reviews aren't the best idea :P
manifests/repo/nodesource/apt.pp
Outdated
|
|
||
| ensure_packages(['apt-transport-https', 'ca-certificates']) | ||
|
|
||
| include ::apt | ||
|
|
||
| if empty($release) { |
There was a problem hiding this comment.
Would pick($release, $::lsbdistcodename) be better here or is this another case of me missing anything? :)
There was a problem hiding this comment.
Didn't know about pick, you're right!
|
Note: the failing travis checks appears to be because of missing nodejs-dev package, but the managed nodejs repo doesn't seem to provide this package - headers are included in the main nodejs package by the look of it. |
| @@ -11,6 +11,7 @@ | |||
| $repo_proxy_password = 'absent' | |||
| $repo_proxy_username = 'absent' | |||
| $repo_url_suffix = '0.10' | |||
| $repo_release = undef | |||
There was a problem hiding this comment.
It's clearer if you default to $::lsbdistcodename here and remove the pick() later, no?
There was a problem hiding this comment.
But that fact is undefined on many OSes so I'm assuming he didn't want to break strict variables. On Debian it is defined so you can safely use it with pick()
There was a problem hiding this comment.
As per voxpupuli/puppet-nginx#1154 (comment), changing to pass $repo_release through unaltered and let apt module deal with undef release.
manifests/init.pp
Outdated
| @@ -23,6 +23,7 @@ | |||
| $repo_proxy_password = $nodejs::params::repo_proxy_password, | |||
| $repo_proxy_username = $nodejs::params::repo_proxy_username, | |||
| $repo_url_suffix = $nodejs::params::repo_url_suffix, | |||
| $repo_release = $nodejs::params::repo_release, | |||
There was a problem hiding this comment.
Please add an entry to the README for this
There was a problem hiding this comment.
Added README entry
40f96bb
There was a problem hiding this comment.
Can you add a datatype for the new param?
|
Have stolen the relevant bits in this change and put them in #349 |
Add explicit Debian 9 (stretch) support, and add support for Ubilinux.
Ubilinux 4 (that supports all variants of the Up boards) is a derivative of Debian 9. However it reports dolcetto as the lsbdistcodename instead of stretch so the nodejs repo fails. This PR adds support for a $nodejs::repo_release parameter that allows the distro code name to be specified rather than blindly taking it from lsbdistcodename.
eg.: