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

caddyhttp: reverseproxy: fix hash selection policy #4137

Merged
merged 2 commits into from
Apr 29, 2021
Merged

caddyhttp: reverseproxy: fix hash selection policy #4137

merged 2 commits into from
Apr 29, 2021

Conversation

simaotwx
Copy link
Contributor

The tests ran without an issue, so this should be the fix.

Fixes: #4135
Test: go test './...' -count=1

@francislavoie francislavoie added the needs tests 💯 Requires automated tests label Apr 29, 2021
@francislavoie francislavoie added this to the v2.4.0 milestone Apr 29, 2021
@francislavoie
Copy link
Member

francislavoie commented Apr 29, 2021 •
edited
Loading

Thanks!

We'll definitely need a test that fails before the change to make sure it actually fixes the problem.

@simaotwx
caddyhttp: reverseproxy: fix hash selection policy
838edef 
Fixes: caddyserver#4135
Test: go test './...' -count=1
@simaotwx
caddyhttp: reverseproxy: add test to catch caddyserver#4135
5a3e746 
If you revert the last commit, the test will fail.
@simaotwx
Copy link
Contributor Author

Test case added. It fails before applying my patch and passes afterwards.

@simaotwx
Copy link
Contributor Author

Is it appropriate to reverse the order of the commits? Maybe that would immediately prove that the commit after it fixes the issue.

@francislavoie
Copy link
Member

No, that's okay! As long as you confirmed it, that's good 👍

Thanks!

@francislavoie francislavoie added under review 🧐 Review is pending before merging and removed needs tests 💯 Requires automated tests labels Apr 29, 2021
@simaotwx
Copy link
Contributor Author

Alright, perfect. :)

mholt
mholt approved these changes Apr 29, 2021
View reviewed changes
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Excellent; thank you so much for the fix and the test case! Please feel free to contribute again.

@mholt mholt merged commit 9017557 into caddyserver:master Apr 29, 2021
@mholt mholt removed the under review 🧐 Review is pending before merging label Apr 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francislavoie francislavoie francislavoie approved these changes

@mholt mholt mholt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.

[Bug] reverse_proxy can skip hosts when using any hashing policy
3 participants
@simaotwx @francislavoie @mholt