(MODULES-8107) - Support added for Ubuntu 18.04. #1850

david22swan · 2018-11-22T15:19:42Z

PR made to finialize work started by community member bastelfreak, (#1809).
Includes work done to migrate spec classes tests to rspec-puppet-facts.

ekohl · 2018-11-22T15:36:19Z

manifests/params.pp

@@ -354,7 +381,7 @@
    $secpcrematchlimit = 1500
    $secpcrematchlimitrecursion = 1500
    $modsec_secruleengine = 'On'
-    if $::operatingsystem == 'Debian' and versioncmp($::operatingsystemrelease, '9') >= 0 {
+    if ($::operatingsystem == 'Debian' and versioncmp($::operatingsystemrelease, '9') >= 0) or ($::operatingsystem == 'Ubuntu' and versioncmp($::operatingsystemrelease, '18.04') >= 0) {


Makes me think a helper function in stdlib could useful: if os_version_gt('Debian', '9') or os_version_gt('Ubuntu', '18.04') {. Might write up a patch for that.

puppetlabs/puppetlabs-stdlib#972

ekohl · 2018-11-22T15:37:45Z

spec/acceptance/mod_passenger_spec.rb

-        passenger_ruby = '/usr/bin/ruby'
-      end
+      passenger_root = '/usr/lib/ruby/vendor_ruby/phusion_passenger/locations.ini'
+      passenger_default_ruby = '/usr/bin/ruby'
    when 'Debian'


Since Wheezy is not mentioned in metadata.json you could simplify just static variables.

Removed the version case statement, just static variables now

ekohl · 2018-11-22T15:40:13Z

spec/acceptance/mod_passenger_spec.rb

-             (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemrelease') == '16.04') ||
-             (fact('operatingsystem') == 'Debian' && fact('operatingsystemmajrelease') == '8') ||
-             (fact('operatingsystem') == 'Debian' && fact('operatingsystemmajrelease') == '9')
+      unless (fact('operatingsystem') == 'Debian' && fact('operatingsystemmajrelease') == '8') ||


This is essentially all supported distros in the Debian osfamily per metadata.json, right?

Is now checking if the os family is debian

ekohl · 2018-11-22T15:40:53Z

spec/acceptance/mod_passenger_spec.rb

-            end
+      end
+      expected_two = if (fact('operatingsystem') == 'Ubuntu') ||
+                        (fact('operatingsystem') == 'Debian' && fact('operatingsystemmajrelease') == '8') ||


Also all supported Debian distros?

ekohl · 2018-11-22T15:44:20Z

spec/classes/dev_spec.rb

+            it { is_expected.to contain_class('apache::params') }
+            it { is_expected.to contain_package('httpd-devel') }
+          end
+        when 'FreeBSD'


This and Gentoo can be folded into the base context

ekohl · 2018-11-22T15:44:51Z

spec/classes/dev_spec.rb

-      }
+        case facts[:os]['name']
+        when 'Debian'
+          context 'on a Debian OS' do


We already know this context so it's redundant (see line 4). I won't repeat this for every spec, but it's a repeated pattern.

Removed the redundant context's

david22swan · 2018-11-23T09:13:46Z

@ekohl Thank you for the review, think I've resolved all of your comment's if you have the time to take another look?

ekohl · 2018-11-23T11:12:55Z

spec/acceptance/vhost_spec.rb

@@ -1568,7 +1568,8 @@ class { 'apache': }
  # Limit testing to Debian, since Centos does not have fastcgi package.
  case fact('osfamily')
  when 'Debian'
-    next if fact('operatingsystemmajrelease') == '9' # Debian 9 does not support this fastcgi
+    next if (fact('operatingsystemmajrelease') == '9') || (fact('operatingsystemmajrelease') == '18.04') # Debian 9/Ubuntu 18.04 does not support this fastcgi


Could also be written as ['9', '18.04'].include?(fact('operatingsystemmajrelease')). I'd also rewrite the whole case to a simple if fact('osfamily') == 'Debian' && !['9', '18.04'].include?(fact('operatingsystemmajrelease')).

Have done as you suggested, thanks for the code.

ekohl · 2018-11-23T11:16:00Z

spec/classes/dev_spec.rb

@@ -1,92 +1,39 @@
 require 'spec_helper'

 describe 'apache::dev', type: :class do


Could remove the redundant type: class while you're at it.

ekohl · 2018-11-23T11:16:56Z

spec/classes/dev_spec.rb

+          it { is_expected.to contain_class('apache::params') }
+          it { is_expected.to contain_package('httpd-devel') }
+        when 'FreeBSD'
+          it { is_expected.to contain_class('apache::params') }


I think this could also be pulled out of the case. It's not listed under Ubuntu, but I'm pretty sure that also contains the params class :)

Have pulled it out.

ekohl · 2018-11-23T11:18:03Z

spec/classes/mod/fastcgi_spec.rb

+      context 'with all defaults' do
+        case facts[:os]['name']
+        when 'Debian'
+          context 'on a Debian OS' do


Context is redundant, also specified at line 5.

Have removed the unneeded context

ekohl · 2018-11-23T11:19:43Z

spec/classes/mod/suphp_spec.rb

+      case facts[:os]['family']
+      when 'Debian'
+        # suphp has been declared EOL and is no longer supported on these modules
+        if (facts[:os]['release']['major'].to_i < 16 && facts[:os]['name'] == 'Ubuntu') ||


Do you want to add an else condition to test it actually fails? There are also no distros listed in metadata that match these conditions so the content of this block is dead code.

Have just removed the section so it only runs on Redhat now.

ekohl · 2018-11-23T11:22:19Z

spec/spec_helper_local.rb

@@ -35,5 +35,5 @@
    }
  end

-  it { is_expected.to compile.with_all_deps }
+  # it { is_expected.to compile.with_all_deps }


Is this still intended? If so, perhaps just remove it?

Have removed it.

ekohl · 2018-11-23T11:23:48Z

spec/spec_helper_local.rb

@@ -19,7 +19,7 @@
  it { is_expected.to compile.with_all_deps }
 end

-shared_examples 'a mod class, without including apache' do
+shared_context 'a mod class, without including apache' do
  let :facts do
    {


Alternative: on_supported_os['debian-8-x86_64'].

@ekohl Not entirely sure what you mean in this comment??? Could you explain a bit?

You can also use rspec-facter-db explicitly to avoid duplication:

let(:facts) { on_supported_os['debian-8-x86_64'] }

ekohl · 2018-11-23T15:18:49Z

manifests/mod/security.pp

    apache::security::rule_link { $activated_rules: }
+


Trivial nit: this line could be removed

ekohl · 2018-11-23T15:19:25Z

manifests/params.pp

@@ -354,7 +381,7 @@
    $secpcrematchlimit = 1500
    $secpcrematchlimitrecursion = 1500
    $modsec_secruleengine = 'On'
-    if $::operatingsystem == 'Debian' and versioncmp($::operatingsystemrelease, '9') >= 0 {
+    if ($::operatingsystem == 'Debian' and versioncmp($::operatingsystemrelease, '9') >= 0) or ($::operatingsystem == 'Ubuntu' and versioncmp($::operatingsystemrelease, '18.04') >= 0) {


puppetlabs/puppetlabs-stdlib#972

ekohl · 2018-11-23T15:20:54Z

spec/acceptance/mod_php_spec.rb

-          it { is_expected.to contain '# somecontent' }
-        end
+
+      path = if (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemmajrelease') == '16.04') ||


Interesting that above it's a long if/else tree where here we define a path. Below it's duplicated. Perhaps it should be a let(:php_config_path) block?

Swapped to a let

Threw an error so swapped to a before

didn't work either. Have just reverted it to how it was.

ekohl · 2018-11-23T15:23:20Z

spec/acceptance/mod_suphp_spec.rb

+# As of 2013 suphp was declared EOL by it's creators and as such support for it has been removed from Ubunto 15.10/Debian 8 onward.
+# https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=738133
+# https://www.howtoforge.com/tutorial/perfect-server-debian-8-jessie-apache-bind-dovecot-ispconfig-3/2/#-install-suphp-optional-not-recommended
+describe 'apache::mod::suphp class', if:  (fact('operatingsystem') == 'Ubuntu' && (fact('operatingsystemmajrelease') < '15.10')) ||


These distros aren't listed in metadata.json so I wonder if it's even relevant to keep this. On the other hand, the unit tests are still run on RH so you might switch it to that.

Was hesitant on outright removing, but what the hell.

ekohl · 2018-11-23T15:23:43Z

spec/acceptance/vhost_spec.rb

@@ -964,7 +964,7 @@ class { 'apache': }
    describe file($ports_file) do
      it { is_expected.to be_file }
      if fact('osfamily') == 'RedHat' && fact('operatingsystemmajrelease') == '7' ||
-         fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemrelease') =~ %r{(14\.04|16\.04)} ||
+         fact('operatingsystem') == 'Ubuntu' ||
         fact('operatingsystem') == 'Debian' && fact('operatingsystemmajrelease') == '8' ||


Somewhat unrelated, but this is all the Debian versions.

Just checks if osfamily is debian now.

ekohl · 2018-11-23T15:24:58Z

spec/classes/mod/suphp_spec.rb

-        path: '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin',
-        is_pe: false,
-      }
+      context 'on a RedHat OS' do


Also a duplicated context from line 7 :)

ekohl · 2018-11-23T15:27:00Z

spec/defines/modsec_link_spec.rb

+      it { is_expected.to compile.with_all_deps }
+      case facts[:os]['family']
+      when 'RedHat'
+        context 'on RedHat based systems' do


Duplicated context :)

ekohl · 2018-11-26T11:44:42Z

spec/acceptance/mod_php_spec.rb

      end
    end

    context 'provide content and template config file' do
+      # does the following even makes sense? Why do we hardcode the php5 template?


I agree. If you remove content the perhaps it can be a test to override the template parameter but currently it doesn't do that. Now it's a duplicate of the provide custom config file test

Was originally a comment from bastelfreak, but looking it over I can't see one good reason for it to be there.

ekohl · 2018-11-26T11:49:29Z

spec/defines/fastcgi_server_spec.rb

-        }
+      case facts[:os]['family']
+      when 'RedHat'
+        context 'on RedHat based systems' do


Duplicated context

Removed Duplication

ekohl · 2018-11-26T11:51:39Z

spec/defines/fastcgi_server_spec.rb

+      case facts[:os]['family']
+      when 'RedHat'
+        context 'on RedHat based systems' do
+          it { is_expected.to contain_class('apache') }


This expectation can be pulled out of the case statement. An alternative is to put the variable path assignment in a case statement and have the same expectations for all.

Top two it's have been moved outside the case

ekohl · 2018-11-27T15:23:12Z

spec/acceptance/mod_dav_svn_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper_acceptance'
 require_relative './version.rb'

-describe 'apache::mod::dav_svn class', unless: (fact('operatingsystem') == 'OracleLinux' && fact('operatingsystemmajrelease') == '7') do
+describe 'apache::mod::dav_svn class', unless: stdlib::os_version_gte('OracleLinux', '7') do


Does this work? Can you refer to module functions in acceptance tests?

Nope, got too caught up in the new function. :(

We could introduce an equivalent spec helper function. Shouldn't be too hard and it does clean up the code.

I like the way you think :)

Just removed it from the test's. Kept running into errors during the compile process.

ekohl · 2018-11-27T15:23:48Z

spec/acceptance/mod_passenger_spec.rb

-            it { is_expected.to contain %(PassengerRuby "#{passenger_ruby}) }
-            it { is_expected.not_to contain '/PassengerDefaultRuby/' }
-          end
+          it { is_expected.to contain %(PassengerDefaultRuby "#{passenger_default_ruby}") }


This now matches the Ubuntu case

Have removed the case

ekohl · 2018-11-27T15:24:04Z

spec/acceptance/mod_php_spec.rb

-      if (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemmajrelease') == '16.04') ||
-         (fact('operatingsystem') == 'Debian' && fact('operatingsystemmajrelease') == '9')
-        describe file("#{$mod_dir}/php7.0.conf") do
+      if stdlib:os_version_gte('Ubuntu', '18.04')


Missing a : here

ekohl · 2018-11-27T15:24:17Z

spec/acceptance/mod_php_spec.rb

-        end
-      elsif fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemmajrelease') == '18.04'
+
+      if stdlib:os_version_gte('Ubuntu', '18.04')


Missing a :

ekohl · 2018-11-27T16:49:19Z

manifests/mod/dav_svn.pp

@@ -4,7 +4,7 @@
  Class['::apache::mod::dav'] -> Class['::apache::mod::dav_svn']
  include ::apache
  include ::apache::mod::dav
-  if($::operatingsystem == 'SLES' and $::operatingsystemmajrelease < '12'){
+  unless stdlib::os_version_gte('SLES', '12') {


I didn't namespace the function so it should be on the top level. Perhaps it should be?

Unfortunately the method seems to be causing error's on the unit test's :(

Failure/Error: it { is_expected.to contain_class('apache::params') } Puppet::PreformattedError: Evaluation Error: Unknown function: 'stdlib::os_version_gte'. (file: /Users/david.swan/GitHub/puppetlabs-apache/spec/fixtures/modules/apache/manifests/mod/fcgid.pp, line: 5, column: 6) on node david.swan-c02l57z6fft1

Might have to remove it.

If you call os_version_gte (without stdlib::) I'd expect it works.

Yeh that worked, though had to undo one of them, I'd accidentally swapped out an OS family.

Ok, it's throwing an unknown function error when the acceptance test's are run, tried a few things but nothing seemed to get rid of it so I just ripped it out.

That's probably because the acceptance test installs a released version. Since the function is not yet in a released version, it's a problem. It'd also be needed to raise the minimum version in metadata.json.

Yeh, that sounds right.
Apologies, but at this point I think I'm just gonna go ahead without it since it only resolved down to a single use in the end.

Oh yes, that totally makes sense. I often also don't want to depend on the very latest version of a very common module (which stdlib absolutely is) to give users a chance to upgrade. So I'm fine with leaving the cleanup to a future PR.

IS everything else good with my pr?? Any more changes you think are needed/beneficial???

ekohl

I think I'm done and this is ready to be merged :)

ekohl · 2018-11-29T14:04:02Z

spec/acceptance/mod_passenger_spec.rb

-  when 'Debian'
-    conf_file = "#{$mod_dir}/passenger.conf"
-    load_file = "#{$mod_dir}/zpassenger.load"
+describe 'apache::mod::passenger class', if: fact('osfamily') == 'Debian' do


I'd be nice if there were acceptance tests for RH since it's something we do rely on but right now the tests aren't executed for RH anyway, right?

Yeh, I looked through the history of the file and found that they were removed by @tphoney

installing the various mods in redhat is painful, hence the removal

david22swan · 2018-11-29T16:54:21Z

david22swan · 2018-11-29T16:54:41Z

Has passed experimental adhoc pipeline

PR made to finialize work started by community member bastelfreak, (puppetlabs#1809). Includes work done to migrate spec classes tests to rspec-puppet-facts. Co-authored-by: Tim Meusel <tim@bastelfreak.de>

HelenCampbell · 2018-11-30T13:32:09Z

Merged 👍 Super good job to everyone involved!

david22swan · 2018-11-30T13:57:03Z

Big thanks to both @bastelfreak, for doing the heavy lifting on getting this work done and @ekohl, for his his excellent reviews.

ekohl reviewed Nov 22, 2018

View reviewed changes

david22swan changed the title ~~(MODULES-8107) - Support added for Ubuntu 18.04.~~ (WIP) (MODULES-8107) - Support added for Ubuntu 18.04. Nov 22, 2018

david22swan force-pushed the ubuntu1804 branch from 3202b12 to cb2f0b3 Compare November 22, 2018 17:15

ekohl reviewed Nov 23, 2018

View reviewed changes

david22swan added the feature label Nov 23, 2018

david22swan force-pushed the ubuntu1804 branch from cb2f0b3 to bd45628 Compare November 23, 2018 15:17

ekohl reviewed Nov 23, 2018

View reviewed changes

david22swan force-pushed the ubuntu1804 branch 2 times, most recently from ab8e3d2 to c3735a3 Compare November 26, 2018 11:49

ekohl reviewed Nov 26, 2018

View reviewed changes

david22swan force-pushed the ubuntu1804 branch 3 times, most recently from 9766e17 to f9f0773 Compare November 26, 2018 14:44

ekohl reviewed Nov 27, 2018

View reviewed changes

david22swan force-pushed the ubuntu1804 branch from 4043488 to 75f0bc6 Compare November 27, 2018 16:33

ekohl reviewed Nov 27, 2018

View reviewed changes

david22swan force-pushed the ubuntu1804 branch 3 times, most recently from 7ee8277 to 30f87bf Compare November 29, 2018 11:51

ekohl mentioned this pull request Nov 29, 2018

Update params.pp #1853

Closed

ekohl approved these changes Nov 29, 2018

View reviewed changes

david22swan changed the title ~~(WIP) (MODULES-8107) - Support added for Ubuntu 18.04.~~ (MODULES-8107) - Support added for Ubuntu 18.04. Nov 29, 2018

david22swan force-pushed the ubuntu1804 branch from 30f87bf to 27406c2 Compare November 30, 2018 11:57

(MODULES-8107) - Support added for Ubuntu 18.04.

1ff11b0

PR made to finialize work started by community member bastelfreak, (puppetlabs#1809). Includes work done to migrate spec classes tests to rspec-puppet-facts. Co-authored-by: Tim Meusel <tim@bastelfreak.de>

david22swan force-pushed the ubuntu1804 branch from 27406c2 to 1ff11b0 Compare November 30, 2018 11:58

HelenCampbell merged commit dda155c into puppetlabs:master Nov 30, 2018

david22swan deleted the ubuntu1804 branch November 30, 2018 13:57

david22swan mentioned this pull request Nov 30, 2018

Add Ubuntu 18.04 support #1809

Closed

		@@ -1,92 +1,39 @@
		require 'spec_helper'

		describe 'apache::dev', type: :class do

(MODULES-8107) - Support added for Ubuntu 18.04. #1850

(MODULES-8107) - Support added for Ubuntu 18.04. #1850

Conversation

david22swan commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david22swan commented Nov 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david22swan commented Nov 29, 2018

david22swan commented Nov 29, 2018

david22swan commented Nov 23, 2018 •

edited

Loading