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

httpcaddyfile: Ensure handle_path is sorted as equal to handle #3676

Merged
merged 5 commits into from
Sep 17, 2020
Merged

httpcaddyfile: Ensure handle_path is sorted as equal to handle #3676

merged 5 commits into from
Sep 17, 2020

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Aug 21, 2020 •
edited
Loading

Fixes an issue found in #3675

I don't love this fix frankly, any hard-coded logic is 😬 but I'm not sure of a better way to do this. We want handle and handle_path to have different dirFunc logic, but we need them to sort equivalently because they are both just handles, where one is just a shortcut to the other.

Having done this, I think we could probably remove handle_path from the directive order since it'll never actually be seen by the sort function.

@francislavoie francislavoie added the bug 🐞 Something isn't working label Aug 21, 2020
@francislavoie francislavoie added this to the v2.2.0 milestone Aug 21, 2020
@francislavoie francislavoie force-pushed the handle_path_sort branch 3 times, most recently from aaea746 to 5474718 Compare August 21, 2020 05:58
@francislavoie
Copy link
Member Author

francislavoie commented Aug 21, 2020 •
edited
Loading

I'm so confused. Why is groupCounter flaky? It adapts to group3 on my machine but CI says it should be group4? 😬

Edit: HUH? Reran locally and it spat out group4 I'm so confused why did I get group3 before with the same config? 😕

@francislavoie francislavoie mentioned this pull request Aug 21, 2020
Closed
@francislavoie
Copy link
Member Author

I hate Go's map. It sucks. 😠

Well, now it should be deterministic. So that's helpful.

I ran 10k iterations of the adapt tests before/after to make sure.

@francislavoie francislavoie linked an issue Aug 21, 2020 that may be closed by this pull request
handle_path is not stripping prefix, or root is using the wrong path #3675
Closed
mholt
mholt requested changes Sep 17, 2020
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.

Just fixing a typo that was bothering me :)

Otherwise LGTM. I can see why this definitely isn't the most elegant fix, but it's pretty simple, hopefully it works well! If not, it's purely internal so we should be able to fix it up again if needed.

caddyconfig/httpcaddyfile/httptype.go Outdated Show resolved Hide resolved
mholt
mholt approved these changes Sep 17, 2020
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.

LGTM, I hate it. 🙃

Actually though I think this is worth a shot. There are worse parts of the Caddyfile adapter's internals, so...

@mholt mholt merged commit e3324aa into caddyserver:master Sep 17, 2020
@francislavoie francislavoie deleted the handle_path_sort branch September 17, 2020 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mholt mholt mholt approved these changes

Assignees
No one assigned
Labels
bug 🐞 Something isn't working
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.

handle_path is not stripping prefix, or root is using the wrong path
2 participants
@francislavoie @mholt