Skip to content

[Issue 2247] fix datatype of vhost:headers from String to Array #2248

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[Issue 2247] fix datatype of vhost:headers from String to Array #2248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d78f978
[Issue 2247] fix datatype of vhost:headers from String to Array
johannesschillinger-dm Jun 13, 2022
75c730f
[Issue 2247] adapt headers example in documentation
johannesschillinger-dm Jun 13, 2022
ac7cbf6
[Issue 2247] make declaration of type consistend, adapt check accorid…
johannesschillinger-dm Jun 14, 2022
257f0ef
[Issue 2247] fix linting errors
johannesschillinger-dm Jun 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions manifests/vhost.pp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1409,7 +1409,7 @@
# docroot => '/path/to/directory',
# directories => {
# path => '/path/to/directory',
# headers => 'Set X-Robots-Tag "noindex, noarchive, nosnippet"',
# headers => ['Set X-Robots-Tag "noindex, noarchive, nosnippet"'],
# },
# }
# ```
Expand Down Expand Up @@ -1853,7 +1853,7 @@
Optional[Variant[Array[String],String]] $redirectmatch_status = undef,
Optional[Variant[Array[String],String]] $redirectmatch_regexp = undef,
Optional[Variant[Array[String],String]] $redirectmatch_dest = undef,
Optional[String] $headers = undef,
Optional[Array[String]] $headers = undef,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're changing it to an array, I'd say drop the Optional part and make it an empty array by default. I'd also narrow it to non-empty strings:

Suggested change
Optional[Array[String]] $headers = undef,
Array[String[1]] $headers = [],

Then you can simplify templates/vhost/_header.erb a lot too.

You can also drop the if $headers and stick to ! empty($headers) on line 2263.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekohl I thought about that, but realised the patch the way I did for the following reasons:

  • I only have the time to actually fix the bug which breaks our puppet runs, I did not want to rework more than really necessary
  • every other array type parameter in the module, including $request_headers is an Optional[Array[String]]. I wanted to stay consistent

For those reasons I would like this fix to be accepted. If the datatype should be changed to Array with default [], I would like this seen as an independent rework issue

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it should be fixed so I opened #2251.

Optional[Array[String]] $request_headers = undef,
Optional[Array[String]] $filters = undef,
Optional[Array] $rewrites = undef,
Expand Down Expand Up @@ -2260,7 +2260,7 @@
}

# Check if mod_headers is required to process $headers/$request_headers
if $headers or $request_headers {
if ($headers and ! empty ($headers)) or ($request_headers and ! empty($request_headers)) {
if ! defined(Class['apache::mod::headers']) {
include apache::mod::headers
}
Expand Down
2 changes: 1 addition & 1 deletion spec/defines/vhost_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@
'redirectmatch_status' => ['404'],
'redirectmatch_regexp' => ['\.git$'],
'redirectmatch_dest' => ['http://www.example.com'],
'headers' => 'Set X-Robots-Tag "noindex, noarchive, nosnippet"',
'headers' => ['Set X-Robots-Tag "noindex, noarchive, nosnippet"', 'Accept: text/html'],
'request_headers' => ['append MirrorID "mirror 12"'],
'rewrites' => [
{
Expand Down