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.

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

Jump to bottom

support listening on specific interfaces; changed params in squid::http_port{} #103

Merged
merged 5 commits into from
Jan 24, 2019
Merged

support listening on specific interfaces; changed params in squid::http_port{} #103

merged 5 commits into from
Jan 24, 2019

Conversation

tequeter
Copy link
Contributor

This allows http_port host:port configuration constructs, instead of just
specifying the port and listening on all interfaces.

Pre-1.0.0 versions used to allow host:port as port/title arg, because the
input wasn't sufficiently checked.

@tequeter
Copy link
Contributor Author

Retriggering CI...

@tequeter tequeter closed this Jul 20, 2018
@tequeter tequeter reopened this Jul 20, 2018
@tequeter tequeter closed this Jul 20, 2018
@tequeter tequeter reopened this Jul 20, 2018
@tequeter
Copy link
Contributor Author

There, tests pass except for the first three that seem unrelated to my change.

@alexjfisher
Copy link
Member

Looks like the tests didn't run because of rubygems/bundler#6629 (comment)

@alexjfisher
Copy link
Member

I've opened #104 which should get the tests running again.

@tequeter After that is merged, would you be able to rebase this PR?

@tequeter tequeter force-pushed the listen-interface branch 2 times, most recently from d157835 to ef6cad4 Compare July 26, 2018 07:56
@tequeter
Copy link
Contributor Author

There we go! All green.

bastelfreak
bastelfreak reviewed Jul 29, 2018
View reviewed changes
manifests/http_port.pp Outdated Show resolved Hide resolved
bastelfreak
bastelfreak reviewed Jul 29, 2018
View reviewed changes
manifests/http_port.pp Outdated Show resolved Hide resolved
@bastelfreak bastelfreak changed the title support listening on specific interfaces support listening on specific interfaces; changed params in squid::http_port{} Jul 29, 2018
@bastelfreak bastelfreak added the needs-work not ready to merge just yet label Jul 29, 2018
@tequeter
Copy link
Contributor Author

tequeter commented Aug 1, 2018 •
edited
Loading

I went for the minimalist approach first, but good points.

Can you have a look at the new version? You can diff tequeter/listen-interface{1,} for an easier review, as c916027 is an edit.

P.S.: WRT the new title of this PR ("changed params"), do note that my API changes are meant to be backward-compatible. I improved the unit tests to ensure that, at least for the usecases I could think of.

ekohl
ekohl reviewed Dec 11, 2018
View reviewed changes
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.

Could you rebase to fix the conflict?

I'd also like do a release which is already breaking and ideally we don't do breaking changes too often. Any chance you could get to this in the not too far future?

manifests/http_port.pp Outdated Show resolved Hide resolved
manifests/http_port.pp Outdated Show resolved Hide resolved
@tequeter
Copy link
Contributor Author

I'll try to have a look at it this week, before my holidays.

ekohl
ekohl reviewed Dec 15, 2018
View reviewed changes
manifests/http_port.pp Outdated Show resolved Hide resolved
templates/squid.conf.port.epp Outdated Show resolved Hide resolved
@tequeter
bump stdlib to 4.25.0 for port types
a89b06f
@tequeter
support listening on specific interfaces
37360d1 
This allows "http_port host:port" configuration constructs, instead of just
specifying the port and listening on all interfaces.

Pre-1.0.0 versions used to allow "host:port" as port/title arg, because the
input wasn't sufficiently checked.
@tequeter
convert http_port template to EPP
43f1245
@tequeter
Copy link
Contributor Author

I refactored this PR a bit, see the commit message. There are two failed acceptance tests, which seem unrelated to my changes.

@bastelfreak
Copy link
Member

looks good to me. @ekohl can you have a look again?

ekohl
ekohl approved these changes Jan 2, 2019
View reviewed changes
manifests/http_port.pp Outdated Show resolved Hide resolved
manifests/http_port.pp Show resolved Hide resolved
@tequeter
refactor the last two commits
486e869 
- Simplify the logic.
- Check the title-provided host against Stdlib::Host.
- Move the canonical host:port construction out of the template.
- Provide separate values for title and host:port to the template.
- Add three new testcases.
@tequeter tequeter force-pushed the listen-interface branch 3 times, most recently from 74d787f to 486e869 Compare January 4, 2019 20:45
ekohl
ekohl approved these changes Jan 6, 2019
View reviewed changes
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.

If nobody beat me to it I'll fix the build infra tomorrow to make sure the tests pass. Other than that I think this is ready to be merged.

@ekohl ekohl removed the needs-work not ready to merge just yet label Jan 24, 2019
@ekohl ekohl merged commit fbf71de into voxpupuli:master Jan 24, 2019
@ekohl
Copy link
Member

ekohl commented Jan 24, 2019

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bastelfreak bastelfreak bastelfreak left review comments

@ekohl ekohl ekohl approved these changes

Assignees
No one assigned
Labels
backwards-incompatible enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tequeter @alexjfisher @bastelfreak @ekohl