-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
caddyfile: Pass blocks to import
for snippets
#6130
base: master
Are you sure you want to change the base?
Conversation
Interesting idea. I could see the value in that, since right now only simple arguments are supported. I think this is still quite limited, and there's room for making it even more powerful. Also, I don't like the "outlet" name, doesn't feel like it fits well with Caddyfile conventions. I only spend like two minutes thinking about it, but I could see something like this being possible; since we already have
Producing 👇:
The implementation may be tricky, means having to reach into the block if we encounter |
the limitation is as you point out - we are currently able to pass
"blocks" makes more sense to me. (funny, outlet was taken from a react project as well https://reactrouter.com/en/main/components/outlet ) i think multiple named blocks makes sense. for instance, if args supplies a
another option would be arrays, ala
however, i'm not convinced that this makes implementation any easier. |
"anonymous" blocks is not a convention we support, except for global options specifically as a special case. So I don't think numerically indexed blocks like that is the right approach. Of course "naming things" is always hard (often tricky with named matchers, and even snippet names, what should they be called?) but it's definitely more flexible/scalable than numeric indexes, and less confusing to read because the name can carry some intent. One thing to think about is we did deprecate So yeah my thoughts right now are |
ah, i didn't know unnamed blocks were a no-no. some more things i thought of:
imo, if
|
I don't think so, that's out of scope. It's not
I don't think we'd want
Not really a concern,
It's not just blocks, you will still be able to use the args on the same line, backwards compatible. It doesn't make sense to have two sets of args. |
this simple example works as intended! so it's a start i'm parsing by just putting the tokens (for handling {block}) into a newly made dispenser. that's the only real hack and the rest seems to play nicely? there's no logic for nested. i extract the key by doing |
https://github.com/caddyserver/caddy/tree/master/caddytest/integration/caddyfile_adapt should tests for this be created here? if i create new .caddyfiletest in the directory, will it just work tm |
Yep, files in there are automatically run. |
created some simple tests, block, blocks, nested snippet with name conflict. let me know if you have any other thoughts, i can try to make some more |
import
for snippets
i was wondering if i needed to do anything to move this along. if not, thats fine, just not sure if i should be doing anything other than merging master and making sure it continues to work and build |
Apologies from my end; you're doing great work, and this is a good contribution; I am just behind. I have less than 2 pages left on my notifications backlog though so I am getting around to it! Francis I'm sure will also get around to it eventually but between any of us maintainers, we will address this :) |
This is still on my list just so you know! My list is much smaller now, and this is near the top. Now we're getting ready for the 2.8 release, and this might have to come after 2.8, but it'll be one of the first new features we circle back to in that case. 👍 |
@mholt do you want me to keep updating the branch and making sure things are good, or should i wait you to tell me its back on your docket |
@elee1766 Thanks for asking! I won't be able to get back to this until after 2.8 (which is nearly ready, RC is going out in the next few days and then we'll tag 2.8 when we're comfortable doing so). So while another rebase may be required before we merge this, I'll let you know when we're ready for that so you don't keep rebasing over and over. |
@mholt sounds good thank u. excited for 2.8! we have been super happy with caddy in production with the new fs directive |
as the amount of caddyfiles and snippets in our configuration grows, we have found it might be useful to be able to declare something like an {outlet} which spits out a configured block in the import statement.
The benefit is that it allows us to create snippets which can serve as entrypoints that must contain a superset. this is makes the configs less prone to error, as the rule becomes to use the snippet, instead of to not forget the import.
for instance, imagine a config like this:
after:
my snippet,
module_func
, is now able to act as a default configuration formodule
. downstream users can be told ONLY to usemodule_func
. This will avoid the mistake of ever usingmodule
withoutimport default_config
.maybe this is a feature you guys might also find interesting?
as import statements currently don't consume blocks at all, i dont think it would break anything. it would allow for some more contrived caddyfiles (is that a bad thing)
i've made this a PR as we were testing this out with our config, and those few lines in the code made it possible - but maybe im missing something and this is really dangerous and there's a good reason the feature doesn't exist.
we refactored parts of our config to use it and it does seem safer - doing X is much easier than not forgetting Y.