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

Update squid.conf.refresh_pattern.erb #65

Closed
wants to merge 4 commits into from

Conversation

Rammgako
Copy link

Not sure if it works..

as we change the place for '-i' we should change and test for it as well...
as we need '-i' placed before refresh_pattern.
@ekohl
Copy link
Member

ekohl commented Nov 9, 2017

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

@Rammgako
Copy link
Author

Rammgako commented Nov 9, 2017

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 alexjfisher changed the title Patch 2 Update squid.conf.refresh_pattern.erb Nov 9, 2017
@alexjfisher
Copy link
Member

alexjfisher commented Nov 9, 2017

This PR replaces #64
I've updated the title accordingly. If this is merged, it should probably be updated to something better (suitable for the changelog).

@ekohl
Copy link
Member

ekohl commented Nov 16, 2017

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
Copy link
Contributor

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

@traylenator
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants