Add Ubuntu 18.04 support #1809

bastelfreak · 2018-08-04T22:22:09Z

I'm currently implementing Ubuntu 18.04 support. Early feedback is highly appreciated :)

This typo is present since ~ 7 months. puppetlabs@02919a8#diff-54659813dfca25ca1f5df88aaccb09ecL199 Nobody noticed it because there are no acceptance tests running for everything where unless does not match. I discovered this in puppetlabs#1809

HelenCampbell

Looking good, just a few general comments at the minute. Awesome job on the test updates!
I've been running this through a test pipeline and it's passing on Debian 8 and Redhat 7 so far, failing on Ubuntu 16 due to that policy.rc issue.

HelenCampbell · 2018-08-07T11:27:31Z

Gemfile

@@ -43,6 +43,9 @@ group :system_tests do
  gem "beaker-pe",                                                               require: false
  gem "beaker-hostgenerator"
  gem "beaker-rspec"
+  gem 'rbnacl', '~> 4',                     :require => false if RUBY_VERSION >= '2.2.6'


What are you using these Gems for? The Gemfile will be overwritten by a pdk run so these changes would eventually get removed. If you want them permanent they'll have to be moved to .sync.yml.

These are needed if you use modern SSH keys (ed25519) and want to run acceptance tests.

The commit already has some information:
7703346

Thanks! We will look in to adding these to puppet-module-gems.

We changed it to allow newer version of rbnacl as well: voxpupuli/modulesync_config@58f32bf

Can you remove these new Gems for merge? We will be adding them to the puppet-module-gems in due time and these would conflict.

HelenCampbell · 2018-08-07T11:28:06Z

manifests/mod/dav_svn.pp

@@ -13,7 +13,7 @@

  ::apache::mod { 'dav_svn': }

