-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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: Implement named routes, invoke
directive
#5107
Conversation
3f7a400
to
75a1df6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Francis -- sorry it took me so long to get to this. It's definitely not a simple change...
Is it worth it do you think? I do feel like it increases the complexity of the Caddyfile a lot, not just the code but also the comprehension (like snippets).
I wonder if we could mark this feature as experimental just in case we need to change or remove it later, but if it saves GB of RAM for large deployments at virtually no performance penalty, then maybe it's worth it.
func (p *parser) isNamedRoute() (bool, string) { | ||
keys := p.block.Keys | ||
// A named route block is a single key with parens, prefixed with &. | ||
if len(keys) == 1 && strings.HasPrefix(keys[0], "&(") && strings.HasSuffix(keys[0], ")") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the named route block should/could just be prefixed with &
instead of including the parens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but I like keeping the parens to keep it similar to snippets because it works similarly but more optimized (reference instead of copying)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just feel like &(foo)
is a lot of typing for what programmers are used to seeing as &foo
🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typing two extra characters is not significantly more. Without parens it would look too much like a site block address.
modules/caddyhttp/routes.go
Outdated
|
||
// Compile prepares a middleware chain from the route list. | ||
// This should only be done once: after all the routes have | ||
// been provisioned, and before serving requests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually called while serving requests though, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That code comment was copied from func (routes RouteList) Compile
. You're right though, it's called from invoke
's ServeHTTP
. I think the RouteList
one's comment is also wrong though, because it gets called from Subroute.ServerHTTP
and such. I'm not sure if how "compiling" is done was changed way back in early versions of v2 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I adjusted the comments for these. Let me know if you agree 👍
I think it's useful. There's a lot of situations where it can help manage memory usage and reduce startup time by only doing work once for things that might happen in multiple sites. 👍 to marking it experimental. |
75a1df6
to
0892043
Compare
0892043
to
d7b8bf4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it is time to give this a try 😄
Definitely mark it as experimental in the docs, subject to change. I'm not 100% on the &(named)
syntax yet (looks like a "reference to a copy" to me) -- I think I still prefer &named
, but there are compelling arguments both ways.
Thanks for working on this 😊
Closes #4995
Adds a new
named_routes
mapping on servers, and these routes can be executed with aninvoke
handler by their name.In the Caddyfile, defining named routes looks similar to snippets, but with a
&
prefix which makes it read more like a reference than a copy, where a snippet is basically a copy (that's the idea anyway).The Caddyfile adapter does a bunch of funky stuff to only pull in named routes into servers where they are used with an
invoke
directive. There's some juggling of options, state and piles, which... I don't super love but it works I guess. If you can think of a cleaner way to do that in code, I'm all ears.I'm also not sure where
invoke
should go in the default directive order. I put it just aboveroute
andhandle
for now (because it technically makes a subroute), but it's debatable where it should go.