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 4 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 = undef
Copy link
Member

@juniorsysadmin juniorsysadmin Nov 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's clearer if you default to $::lsbdistcodename here and remove the pick() later, no?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per voxpupuli/puppet-nginx#1154 (comment), changing to pass $repo_release through unaltered and let apt module deal with undef release.

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 datatype to new parameter, and pass it straight through apt module in commits fdb655d 63c3d3c

$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 => pick($release, $::lsbdistcodename),
repos => 'main',
require => [
Package['apt-transport-https'],
Expand Down