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.

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

Remove EOL operating systems, Legacy facts. #343

Merged
merged 1 commit into from
Dec 4, 2017

Conversation

juniorsysadmin
Copy link
Member

@juniorsysadmin juniorsysadmin commented Nov 29, 2017

  • Update the repo_url_suffix table in the README, dropping EOL
    operating systems and adding new ones
  • Drop explicit handling of Debian 6 and 7 (EOL)
  • Drop explicit handling of Ubuntu 10.04 and 12.04 (EOL)
  • Drop explicit handling of EOL Fedora versions
  • Drop explicit handling of RHEL5 (EOL)
  • Add explicit handling of Debian 9
  • Replace most Legacy facts with Modern facts using the new facts hash
    syntax. This explicitly drops support for Facter versions less than 3,
    which are not supported under Puppet4/5 anyway.
  • Drop explicit support for Amazon Linux versions less than 2015.03.
    Amazon Linux is rolling distribution, and earlier versions did not have
    Facter 3
  • Use the Modern system32 fact

Also:

  • Remove the old Puppetlabs footer from the README Reference CONTRIBUTING.md instead
  • Perform minor doc fixes
  • Use less granular error handling

@juniorsysadmin
Copy link
Member Author

I haven't replaced $::lsbdistcodename
$facts['os'['distro']['codename'] (to replace $::lsbdistcodename in nodesource/apt.pp) is not handled by rspec-puppet-facts, so will need mocked values.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks like a good start to clean up the module.

$repo_class = '::nodejs::repo::nodesource'
}
elsif $::operatingsystemrelease =~ /^16.04$/ {
elsif $facts['os']['release']['full'] =~ /^14.04$|^16.04$/ {
Copy link
Member

Choose a reason for hiding this comment

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

How about /^1[46]\.04$/ for the regex?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

}

elsif $::operatingsystemrelease =~ /^6\.(\d+)/ {
if $facts['os']['release']['full'] =~ /^6\.(\d+)/ {
Copy link
Member

Choose a reason for hiding this comment

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

The major should be there in facts too and doesn't need a regex. You can even simplify the later code to use those facts in $dist_version and $name_string.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

# On EL 5, EPEL needs to be applied first
Class['::epel'] -> Class['::nodejs::repo::nodesource::yum']
}

}
'Linux': {
Copy link
Member

Choose a reason for hiding this comment

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

I think you can just remove the whole Linux block and allow it to fall back to the generic default.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

facts[:operatingsystemrelease] == '10.04' ||
%w[7 8].include?(facts[:operatingsystemmajrelease])
)
if facts[:os]['family'] == 'Debian' &&
Copy link
Member

Choose a reason for hiding this comment

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

I'd extract this into a separate variable since it's reused a lot.

end

['5.0', '6.0', '7.0', '25'].each do |operatingsystemrelease|
['6.0', '7.0', '27'].each do |operatingsystemrelease|
Copy link
Member

Choose a reason for hiding this comment

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

This and a lot below should be in on_supported_os too. Could we reuse that instead of duplicating facts?

Copy link
Member Author

@juniorsysadmin juniorsysadmin Dec 3, 2017

Choose a reason for hiding this comment

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

Hmm maybe. We also would need to filter CentOS/OracleLinux/Scientific Linux as we only want EL6/7 types tested once.

README.md Outdated
module is installed.

If using CentoOS/RHEL 5/6/7 and you wish to install Node.js from EPEL rather
If using CentoOS/RHEL 6/7 and you wish to install Node.js from EPEL rather
Copy link
Member

Choose a reason for hiding this comment

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

CentOS

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

fail("The ${module_name} module is not supported on Debian Squeeze.")
}
elsif $::operatingsystemrelease =~ /^[78]\.(\d+)/ {
if $facts['os']['release']['full'] =~ /^[89]\.(\d+)/ {
Copy link
Member

Choose a reason for hiding this comment

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

Should this use ['major'] =~ /^[89]$/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -102,7 +61,7 @@
'RedHat': {
$legacy_debian_symlinks = false

if $::operatingsystemrelease =~ /^[5-7]\.(\d+)/ {
if $facts['os']['release']['full'] =~ /^[6-7]\.(\d+)/ {
Copy link
Member

Choose a reason for hiding this comment

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

Also use major?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -227,43 +185,10 @@
$repo_class = undef
}
'Linux': {
Copy link
Member

Choose a reason for hiding this comment

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

I think this adds little over the default failure

Copy link
Member Author

Choose a reason for hiding this comment

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

Have dropped it.

- Update the `repo_url_suffix` table in the README, dropping EOL
operating systems and adding new ones
- Drop explicit handling of Debian 6 and 7 (EOL)
- Drop explicit handling of Ubuntu 10.04 and 12.04 (EOL)
- Drop explicit handling of EOL Fedora versions
- Drop explicit handling of RHEL5 (EOL)
- Add explicit handling of Debian 9
- Replace most Legacy facts with Modern facts using the new facts hash
syntax. This explicitly drops support for Facter versions less than 3,
which are not supported under Puppet4/5 anyway.
- Drop explicit support for Amazon Linux versions less than 2015.03.
Amazon Linux is rolling distribution, and earlier versions did not have
Facter 3
- Use the Modern system32 fact

Also:
- Remove the old Puppetlabs footer from the README. Reference
CONTRIBUTING.md instead
- Perform minor doc fixes
- Use less granular error handling
@bastelfreak bastelfreak merged commit 1ab821c into voxpupuli:master Dec 4, 2017
@juniorsysadmin juniorsysadmin deleted the cleanup branch December 4, 2017 10:27
This was referenced Dec 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants