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

MODULES-10586 Centos 8: wrong package used to install mod_authnz_ldap #2021

Merged
merged 3 commits into from
Jun 29, 2020
Merged

Conversation

farebers
Copy link

see https://tickets.puppetlabs.com/browse/MODULES-10586

the change was tested on a fresh centos 8 install

note on unit tests:

  • I edited the tests 'default configuration with parameters on a RedHat OS' in authnz_ldap_spec.rb and ldap_spec.rb (updated the facts operatingsystemrelease line to '8') in the puppetlabs module (not containing my change)
  • so those tests should have failed but didn't

so I was unable to provide a sensible unit test

@farebers farebers requested a review from a team as a code owner April 29, 2020 12:14
@farebers farebers changed the title MODULES-10586 Centos 8: wrong package used to install mod_ldap MODULES-10586 Centos 8: wrong package used to install mod_authnz_ldap Apr 29, 2020
@sanfrancrisko
Copy link
Contributor

sanfrancrisko commented Apr 30, 2020

Hi @farebers,

Firstly, thank you for the fix and highlighting the gap in the test coverage to capture this. As you pointed out, the unit tests did not catch this failure. We can certainly extend spec/classes/mod/authnz_ldap_spec.rb to verify that we are using the correct package name on RHEL systems based on their major versions, but, unfortunately this will not catch the failure until the manifest is actually compiled on an OS.

I think the best thing is to to add an acceptance test for this particular scenario in a file called spec/acceptance/mod_authnz_ldap_spec.rb, something along the lines of this:

require 'spec_helper_acceptance'
apache_hash = apache_settings_hash

describe 'apache::mod_authnz_ldap' do
  context 'Default mod_authnz_ldap module installation' do
    pp = <<-MANIFEST
      class { 'apache': }
      class { 'apache::mod::auth::authnz_ldap': }
    MANIFEST

    it 'succeeds in installing the mod_authnz_ldap module' do
      apply_manifest(pp, catch_failures: true)

      describe file("#{apache_hash['mod_dir']}/authnz_ldap.load") do
        it { is_expected.to contain 'mod_authnz_ldap.so' }
      end
    end
  end
end

This will ensure that the installation of the package works correctly.

I have been taking a look at how we could enhance the unit tests, doing something like this:

on_supported_os.each do |os, facts|
      context "On #{os}" do
        let :facts do
          facts
        end

        it { is_expected.to contain_class('apache::params') }
        it { is_expected.to contain_class('apache::mod::ldap') }
        it { is_expected.to contain_apache__mod('authnz_ldap') }

        if facts[:os]['name'] == 'Redhat' and facts[:os]['version'].to_i < 7
          it { is_expected.to contain_package('mod_authz_ldap') }
        elsif facts[:os]['name'] == 'Redhat' and facts[:os]['version'].to_i > 6
          it { is_expected.to contain_package('mod_ldap')}
        end

...but there's a few issues I'm running in to with this approach.

I think the path of least resistance would be to add an acceptance test, similar to above. Let me know if I can be of any further help.

spec/classes/mod/authnz_ldap_spec.rb Outdated Show resolved Hide resolved
spec/classes/mod/authnz_ldap_spec.rb Outdated Show resolved Hide resolved
spec/classes/mod/authnz_ldap_spec.rb Show resolved Hide resolved
spec/classes/mod/authnz_ldap_spec.rb Outdated Show resolved Hide resolved
Christian Klein and others added 3 commits June 29, 2020 18:40
Enhance spec/classes/mod/authnz_ldap_spec.rb unit tests

Expand the RHEL tests to ensure that the 'mod_authnz_ldap' pkg is
present on versions >= 7 and 'mod_ldap' is present on all other
versions.

An acceptance test executing against an actual RHEL instance will
be the only way to catch any divergence in package names in the
future. The unit tests will need to be updated with that change.

There seems to be more effort required to get this running against
all supported OSs. This was beyond the scope of this PR.

Add acceptance test to ensure correct authnz_ldap pkg is installed

Fix typos and nesting issues in mod_authnz_ldap_spec.rb

Restrict mod_authnz_ldap_spec.rb tests to RHEL 7.x, 8.x

Apply suggestions from code review

Co-authored-by: Ewoud Kohl van Wijngaarden <[email protected]>

Fix authnz_ldap_spec.rb test expectations

Some additional cleanup and filtering of scenarios too.

Move filtering rule to context block
@michaeltlombardi michaeltlombardi merged commit 8a8d544 into puppetlabs:master Jun 29, 2020
mwhahaha added a commit to mwhahaha/puppetlabs-apache that referenced this pull request Jul 26, 2021
This change updates the version handling to better future proof the
logic when newer versions are shipped. This should prevent having to add
things like puppetlabs#2063, puppetlabs#2021, puppetlabs#2038, etc.
daveseff pushed a commit to daveseff/puppetlabs-apache that referenced this pull request Jul 19, 2022
This change updates the version handling to better future proof the
logic when newer versions are shipped. This should prevent having to add
things like puppetlabs#2063, puppetlabs#2021, puppetlabs#2038, etc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants