Add ::apache::vhost::custom #1271
Conversation
pabelanger
force-pushed
the
temp/vhost_custom
branch
7 times, most recently
from
25d238f
to
3bf69ca
Compare
pabelanger
changed the title
| path => "${::apache::vhost_enable_dir}/${priority}-${filename}.conf", | ||
| target => "${::apache::vhost_dir}/${priority}-${filename}.conf", | ||
| owner => 'root', | ||
| group => $::apache::params::root_group, |
There was a problem hiding this comment.
This doesn't look like it supports the same root group override that the base apache class does. Is that the plan? Also what about the user for managing it? Would it always be 'root' even if overridden in ::apache?
There was a problem hiding this comment.
So, honestly I don't know the answer. For the most part, this is copied verbatim from apache::vhost, so I would say the issue is there too.
Moving forward, I think this code should be broken out from both apache::vhost and apache::vhost::custom into a shared class. Making bug fixes there too.
I didn't want to do that here, as to not make things more complicated.
There was a problem hiding this comment.
Good point. After some reading, it was initially included as root/root and then expanded out to support different root_groups based on distros in this commit b87ce2d
So it looks like vhost conf files are always owned by root as far as the apache module is concerned? I have no idea if that's correct as far as Apache itself is concerned or not but it seems to be working so far. So I guess par for course is the correct method to follow on this update.
|
Unfortunately we weren't running apache in pcci so I don't have a baseline to compare this to. Also now that we are running it everything is failing, so that's not ideal. |
|
This code looks good to me +1 |
|
I would like to see us have beaker tests for it but not critical. |
|
I can try and stab at breaker tests, but never done one before. Also, I don't have the infra to really test it. |
|
Initially looks good to me. I would like to see a common class for apache::vhost and this custom module in the future, to don't repeat logic. |
| group => $::apache::params::root_group, | ||
| mode => '0644', | ||
| require => Apache::Custom_config[$filename], | ||
| notify => Class['apache::service'], |
There was a problem hiding this comment.
So.. I believe in the apache::custom_config class, there is a conditional that will set the notify if $verify_config is true. Do we want to adhere to that here?
Also corollary to that... since this is merely a wrapper around apache::custom_config should the notify be omitted here? So that you don't a) trigger an unwanted refresh or b) trigger a secondary refresh. Could there be some idempotency issues with that as well?
There was a problem hiding this comment.
I'm not sure what we want to do here honestly. I can drop the notify here, this was a copy and paste from vhost.pp.
Here we are adding a thin wrapper to apache::custom_config but setting default specific to the vhost apache folder. Signed-off-by: Paul Belanger <pabelanger@redhat.com>
pabelanger
force-pushed
the
temp/vhost_custom
branch
from
3bf69ca
to
b9f630a
Compare
Here we are adding a thin wrapper to apache::custom_config but setting
default specific to the vhost apache folder.
Signed-off-by: Paul Belanger pabelanger@redhat.com