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
Make management of /etc/pki directory optional #252
Conversation
Waiting for CLA signature by @danpatdav @danpatdav - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppet.com/ Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppet.com/community/trivial_patch_exemption.html |
CLA signed by all contributors. |
@MikaelSmith Can you take a look at this PR? Not clear on why it failed the build checks. (this is my first PR to this project, so I may be overlooking something) |
file { ['/etc/pki', '/etc/pki/deb-gpg']: | ||
ensure => directory, | ||
if getvar('::puppet_agent::manage_pki_dir') == true { | ||
file { ['/etc/pki', '/etc/pki/deb-gpg']: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something you manage elsewhere? This is something I really wish we'd fix in Puppet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, some of our profiles manage this resource. And in general, since this module is about managing the puppet agent, rather than gpg keys, I would avoid managing this particular directory within this module.
@@ -52,6 +52,7 @@ | |||
$arch = $::architecture, | |||
$collection = 'PC1', | |||
$is_pe = $::puppet_agent::params::_is_pe, | |||
$manage_pki_dir = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment above describing this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
The Travis CI failure was an issue with our configuration of that service. If you rebase this PR against master, you should pickup the .travis.yml change that fixes it. It would also be nice to see tests added to assert that disabling management works. They would be somewhere around https://github.com/puppetlabs/puppetlabs-puppet_agent/blob/master/spec/classes/puppet_agent_osfamily_suse_spec.rb#L146-L150 for each of the OS you changed. |
@MikaelSmith I am not experienced with rspec-puppet tests, unfortunately. I have taken a few cracks at it, but can't seem to get the right syntax. Can you tell what I am doing wrong? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MikaelSmith ready for your review
Sorry, I've been busy. I'll try to look at this again today or tomorrow. |
Oh, thanks for working through the tests! |
Created https://tickets.puppetlabs.com/browse/MODULES-6068 to track this issue. |
Managing the /etc/pki directory inside the puppet_agent module can be problematic for organizations that manage gpg keys and settings in other modules. Adding a manage_pki_dir parameter that defaults to true allows some flexibility to the end users.