search

voxpupuli / puppet-squid

Puppet module for configuration of squid caching proxy.
https://forge.puppet.com/puppet/squid
Other
12 stars 54 forks source link

Update squid.conf.refresh_pattern.erb #65

Closed Rammgako closed 6 years ago

Rammgako commented 7 years ago

Not sure if it works..

ekohl commented 7 years ago

Could you help me by describing the problem you're trying to solve?

Rammgako commented 7 years ago

Hi, sure!

I've tried to use this puppet module and found a strange behaviour in the section puppet-squid/templates/squid.conf.refresh_pattern.erb:

"refresh_pattern <%= @pattern %>: <%= @case_sensitive?'':'-i ' %><%= @min %>"

The part @case_sensitive going after @pattern means that '-i' will be after ':' in section with parameters for refresh pattern in squid.conf file, squid failed to start with this line in squid.conf. However according to the documentation on refresh_pattern, '-i' should go first. I changed this part (tested in my env) and created a pull request (first one). This merge request was failed on test:

# spec/defines/refresh_pattern_spec. 
        it { is_expected.to contain_concat_fragment(fname).with_content(%r{^refresh_pattern\s+-i\s+case_insensitive:\s+0\s+0%\s+0$}) }

because I have swapped "<%= @case_sensitive?'':'-i ' " and "<%= @pattern %>: ". So I've tried to change the test itself:

    it { is_expected.to contain_concat_fragment(fname).with_content(%r{^case_insensitive\s+-i\s+refresh_pattern:\s+0\s+0%\s+0$}) }

but due to my little experience with the github I've created different branch...

So the issue is:

  1. "refresh_pattern <%= @pattern %>: <%= @case_sensitive?'':'-i ' %><%= @min %>" will generate wrong line in the squid.conf
  2. Test accepts only line which has a format: "refresh_pattern <%= @pattern %>: <%= @case_sensitive?'':'-i ' %><%= @min %>"
alexjfisher commented 7 years ago

This PR replaces https://github.com/voxpupuli/puppet-squid/pull/64 I've updated the title accordingly. If this is merged, it should probably be updated to something better (suitable for the changelog).

ekohl commented 7 years ago

It looks like this introduces test failures. Could you have a look? If you rebase on current master then it should have a clean base with passing tests.

traylenator commented 6 years ago

This is obsoleted by #87 which solves the exact same problem.

traylenator commented 6 years ago

87 now merged so thios is not needed. Thanks @Rammgako @ekohl