-
Notifications
You must be signed in to change notification settings - Fork 39
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
remove dependencies to external modules #142
Conversation
PR agains puppet-foreman_proxy_content: theforeman/puppet-foreman_proxy_content#114 |
PR against puppet-katello: theforeman/puppet-katello#172 |
manifests/puppet.pp
Outdated
ensure => directory, | ||
owner => 'puppet', | ||
mode => '0700', | ||
require => Class['puppet::server::install'], |
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.
Unfortunately, this exists because Puppet 4 packages do not create a puppet
user (unless it's been fixed) in the RPM so we aren't guaranteed this will work at the time of calling without requiring this class here.
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.
We could let our puppet module manage the puppet user (possibly in puppet::server::config
). Then puppet autorequires would fix this for us.
I think this is a nice cleanup. I do have a question regarding the design change. The intent of the notifies is such that when the certificates change the services would be notified and restarted to ensure they pick up the certificate changes. By dropping that, how do we ensure this functionality? |
Oh I see, you are moving it into the calling module. Which pushes the reload responsibility off to a third party module. I suppose this gives more control to the third party module and pushes the certs module to be more of a certificate manager who doesn't care where and how you use the certs. Thinking out loud :) |
Exactly. And it makes testing this class easier or would allow users to use this class for something else. :-) Regarding the puppet dependency: I'm unsure, what's the best way to solve this. The current implementation has the issue, that it relies on the internals of |
manifests/apache.pp
Outdated
@@ -62,8 +62,6 @@ | |||
owner => $::apache::user, | |||
group => $::certs::group, | |||
mode => '0440', | |||
} -> |
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.
Are you sure this is correct? Isn't there a chance the key will have incorrect permissions for apache? #137 does solve it by containing the keypair in a define and let that notify apache.
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.
I agree, if we actually include ::apache
to get the user and group, we should just leave this in.
I believe @stbenjam filed an issue with Puppetlabs to fix this in their RPM. I don't know the current state but maybe he can inform us. |
It's unresolved. |
Will need a rebase as well |
Rebased and re-added the puppet dependency and httpd requirement. |
Thanks @timogoebel ! |
This commit removes dependencies to external services.
They should be defined in a profile module, something like this: