Skip to content
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.

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

Add visible_hostname, via, httpd_suppress_version_string and forwarded_for parameters #81

Merged
merged 1 commit into from
May 14, 2018
Merged

Conversation

SourceDoctor
Copy link
Contributor

set visible_hostname for Error Pages
also configure other visibility Options

<% if @visible_hostname -%>
visible_hostname <%= @visible_hostname -%>
<% end -%>
via <%= (@via == 'on' || @via == true)?'on':'off' %>
Copy link
Member

Choose a reason for hiding this comment

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

It can't be on as via is a Boolean.

@@ -7,6 +7,10 @@
$ensure_service = 'running'
$enable_service = true
$cache_mem = '256 MB'
$visible_hostname = 'mypc@workgroup'
$via = true
Copy link
Member

Choose a reason for hiding this comment

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

This is the default in squid, but perhaps it'd be better to default to undef and not write anything into the configuration file by default.

That way, nothing will break for users who are already setting these options by using extra_config_sections.

@@ -1,6 +1,8 @@
class squid (
String $access_log = $squid::params::access_log,
Pattern[/\d+ MB/] $cache_mem = $squid::params::cache_mem,
Boolean $httpd_suppress_version_string = $squid::params::httpd_suppress_version_string,
Copy link
Member

Choose a reason for hiding this comment

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

I think these two could also be made Optional[Boolean]?

visible_hostname: 'testhost',
via: 'off',
httpd_suppress_version_string: 'on',
forwarded_for: 'off',
Copy link
Member

Choose a reason for hiding this comment

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

String off is not a boolean.

@alexjfisher
Copy link
Member

@SourceDoctor Could you look at the test failures?

@SourceDoctor
Copy link
Contributor Author

@alexjfisher
everything done, something else to handle or ready to merge?

@@ -32,6 +32,7 @@
it { is_expected.to contain_concat_fragment('squid_header').with_content(%r{^access_log\s+daemon:/var/log/#{squid_name}/access.log\s+squid$}) }
it { is_expected.to contain_concat_fragment('squid_header').with_content(%r{^cache_mem\s+256 MB$}) }
it { is_expected.to contain_concat_fragment('squid_header').with_content(%r{^maximum_object_size_in_memory\s+512 KB$}) }

Copy link
Contributor

Choose a reason for hiding this comment

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

Unescesary blank line.

@traylenator traylenator added enhancement New feature or request and removed needs-docs needs-tests labels Feb 15, 2018
Copy link
Member

@alexjfisher alexjfisher left a comment

Choose a reason for hiding this comment

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

A couple of slight tweaks needed, and then I think this will be good to merge.

README.md Outdated
* `visible_hostname` defaults to system hostname. [visible_hostname docs](http://www.squid-cache.org/Doc/config/visible_hostname/)
* `via` defaults to `on`. [via docs](http://www.squid-cache.org/Doc/config/via/)
* `httpd_suppress_version_string` defaults to `off`. [httpd_suppress_version_string docs](http://www.squid-cache.org/Doc/config/httpd_suppress_version_string/)
* `forwarded_for` defaults to `on`. [forwarded_for docs](http://www.squid-cache.org/Doc/config/forwarded_for/)
Copy link
Member

Choose a reason for hiding this comment

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

Squid might default to these settings, but the parameters actually default to undef.

@@ -39,3 +39,16 @@ workers <%= @workers %>
<% if @snmp_incoming_address -%>
snmp_incoming_address <%= @snmp_incoming_address -%>
<% end -%>

Copy link
Member

Choose a reason for hiding this comment

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

Should probably not have this blank line.

@@ -39,3 +39,16 @@ workers <%= @workers %>
<% if @snmp_incoming_address -%>
snmp_incoming_address <%= @snmp_incoming_address -%>
<% end -%>

<% unless @visible_hostname.nil? -%>
Copy link
Member

Choose a reason for hiding this comment

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

This parameter isn't a boolean, so the simpler if @visible_hostname would be better.

@alexjfisher alexjfisher changed the title proxy cache visibility options Add visible_hostname, via, httpd_suppress_version_string and forwarded_for parameters Feb 17, 2018
@traylenator traylenator merged commit 7586f18 into voxpupuli:master May 14, 2018
@traylenator
Copy link
Contributor

@SourceDoctor thanks for all the patches. Time a release after that lot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants