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

caddyhttp: Enhance vars matcher #4433

Merged
merged 2 commits into from
Dec 13, 2021
Merged

caddyhttp: Enhance vars matcher #4433

merged 2 commits into from
Dec 13, 2021

Conversation

mholt
Copy link
Member

@mholt mholt commented Nov 23, 2021

Enable "or" logic for multiple values. Fall back to checking placeholders if not a var name.

Seemed useful, idk. 🤷‍♂️

This matcher is actually undocumented so far, since I wasn't sure if it'd have any purpose. But I think there might be value in matching on placeholders and variables for more advanced configurations.

Enable "or" logic for multiple values.
Fall back to checking placeholders if not a var name.
@mholt mholt added this to the v2.5.0 milestone Nov 23, 2021
@mholt mholt requested a review from mohammed90 November 23, 2021 06:22
@francislavoie francislavoie added feature ⚙️ New feature or request under review 🧐 Review is pending before merging labels Nov 23, 2021
@mohammed90
Copy link
Member

I like making it more powerful. Are we concerned about breaking users? Unfortunately we don't know who's using it. It's documented in JSON.

This diff fixes the tests:

--- a/caddytest/integration/caddyfile_adapt/matcher_syntax.txt
+++ b/caddytest/integration/caddyfile_adapt/matcher_syntax.txt
@@ -101,7 +101,9 @@
                                                        "match": [
                                                                {
                                                                        "vars": {
-                                                                               "{http.request.uri}": "/vars-matcher"
+                                                                               "{http.request.uri}": [
+                                                                                       "/vars-matcher"
+                                                                               ]
                                                                        }
                                                                }
                                                        ],

@mholt
Copy link
Member Author

mholt commented Dec 2, 2021

True, but I'm pretty sure I've never seen or heard of a config where the vars matcher is being used directly in JSON config (only the Caddyfile adapter). I actually meant to make it be an array when I originally made it but the matcher got kind of forgotten about. It's intended to be an array.

And thanks for the diff, I need to apply that.

@mholt mholt removed the under review 🧐 Review is pending before merging label Dec 13, 2021
@mholt mholt merged commit ecac03c into master Dec 13, 2021
@mholt mholt deleted the vars-matcher branch December 13, 2021 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants