Fixes #30716: Ensure /pub on foreman proxy can be browsed by default #277
Conversation
manifests/init.pp
Outdated
| @@ -267,7 +271,9 @@ | |||
| require => Class['certs::apache'], | |||
| } | |||
|
|
|||
| include foreman_proxy_content::pub_dir | |||
| class { 'foreman_proxy_content::pub_dir': | |||
| pub_dir_options => $pub_dir_options, | |||
There was a problem hiding this comment.
Looking at https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/pub_dir.pp#L12 it looks like this is not actually used anywhere?
The options that apache::vhost receives seem to be hard coded at https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/manifests/pub_dir.pp#L24
There was a problem hiding this comment.
This is used in the template: https://github.com/theforeman/puppet-foreman_proxy_content/blob/master/templates/httpd_pub.erb#L7
There was a problem hiding this comment.
I always like it when a commit explains why this is a problem. That makes life easier for reviewers. Especially when someone already has done the research. Quoting myself:
Either way, this looks to be introduced by f6790c4. The template is used in pub_dir.pp (for the Satellite) and init.pp (for the Capsule). In the former the pub_dir_options variable is set, but in the latter it isn't.
This does require some updates for users - AFAIK we've told users that they can change the options via hiera and after this change they no longer can. In fact, it'll actually revert their changes.
I'd also like to see a test that the content is actually there. Alternatively we can switch the template to EPP and enforce content via a data type. I've been considering replacing ERB with EPP for that reason, but been hesitant on it.
|
Is there a simple option (to fix the immediate bug) that hiera still remains the way without me having to expose this as a parameter in |
|
Just define a variable in the body, ideally very close to where the template is actually used so you see the correlation. |
|
Switched to a single variable |
|
Would you mind including that f6790c4 introduced this regression in the commit message? That's always a good thing to have. Other than that the actual code looks good. |
Fixes regression introduced by f6790c4
|
Updated |
No description provided.