Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance distro support #342

Closed
wants to merge 7 commits into from
Closed

Enhance distro support #342

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
dbb4eba
Add Debian 9 to params
fnoop Nov 18, 2017
0a95f33
Add a nodejs::repo_release parameter to specify alternative to lsbdis…
fnoop Nov 18, 2017
193da7e
Fix broken use of fact in parameter value
fnoop Nov 18, 2017
b553b86
Use pick to improve patch
fnoop Nov 18, 2017
40f96bb
Add repo_release note to README
fnoop Nov 20, 2017
fdb655d
Add type to repo_release parameter
fnoop Nov 20, 2017
63c3d3c
Pass repo_release param straight through apt module
fnoop Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions manifests/init.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Copy link
Member

Choose a reason for hiding this comment

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

Please add an entry to the README for this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added README entry
40f96bb

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a datatype for the new param?

Array $use_flags = $nodejs::params::use_flags,
) inherits nodejs::params {

Expand Down
3 changes: 2 additions & 1 deletion manifests/params.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
$repo_proxy_password = 'absent'
$repo_proxy_username = 'absent'
$repo_url_suffix = '0.10'
$repo_release = $::lsbdistcodename
$use_flags = ['npm', 'snapshot']

# The full path to cmd.exe is required on Windows. The system32 fact is only
Expand All @@ -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+)/ {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't versioncmp($::operatingsystemmajrelease, '7') >= 0 be better and more future proof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that would incorrectly catch the Ubuntu versions as well wouldn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

You're right. Early morning code reviews aren't the best idea :P

$legacy_debian_symlinks = false
$manage_package_repo = true
$nodejs_debug_package_name = 'nodejs-dbg'
Expand Down
1 change: 1 addition & 0 deletions manifests/repo/nodesource.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
$proxy_password = $nodejs::repo_proxy_password
$proxy_username = $nodejs::repo_proxy_username
$url_suffix = $nodejs::repo_url_suffix
$release = $nodejs::repo_release

case $::osfamily {
'RedHat': {
Expand Down
3 changes: 2 additions & 1 deletion manifests/repo/nodesource/apt.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
$ensure = $nodejs::repo::nodesource::ensure
$pin = $nodejs::repo::nodesource::pin
$url_suffix = $nodejs::repo::nodesource::url_suffix
$release = $nodejs::repo::nodesource::release

ensure_packages(['apt-transport-https', 'ca-certificates'])

Expand All @@ -21,7 +22,7 @@
},
location => "https://deb.nodesource.com/node_${url_suffix}",
pin => $pin,
release => $::lsbdistcodename,
release => $release,
repos => 'main',
require => [
Package['apt-transport-https'],
Expand Down