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

Refactors #172

Merged
merged 8 commits into from
Dec 24, 2017
Merged

Refactors #172

merged 8 commits into from
Dec 24, 2017

Conversation

Phil-Friderici
Copy link
Contributor

Using iterations with each instead of resource declarations for each single common* file on Linux systems.

@Phil-Friderici
Copy link
Contributor Author

As this is a quite complex refactor I have taken extra care to NOT change the existing spec tests.
The only change needed does adjust the internaly used resource names for $common_session_noninteractive_file on Debian/Ubuntu.

@Phil-Friderici
Copy link
Contributor Author

Added some refactoring for the common* files in #172.

@Phil-Friderici
Copy link
Contributor Author

If you like this refactoring I will move as much data into Hiera 5 module data to clean up the code even more.

@Phil-Friderici Phil-Friderici mentioned this pull request Dec 19, 2017
12 tasks
@@ -136,324 +114,98 @@
mode => $pam_d_sshd_mode,
}

case $::osfamily {
case $facts['os']['family'] {
Copy link
Owner

Choose a reason for hiding this comment

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

repeating the case statement inside itself seems wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, that took more time for refactoring. Checkout the commit "Refactor case statements".

}
}
}
default: {
fail('Pam is not supported on your osfamily')
}
}

$_common_files.each |$_common_file| {
Copy link
Owner

Choose a reason for hiding this comment

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

very clever

@@ -268,15 +266,13 @@
:packages => [ 'libpam0g', ],
:files => [
{ :prefix => 'pam_common_',
:types => ['auth', 'account', 'password', 'session', 'noninteractive_session' ],
:types => ['auth', 'account', 'password', 'session', 'session_noninteractive' ],
Copy link
Owner

Choose a reason for hiding this comment

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

this would change the name of the file and i believe would result in both noninteractive_session and session_noninteractive being on disk. Does that pose any issue?

Copy link
Contributor Author

@Phil-Friderici Phil-Friderici Dec 23, 2017

Choose a reason for hiding this comment

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

I double checked the code that it only change the internal resource name but leaves the path untouched.

There was a special line (394) needed for these cases in the spec tests:
https://github.com/ghoneycutt/puppet-module-pam/blob/v2.33.0/spec/classes/init_spec.rb#L389-L403. With that removed now it is easier to see.

@Phil-Friderici
Copy link
Contributor Author

Main class looks so much cleaner now without all theses osfamily dependend settings. Based on the great work @treydock did in a8e9786 I have now moved the rest into hiera data.

Using iteration for the common* files to get rid of the os specific resource collections.

Finalise: Use module hiera data
Fix: Get rid of lsb* facts

@ghoneycutt
Copy link
Owner

Thank you!

@ghoneycutt ghoneycutt merged commit 1e32e00 into ghoneycutt:puppet5 Dec 24, 2017
@Phil-Friderici Phil-Friderici deleted the refactors branch December 28, 2017 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants