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

Fix application of augeas changes on a Centos7/RHEL7 machine. #46

Merged
merged 3 commits into from
Aug 31, 2018

Conversation

bekarau
Copy link
Contributor

@bekarau bekarau commented Aug 29, 2018

Pull Request (PR) description

Fixes the application of augeas changes on a Centos7/RHEL7 installation.

This Pull Request (PR) fixes the following issues

Fixes #11

Further comments

As I've not tested the newer augeas changes against older releases, I've put an explicit test for major release 7 or greater (assuming it won't change in the future either).

],
require => File["/etc/pam.d/${rule}"],
notify => Service['sshd'],
if ($::facts['os']['release']['major'] >= 7) {
Copy link
Member

Choose a reason for hiding this comment

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

please remove ::

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should now be done.

@bastelfreak bastelfreak added bug Something isn't working needs-work not ready to merge just yet labels Aug 29, 2018
@bastelfreak
Copy link
Member

Thanks for the PR! Please take a look at the failing travis tests.

@bastelfreak bastelfreak added tests-fail and removed needs-work not ready to merge just yet labels Aug 30, 2018
@bastelfreak
Copy link
Member

there are still some travis tests failing, please have a look. Also please check the email address used in the commits, it is not associated with your github account.

@bekarau
Copy link
Contributor Author

bekarau commented Aug 31, 2018

Work vs Home email. Added work email, so should be there now.

@bastelfreak
Copy link
Member

Thanks!

@bastelfreak bastelfreak merged commit 8ca809a into voxpupuli:master Aug 31, 2018
],
require => File["/etc/pam.d/${rule}"],
notify => Service['sshd'],
if ($facts['os']['release']['major'] >= 7) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be restricted to RedHat?

Copy link
Member

Choose a reason for hiding this comment

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

good catch. @bekarau what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The restriction to redhat is done in pam.pp; where this module is called from:

  case $::operatingsystem {
    /Debian|Ubuntu/ : {
        googleauthenticator::pam::debian {$name:
          ensure => $ensure,
          mode   => $mode,
        }
      }
    /RedHat|CentOS/ : {
        googleauthenticator::pam::redhat {$name:
          ensure => $ensure,
          mode   => $mode,
        }
      }
    default         : { fail("not supported on ${::operatingsystem}") }
  }

Copy link
Member

Choose a reason for hiding this comment

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

Ah, my bad, I didn't see this was in redhat.pp!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:) no problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pam no longer supports @include?
3 participants