Add feature to reload apache service when content of ssl files has changed #2157
Conversation
|
|
|
Ah well, so many hours with tests and i oversaw the basics... 🙈 Please apologize! However if you have feedback to the feature itself, let me know. |
timdeluxe
force-pushed
the
te_ssl_reload
branch
5 times, most recently
from
bd14e2b to
649f7f5
Compare
@ekohl Before i answer to each of the code remarks, lets discuss this first, because those code remark discussions depend on that: I struggled with exactly that question, but the reload thing should be a per-vhost-based thing. One should be able to have apache reloaded if the ssl files change for a vhost, but not for the other. At least that is my opinion. I need to manage the folder in ssl.pp then, because vhost.pp is a define and managing the folder there would lead to duplicate declarations. Also even if the reload is deactivated for default vhost and for the mod::ssl module, it still can be activated for a vhost and then the folder is required. Or do we say that reload thing is a general setting and can't be changed on a per vhost base? (In that case your requested changes could be implemented) |
|
You're right that we should take it to a higher design level. So we have a few options. The current one obviously. We can also introduce a global parameter. Another is introducing a private class (like The benefit is that |
|
A private class is indeed a good idea, will try that, thanks! However mod_ssl does need at least the file resources for the ssl files, because one could work with apache without using vhosts at all (extremly rare, but the module is widely spread, so we should not forget those people). |
timdeluxe
force-pushed
the
te_ssl_reload
branch
2 times, most recently
from
3e3296b to
2987339
Compare
|
On real world usage of my branch i found two further issues, which i now fixed: The puppet_ssl_dir was managed before apache was even installed and resulted in missing httpd_dir, so management of puppet_ssl_dir failed. Also even if the default-ssl vhost was set to absent, it still managed its certificate copy - this is also now fixed. This PR still says @ekohl requested 1 change, however i can only find resolved conversations. Can someone help me? |
|
@timdeluxe As stated above by @ekohl it looks like some of the reference has been deleted by your changes. |
|
@david22swan Sorry, missed that. Is fixed now. |
Sorry, i am still confused by the github review process. It still shows a change requested by @ekohl, despite the fact that i resolved all conversations :-/ |
|
@timdeluxe Apologies, for that to disappear @ekohl must approve the PR. |
timdeluxe
force-pushed
the
te_ssl_reload
branch
2 times, most recently
from
7d7bae1 to
fd05fcf
Compare
There was a problem hiding this comment.
Perhaps some example in https://github.com/puppetlabs/puppetlabs-apache/tree/main/examples would be nice?
I don't think that this feature is so important, that it needs an example. The ones who need it, will find it anyway... |
…ssl files has changed
After a fighting a bit with git logic (i always get knots in my brain) it is hopefully correctly done now. |
I hope the same. Also thanks to you for guiding me through it and spotting a lot of little things to improve. |
|
Tests on RedHat failed not due to my code but due to mirrors not answering correctly - don't know if it is a temporary or a general problem, which can be fixed :/ |
|
That's a known issue (#2171 tries to look into it) |
This fixes issue MODULES-10847, which we also wanted to solve.
The solution with those helper files seems a bit odd, but the original ssl files could be managed by any means outside of the apache module or even outside of puppet, so this was the only way i could image to achieve it.
(Slightly offtopic: Let me add that this module gave me a love/hate relationship with spec/acceptance tests. First i blamed wrong tests to be the cause for failing, but in the end it was my own stupid code. Finally i could manage getting tests run green at my desktop - hope they will do in the modules CI setup as well.)