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

support forwarded for #137

Merged
merged 4 commits into from
Sep 27, 2019
Merged

Conversation

ssanden
Copy link
Contributor

@ssanden ssanden commented Sep 26, 2019

Pull Request (PR) description

right now the param "forward_for" is a boolean that allows to set "on" or "off". Squid however can be configured using one of the values: "on", "off", "transparent", "delete", "truncate".
this PR adds support for this..
also see: http://www.squid-cache.org/Doc/config/forwarded_for/

seems to work for me, please consider to merge it. thanks.

@ekohl
Copy link
Member

ekohl commented Sep 26, 2019

Currently this would be a breaking change and I'm a bit hesititant to release a major version. Have you thought about compatibility?

It also looks like the tests are failing. Could you also have a look at adding one for the new values?

@ssanden
Copy link
Contributor Author

ssanden commented Sep 26, 2019

hi @ekohl,
thanks for the feedback. i added some updates for backward compatibility.
cheers

@traylenator
Copy link
Contributor

traylenator commented Sep 27, 2019

Looks good I'd say and that's nice way of avoiding one the if, elses, I'll steal that for the future.

Some tests with the new values would be good.

Currently there are zero docs for

templates/squid.conf.header.erb Outdated Show resolved Hide resolved
templates/squid.conf.header.erb Show resolved Hide resolved
ssanden added 2 commits September 27, 2019 13:56
- added some tests
- added supported values to readme
@ssanden ssanden requested a review from ekohl September 27, 2019 12:13
@ssanden
Copy link
Contributor Author

ssanden commented Sep 27, 2019

thanks for your feedback.
i added the suggested code from the review and also some tests and the supported values in the readme.
please have another look.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

LGTM.

@traylenator traylenator merged commit 8ed3b9c into voxpupuli:master Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants