Exposed $logroot_mode in init.pp #807
Conversation
|
Hi @Ginja , Sorry that I've introduced a bug that impacts you. For this PR, why not adding the This way different vhosts might set different logroots, yet if no vhost (ie. Would that match your use case ? |
|
Hi @Spredzy, No problem. I thought about adding a $manage_logroot parameter, but I then realized that it wouldn't help solve your problem unless you also exposed it in the init.pp class. I'll explain my reasoning, and please correct me if I'm wrong. If we added a $manage_logroot parameter but it wasn't exposed in init.pp, and its default value was set to true, wouldn't you be in the same situation (i.e. not able to set $logroot_mode from another vhost resource)? The log directory will always get created no matter what $default_vhost is set to, as it's only surrounded by a Another way I see to resolve this issue would be to move the creation of the default log location (i.e. what ErrorLog is set to in httpd.conf) to the init.pp class. If that's not possible for some reason, this PR at least lets you control the permissions of the default log location without specifying another vhost resource. What do you think? |
|
Bump :) |
| @@ -45,6 +45,7 @@ | |||
| $conf_file = 'httpd.conf' | |||
| $ports_file = "${conf_dir}/ports.conf" | |||
| $logroot = '/var/log/httpd' | |||
| $logroot_mode = '0700' | |||
There was a problem hiding this comment.
I don't think this is the default right now, is it?
There was a problem hiding this comment.
This debuntu box for instance has 0750 for root:adm
|
Hi @igalic, It's the default permissions I get whenever I use the Apache module on a RH host. However, I have no problem changing it to 0750. Let me know if that's what you would like to see. |
|
I'd change them to whatever they are on the appropriate operating systems, so we don't actually change anything ;) |
|
Hm, it might be better if I change the value to undef as that's the value in apache::vhost. This way nothing changes, but still allows for overrides. Thoughts? |
|
+1 |
|
Done. |
| @@ -273,7 +273,7 @@ | |||
| } | |||
|
|
|||
| # Same as above, but for logroot | |||
| if ! defined(File[$logroot]) and $ensure == 'present' { | |||
| if ! defined(File[$logroot]) { | |||
There was a problem hiding this comment.
@igalic do you see this change causing any issues? I'm not sure I like this as it will create the logroot directory even if you're deleting the vhost.
There was a problem hiding this comment.
@mhaskel this is true, but that resource also creates the default log directory (/var/log/httpd, or /var/log/apache2). So when you add $ensure == 'present' as a condition, you're effectively making default_vhost => false useless; as Apache will fail to start if the log directory specified in httpd.conf (ErrorLog) doesn't exist. I agree with you that it does make sense to add that second condition, and one way to include it safely is to move the creation of the default log location to init.pp.
|
CLA signed by all contributors. |
|
@mhaskel frankly, |
|
@igalic @mhaskel Can we come to an agreement on what should be done? Are there to be anymore changes? |
|
@Ginja I think Also, this will need to be rebased. |
Ginja
force-pushed
the
expose_logroot_mode
branch
from
86310ea
to
90265d7
Compare
|
@mhaskel Done :) |
|
@Ginja looks like you still need a rebase...thanks! |
Ginja
force-pushed
the
expose_logroot_mode
branch
from
90265d7
to
cd6f974
Compare
|
@mhaskel Oops! Thanks. |
|
@Ginja So, the code is looking good now, just need a couple quick things before we can get it merged. Can you update the README with the new parameter and make sure you have unit test coverage for the parameter (probably added to spec/defines/vhost_spec.rb)? It also looks like this update broke a test case at So, that should be updated to match the new logic in the module. We're definitely getting close, apologies that this has taken so long :) |
This commit now allows you to set the directory permissions on the default $logroot directory by exposing $logroot_mode in init.pp. It also adds a $logroot_ensure parameter to apache::vhost that can toggle the creation of a vhost's logroot resource.
Ginja
force-pushed
the
expose_logroot_mode
branch
from
cd6f974
to
1b4e942
Compare
|
@mhaskel Of course, I should have thought to do that last time. I've added a validation test for logroot_ensure, as well as made some of the existing resource checks more specific (ex: add resource attributes that we're setting). The spec tests run clean locally, but we'll see what Travis CI says :) I've also updated the README.md file with the two new parameters. In doing so, I actually noticed that 2771531 accidentally put the vhost's logroot_mode parameter definition in the wrong section of the documentation. However, what he didn't know was that he was just doing me a future favour because I didn't need to type as much :) No need to apologize, thanks for your help. |
Exposed $logroot_mode in init.pp
|
@Ginja great, thanks for the contribution! |
The problem raised in puppetlabs/puppetlabs-apache#803 has been resolved in puppetlabs/puppetlabs-apache#807
…main/remove_ubuntu_16.04_support pdksync - (GH-iac-334) Remove Support for Ubuntu 14.04/16.04
This commit now allows you to set the directory permissions on the $logroot
directory. It also resolves a catalog compilation issue when specifying the
apache class with default_vhost => false, and no other vhost resources.