-  if $::osfamily == 'Debian' and ($::operatingsystemmajrelease != '6' and $::operatingsystemmajrelease != '10.04' and $::operatingsystemrelease != '10.04' and $::operatingsystemmajrelease != '16.04') {
+  if $::osfamily == 'Debian' and ($::operatingsystemmajrelease != '6' and !($::operatingsystemmajrelease in ['10.04', '16.04', '18.04'])) {


We have long dropped support for 10.04 so you can remove it here :)

HelenCampbell · 2018-08-07T11:42:37Z

spec/acceptance/mod_suphp_spec.rb

+
+    # without this apache fails to start -> installation of mod-php-something fails because it reloads apache to enable the module. WTF Ubuntu
+    # exit codes are documented at https://askubuntu.com/a/365912. Default for docker images is 101
+    shell("sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d")


Would it be worth adding this code into the spec_helper_acceptance since you reuse it a couple of times? Also gross to these bugs :| I am running this PR through an ad-hoc pipeline and am hitting this issue on Ubuntu 16 as you seem to be.

Did you run the master branch and that failed, or my version?

Your version. I'll re-run again this morning with your recent changes :)

So I've found locally that my tests will run if I change to
before :all do on(host, "if [ -a '/usr/sbin/policy-rc.d' ]; then sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d; fi") if ['16.04', '18.04'].include?(fact('operatingsystemmajrelease')) end
instead of doing it in the describe.

Ew that didn't format well.. To summarise I just moved it into a 'before :all' clause. That seems to be why I kept hitting into that issue where the tests bombed out instead of running.

I added it like this:

spec/acceptance/mod_suphp_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/acceptance/mod_suphp_spec.rb b/spec/acceptance/mod_suphp_spec.rb index dbf3a28c..4a1388b8 100644 --- a/spec/acceptance/mod_suphp_spec.rb +++ b/spec/acceptance/mod_suphp_spec.rb @@ -1,8 +1,8 @@ require 'spec_helper_acceptance' # why is that not executed on 16.04 -describe 'apache::mod::suphp class', if: (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemmajrelease') != '16.04') do - if ['16.04', '18.04'].include?(fact('operatingsystemmajrelease')) +describe 'apache::mod::suphp class' do + before :all do # this policy defaults to 101. it prevents newly installed services from starting # it is useful for containers, it prevents new processes during 'docker build' # but we actually want to test the services and this should not behave like docker @@ -10,8 +10,9 @@ describe 'apache::mod::suphp class', if: (fact('operatingsystem') == 'Ubuntu' && # without this apache fails to start -> installation of mod-php-something fails because it reloads apache to enable the module. WTF Ubuntu # exit codes are documented at https://askubuntu.com/a/365912. Default for docker images is 101 - shell("sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d") + on(host, "if [ -a '/usr/sbin/policy-rc.d' ]; then sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d; fi") if ['16.04', '18.04'].include?(fact('operatingsystemmajrelease')) end + # mod php isn't supported for ubuntu 1.04 next if fact('operatingsystemmajrelease') == '18.04' context 'default suphp config' do

but I get this error:

Failures: 1) apache::mod::suphp class default suphp config succeeds in puppeting suphp Failure/Error: on(host, "if [ -a '/usr/sbin/policy-rc.d' ]; then sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d; fi") if ['16.04', '18.04'].include?(fact('operatingsystemmajrelease')) NoMethodError: private method `exec' called for Host "":Serverspec::Type::Host

HelenCampbell · 2018-08-07T11:57:02Z

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'd agree with you, not sure there's much point to this test

rcousens · 2018-08-13T04:48:00Z

spec/acceptance/mod_php_spec.rb

+        shell("sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d")
+      end
+      if fact('operatingsystemmajrelease') == '18.04'
+        # Again Ubuntu. W T F.


I'd just like to point out this is an upstream Debian bug, so your 'WTF Ubuntu' is misdirected and misplaced. Regardless your personal feelings and reactions should most likely never appear in public code in the first place even if misguided.

sorry, that was written after hours of personal frustration, I removed it

I know the feeling, thanks for cleaning it up!

tphoney · 2018-08-14T13:10:41Z

Having a look, what a PR 😮

HelenCampbell · 2018-08-14T13:30:08Z

.sync.yml

@@ -5,6 +5,8 @@
  docker_sets:
    - set: docker/centos-7
    - set: docker/ubuntu-14.04
+    - set: docker/ubuntu-16.04


Can we get both Ubuntu16&18 removed from here and the travis file.

HelenCampbell · 2018-08-14T13:33:50Z

.travis.yml

      dist: trusty
-      env: PUPPET_INSTALL_TYPE=agent BEAKER_debug=true BEAKER_PUPPET_COLLECTION=puppet5 BEAKER_set=docker/centos-7
+      env: PUPPET_INSTALL_TYPE=agent BEAKER_debug=true BEAKER_PUPPET_COLLECTION=puppet5 BEAKER_setfile=centos7-64{hypervisor=docker}


Can we get this change reverted back to BEAKER_set=docker/centos-7 and BEAKER_set=docker/ubuntu-14.04 please? (Work is ongoing to implement this functionality.)

HelenCampbell · 2018-08-14T13:58:18Z

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 there any reason this is commented out?

It didn't work in combination with rspec-puppet-facts and also felt redundant. All the mod files should already include it { is_expected.to compile.with_all_deps }

ekohl · 2018-08-15T16:38:40Z

manifests/mod/dav_svn.pp

@@ -13,7 +13,7 @@

  ::apache::mod { 'dav_svn': }

-  if $::osfamily == 'Debian' and ($::operatingsystemmajrelease != '6' and $::operatingsystemmajrelease != '10.04' and $::operatingsystemrelease != '10.04' and $::operatingsystemmajrelease != '16.04') {
+  if $::osfamily == 'Debian' and ($::operatingsystemmajrelease != '6' and !($::operatingsystemmajrelease in ['16.04', '18.04'])) {


The $::operatingsystemmajrelease != '6' looks redundant. I hope Debian 6 isn't supported. It's also pretty hard to parse. So the result is that for Ubuntu 14.04 it's undef but everything else has dav_svn_authz_svn.load?

I tried to not rewrite it because that would cause an even bigger diff. A general cleanup is advised after this PR + #1804 got merged.

cleaned it up

ekohl · 2018-08-15T16:39:23Z

manifests/mod/security.pp

-  unless $::operatingsystem == 'SLES' { apache::security::rule_link { $activated_rules: } }
+  # the rule setup on ubuntu 18.04 is completely different,
+  # the default rules used within this module aren't available
+  unless $::operatingsystem == 'SLES' or $facts['os']['release']['major'] == '18.04' { apache::security::rule_link { $activated_rules: } }


Might be a bit more readable to put the body on a new line. It's very long.

ekohl · 2018-08-15T16:40:20Z

manifests/mod/suphp.pp

@@ -1,5 +1,8 @@
 class apache::mod::suphp (
 ){
+  if ($facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '18.04') >= 0) {
+    fail('mod_fastcgi is no longer supported on Ubuntu 18.04 and above. Please use mod_proxy_fcgi')


This message doesn't make sense to me. Does suphp depend on mod_fastcgi? Copy paste error?

good spot, copy and paste error. I will fix it

ekohl · 2018-08-15T16:41:09Z

manifests/params.pp

@@ -263,6 +263,33 @@
        'wsgi'                  => 'libapache2-mod-wsgi',
        'xsendfile'             => 'libapache2-mod-xsendfile',
      }
+    } elsif $facts['os']['release']['major'] == '18.04' {
+      # major.minor version used since Debian stretch and Ubuntu Xenial
+      $php_version = '7.2' # different to Ubuntu 18.04


Did you mean 16.04 here?

ekohl · 2018-08-15T16:41:33Z

metadata.json

@@ -21,15 +21,13 @@
    {
      "operatingsystem": "RedHat",
      "operatingsystemrelease": [
-        "5",


Is this the right PR to drop these?

cherry-picked into #1824

ekohl · 2018-08-15T16:48:01Z

spec/acceptance/mod_passenger_spec.rb

@@ -171,10 +181,9 @@ class { 'apache::mod::passenger': }
      expected_one = [%r{Apache processes}, %r{Nginx processes}, %r{Passenger processes}]
      # passenger-memory-stats output on newer Debian/Ubuntu verions do not contain
      # these two lines
-      unless (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemrelease') == '14.04') ||
-             (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemrelease') == '16.04') ||
+      unless (fact('operatingsystem') == 'Ubuntu' && ['14.04', '16.04', '18.04'].include?(fact('operatingsystemrelease'))) ||


Isn't this basically every supported Ubuntu version? I'd even thing Debian 9 would fail here so I think this whole block can be removed if it's no longer included.

ekohl · 2018-08-15T16:49:30Z

spec/acceptance/mod_passenger_spec.rb

            # spacing may vary
            expect(r.stdout).to match(%r{[\-]+ General information [\-]+})
          end
        end
-        expected_two = if fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemrelease') == '14.04' ||
-                          (fact('operatingsystem') == 'Ubuntu' && fact('operatingsystemrelease') == '16.04') ||
+        expected_two = if fact('operatingsystem') == 'Ubuntu' && ['14.04', '16.04', '18.04'].include?(fact('operatingsystemrelease')) ||


I think you can drop the Ubuntu version checks here as well.

ekohl · 2018-08-15T16:53:16Z

spec/acceptance/mod_suphp_spec.rb

+    shell("sed -i 's/^exit.*/exit 0/' /usr/sbin/policy-rc.d")
+  end
+  # mod php isn't supported for ubuntu 1.04
+  next if fact('operatingsystemmajrelease') == '18.04'


It's better to do this in the describe like 16.04.

ekohl · 2018-08-15T16:53:36Z

spec/acceptance/mod_suphp_spec.rb

@@ -1,6 +1,19 @@
 require 'spec_helper_acceptance'

+# why is that not executed on 16.04


And why is it only executed on Ubuntu setups? Do other distros not ship it?

ekohl · 2018-08-15T16:54:54Z

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('operatingsystemrelease') =~ %r{(14\.04|16\.04|18\.04)} ||


I think this is all supported Ubuntu versions.

mmoll · 2018-11-06T03:03:41Z

this needs a rebase...
without having a closer look: what's backwards-incompatible here?

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

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-authered-by: Tim Meusel <tim@bastelfreak.de>

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 · 2018-11-30T14:00:47Z

@bastelfreak Closing as a pr finishing this work has been merged in:
#1850
Thanks for the great work you put into it though

This typo is present since ~ 7 months. puppetlabs@02919a8#diff-54659813dfca25ca1f5df88aaccb09ecL199 Nobody noticed it because there are no acceptance tests running for everything where unless does not match. I discovered this in puppetlabs#1809

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>

This typo is present since ~ 7 months. puppetlabs@02919a8#diff-54659813dfca25ca1f5df88aaccb09ecL199 Nobody noticed it because there are no acceptance tests running for everything where unless does not match. I discovered this in puppetlabs#1809

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>

bastelfreak force-pushed the ubuntu1804 branch 7 times, most recently from 40f911b to c8845c6 Compare August 5, 2018 18:01

bastelfreak mentioned this pull request Aug 6, 2018

fix typo in the tests #1810

Merged

bastelfreak force-pushed the ubuntu1804 branch 2 times, most recently from bf2bf7f to b51e3f7 Compare August 6, 2018 19:34

bastelfreak changed the title ~~[WIP] Add Ubuntu 18.04 support~~ Add Ubuntu 18.04 support Aug 6, 2018

HelenCampbell reviewed Aug 7, 2018

View reviewed changes

david22swan added the feature label Aug 7, 2018

bastelfreak force-pushed the ubuntu1804 branch from b51e3f7 to e6016f6 Compare August 7, 2018 14:10

rcousens reviewed Aug 13, 2018

View reviewed changes

bastelfreak force-pushed the ubuntu1804 branch 2 times, most recently from cfba469 to 82e2fe9 Compare August 14, 2018 07:08

HelenCampbell reviewed Aug 14, 2018

View reviewed changes

bastelfreak force-pushed the ubuntu1804 branch 2 times, most recently from d95cb1e to 4a89433 Compare August 14, 2018 13:38

HelenCampbell added the backwards-incompatible label Aug 14, 2018

HelenCampbell reviewed Aug 14, 2018

View reviewed changes

ekohl reviewed Aug 15, 2018

View reviewed changes

bastelfreak force-pushed the ubuntu1804 branch from 4a89433 to f82da3f Compare August 16, 2018 07:37

bastelfreak force-pushed the ubuntu1804 branch 2 times, most recently from ba6be49 to df5e3b3 Compare August 30, 2018 09:17

bastelfreak mentioned this pull request Aug 30, 2018

remove EOL RHEL 5 from metadata.json #1824

Closed

bastelfreak added 8 commits September 29, 2018 01:04

migrate vhost spec tests to rspec-puppet-facts

1bfe062

migrate dev spec tests to rspec-puppet-facts

693f7c1

migrate fastcgi spec tests to rspec-puppet-facts

e1f363a

disable legacy shared_example

041836a

migrate php spec tests to rspec-puppet-facts

d8f8a81

migrate suphp spec tests to rspec-puppet-facts

b780d65

migrate mod-security spec tests to rspec-puppet-facts

e9b5d42

migrate mod-passenger spec tests to rspec-puppet-facts

500d9e1

bastelfreak force-pushed the ubuntu1804 branch from 8ac37b9 to 500d9e1 Compare September 28, 2018 23:05

mmoll mentioned this pull request Nov 6, 2018

ubuntu 18.04 has php 7.2 and libapache2-mod-security2 #1842

Closed

david22swan mentioned this pull request Nov 22, 2018

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

Merged

david22swan closed this Nov 30, 2018

		@@ -1,6 +1,19 @@
		require 'spec_helper_acceptance'

		# why is that not executed on 16.04

Add Ubuntu 18.04 support #1809

Add Ubuntu 18.04 support #1809

Uh oh!

Conversation

bastelfreak commented Aug 4, 2018

Uh oh!

HelenCampbell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tphoney commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mmoll commented Nov 6, 2018 •

edited

Loading