Caddyfile snippets #1921

captncraig · 2017-10-13T15:07:50Z

1. What does this change do, exactly?

Allows macros of the form:

macro foo {
  content....
}

or

(foo) {
  content...
}

We had some discussion about particular syntax, so I implemented both. I don't really care which anymore at this point. Just need to choose one.

Can be used in any server with import foo, it just imports the saved macro rather than read a file.

2. Please link to the relevant issues.

https://caddy.community/t/caddyfile-macros-or-inline-includes/2852/8

3. Which documentation changes (if any) need to be made because of this PR?

Needs to be documented in caddyfile docs

4. Checklist

I have written tests and verified that they fail without my change
I have squashed any insignificant commits
This change has comments for package types, values, functions, and non-obvious lines of code
I am willing to help maintain this change if there are issues with it later

mholt · 2017-10-14T16:28:42Z

Cool, thanks!

I think the name "macro" leaves a bad taste in my mouth after some bad experiences with C. Also, it feels a little weird to call a declarative unit a "macro" when it's not procedural or operative.

That said, I do like the symbolic notation a little better than another keyword exception.

I like the parentheses, personally. Or I still like the idea of single quotes: 'foo' to declare a literal block named foo.

Are there other preferences for this syntax? And what should such a block be called? Just a 'block literal' or a 'reusable block'? Once we decide which syntax to use, I'll begin a review. :)

miekg · 2017-10-14T16:38:35Z

Oh, I like the 'foo' syntax as well (and dropping "macro" I assume?)

mholt · 2017-10-14T16:42:47Z

Yeah, I'd like to drop 'macro' in favor of some other syntax. Either way, I think beginners will have to learn what the block is/means, whether it's the word macro or a single-quoted word.

francislavoie · 2017-10-14T16:56:44Z

How about define foo { or something? Personally I think using a keyword really helps make things clear. If we use somekind of syntactic symbol like single quotes, then it won't really fit with the rest of Caddyfile's design in terms of "simple words" for each directive. I personally like macro, but any synonym can do, really.

Other possibilities: var or let

aaronellington · 2017-10-14T17:21:54Z

I like the ‘(foo) {‘ syntax. Seems more clear that it is not a normal label.

msfjarvis · 2017-10-14T17:38:16Z

I'd love define foo {... or maybe var foo { so that you know that this following block is not your standard virtual host definition. Just my two cents. Great work @captncraig !

mholt · 2017-10-14T17:41:52Z

Again, the problem with using a keyword (and I sometimes regret using import as a special case already) is that it's indistinguishable from a hostname. Directives really do not belong outside server blocks.

francislavoie · 2017-10-14T18:16:00Z

Do labels ever have spaces though? If you really needed a space in it, wouldn't you do %20 instead?

mholt · 2017-10-14T18:22:28Z

It depends on the server type - technically a label can have spaces, but usually you'd do that by enclosing it in quotes "like this" not with URL-encoding... I don't know of a server type that expects a single label with a space.

Multiple labels like this: site1.com site2.com { are a valid way to open a server block.

elcore · 2017-10-14T18:23:18Z

I like (foo) and 'foo'

tobya · 2017-10-15T16:48:23Z

I like the idea of using a special character such as

$foo
@foo
&foo

otherwise I prefer the use of (foo) to 'foo' since IMHO it is much more obvious that (foo) is not a host name. I think whatever we decide it should be something that it is obvious by looking at it cannot be a valid domain.

I also think we should explicitly not allow . (dot) in label names.

I think this is an excellent PR and support its merging.

tobya · 2017-10-15T21:03:40Z

Personally I would love to be able to use this without the import keyword so it can be used like a placeholder.

This would enable me to define a log format for example

(StandardLog) {
        {path} {host} etc
   }

Then use this in the caddyfile where I want

Mydomain.com {
  Log / my file.log {(StandardLog)}
 }

Would make it a lot easier to manage large caddyfile s

mholt · 2017-10-15T21:44:42Z

@tobya In that case, I'd prefer the single quote syntax more than parentheses: {'StandardLog'}

But I'm not sure I like that more than just setting and using environment variables (which you can already do): {$StandardLog} - edit: although I dunno if the expansion is recursive... 🤔

francislavoie · 2017-10-15T22:20:04Z

@mholt I do know env var expansion doesn't expand out to multiple arguments (see #1235)

tobya · 2017-10-15T22:41:11Z

I must admit I was only sort of aware that I could use env variables like that in a caddyfile, however I would much rather have all my declarations in the caddyfile rather than being pulled in from outside.

I'm fine with whichever syntax the majority prefer.

Although this may need to be a separate feature request 😄 as I can see complications.

captncraig · 2017-10-15T23:11:57Z

I removed the macro foo syntax, so now it only allows (foo) { }

captncraig · 2017-10-15T23:14:28Z

@tobya, I kinda see where you are coming from there. import does currently preclude that because it has to be it's own line.

I don't see why your {{name}} syntax couldn't be added at a future time though. It is fairly orthogonal to this particular change.

tobya

The block seems to only work if declared before its use. This may be intentional and may be desired?

tobya · 2017-10-18T21:22:11Z

caddyfile/parse.go

+			p.definedMacros = map[string][]Token{}
+		}
+		if p.definedMacros[name] != nil {
+			p.Errf("redeclaration of previously declared macro %s", name)


This error doesn't seem to be triggered!

I declared two blocks directly after each other and both were parsed, but the second one applied with no error raised.

Ooops, I forgot to actually return the error. Good catch.

captncraig · 2017-10-19T14:28:52Z

Yeah, macros only work if pre-declared. The alternative is to pre-scan for all macros and defer loading actual server blocks until we've collected everything. I'm not sure that would be so easy to implement though. I'm ok with this limitation for now.

captncraig · 2017-10-19T14:36:48Z

@mholt, looks like the builds are all failing to find go vet. I don't feel like that is related to my change.

francislavoie · 2017-10-19T18:20:16Z

@captncraig it's fixed upstream #1929

mholt · 2017-10-19T20:48:20Z

I guess gometalinter changed...

So what do we call these exactly? Not macros. Preset blocks? Block presets? Something like that, or something else?

captncraig · 2017-10-19T23:48:12Z

I still like macros. I feel that best describes what it is. Yeah, it can be scary remembering C or Lisp or whatever, but it is also familiar to excel users or keyboard macros or things like that. Literally one word that represents a larger block.

Preset is my next favorite, but I don't feel it really captures what it is. I fear it may not be fully applicable in all foreseeable scenarios you may use this in.

Its not a default really, since it needs to be explicitly imported.

snippet may be workable, but I'm not sure.

command, shortcut, define, group, lookup, function, block, insertion. I dunno about any of those.

The only place the chosen term will show up is in the docs and in the code.

Macro still has my vote.

pgaskin · 2017-10-20T02:56:00Z

@captncraig @mholt How about includes/include.

I prefer (in decreasing order): include, macro, snippet, preset, block.

francislavoie · 2017-10-20T13:11:50Z

Considering we have import already, include doesn't fit here.

tobya · 2017-10-20T13:49:43Z

I dont like macro.

I would suggest snippet or preferably caddyfile snippet

captncraig · 2017-10-20T14:32:00Z

Snippet is growing on me. Since the name only really matters for the doc site. I think:

Reusable Snippets

Makes a tolerably good and descriptive heading.

mholt · 2017-10-20T14:44:41Z

Cool, 'snippet' it is then.

Might be worth renaming macro to snippet in the code and then I'm happy to merge this PR (subject to a final review of course)!

mholt · 2017-10-31T16:10:09Z

(To clarify, once the word "macro" has been replaced in the code with "snippet", I'll do a final review!)

mholt

Looking good! Thanks Craig. I just had one insignificant question and then it's on track for being merged.

mholt · 2017-11-02T01:46:09Z

caddyfile/parse.go

+	keys := p.block.Keys
+	// A snippet block is a single key with parens. Nothing else qualifies.
+	if len(keys) == 1 && strings.HasPrefix(keys[0], "(") && strings.HasSuffix(keys[0], ")") {
+		return true, strings.TrimSuffix(keys[0][1:], ")")


Why TrimSuffix instead of just [1:len(keys[0])-1]?

Can we explicitly check here that the snippet name inside the parens () does not include a . (dot) this will avoid 1 possible route to confusion.

What is confusing about it?

I just think it would be good to avoid

(example.com) {
log example.log "{common}"
}

as a snippet

Hmm. Well, I'm curious to see if that will actually be a problem. How about we wait and find out before we add a restriction...

I'm not sure I understand why it shouldn't be added as a restriction? It needs to be added from the beginning or not at all, otherwise you will have the potential to break installations. The only legitimate use I could see would be if someone wished to create a snippet

(snippet for all subdomains of example.com) { tls off }

I wonder how likely that is. I would suggest it is easier to remove the restriction if people feel it gets in the way rather than add it in at a later point.

Hmm, well, I see what you mean... (I think this change requires that the name in parens is only one word, and I assume your example is just an example in that sense) - what do you think @captncraig ?

mholt

LGTM

Craig Peterson added 2 commits October 13, 2017 11:04

initial implementation of caddyfile macros

425f611

add nil check

3a969bc

remove 'macro foo' syntax

dd4b3ef

tobya reviewed Oct 18, 2017

View reviewed changes

actually return error on redeclaration

68a495f

Merge branch 'master' into macros

1889049

Craig Peterson added 2 commits October 19, 2017 19:54

retrigger build

02ac1f6

Merge branch 'macros' of github.com:mholt/caddy into macros

5c9fc3a

Merge branch 'master' into macros

91a60a8

mholt changed the title ~~Caddyfile macros~~ Caddyfile snippets Oct 31, 2017

Craig Peterson added 2 commits October 31, 2017 23:56

snippets now

ad2956f

Merge branch 'macros' of github.com:mholt/caddy into macros

4ec5522

mholt reviewed Nov 2, 2017

View reviewed changes

tobya added 2 commits November 4, 2017 11:10

Merge branch 'master' into macros

9ebc11d

Merge branch 'master' into macros

345b312

mholt approved these changes Nov 7, 2017

View reviewed changes

mholt added this to the 0.10.11 milestone Nov 7, 2017

mholt mentioned this pull request Nov 9, 2017

Proposal: Upgrades to the Caddyfile #619

Closed

Merge branch 'master' into macros

8658e18

captncraig merged commit 1125a23 into master Nov 13, 2017

tobya deleted the macros branch January 29, 2018 21:04

tobya restored the macros branch January 29, 2018 21:04

francislavoie mentioned this pull request Feb 4, 2018

tls: Restructure and improve certificate management #2015

Merged

4 tasks

miekg mentioned this pull request Feb 5, 2018

kubernetes: Upstream to self coredns/deployment#55

Closed

mholt deleted the macros branch February 16, 2018 21:02

Caddyfile snippets #1921

Caddyfile snippets #1921

Conversation

captncraig commented Oct 13, 2017

1. What does this change do, exactly?

2. Please link to the relevant issues.

3. Which documentation changes (if any) need to be made because of this PR?

4. Checklist

mholt commented Oct 14, 2017 • edited Loading

miekg commented Oct 14, 2017

mholt commented Oct 14, 2017

francislavoie commented Oct 14, 2017 • edited Loading

aaronellington commented Oct 14, 2017

msfjarvis commented Oct 14, 2017

mholt commented Oct 14, 2017

francislavoie commented Oct 14, 2017

mholt commented Oct 14, 2017

elcore commented Oct 14, 2017

tobya commented Oct 15, 2017 • edited Loading

tobya commented Oct 15, 2017

mholt commented Oct 15, 2017 • edited Loading

francislavoie commented Oct 15, 2017

tobya commented Oct 15, 2017 • edited Loading

captncraig commented Oct 15, 2017

captncraig commented Oct 15, 2017

tobya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captncraig commented Oct 19, 2017

captncraig commented Oct 19, 2017

francislavoie commented Oct 19, 2017

mholt commented Oct 19, 2017

captncraig commented Oct 19, 2017

pgaskin commented Oct 20, 2017

francislavoie commented Oct 20, 2017

tobya commented Oct 20, 2017

captncraig commented Oct 20, 2017

Reusable Snippets

mholt commented Oct 20, 2017

mholt commented Oct 31, 2017

mholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mholt Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

mholt commented Oct 14, 2017 •

edited

Loading

francislavoie commented Oct 14, 2017 •

edited

Loading

tobya commented Oct 15, 2017 •

edited

Loading

mholt commented Oct 15, 2017 •

edited

Loading

tobya commented Oct 15, 2017 •

edited

Loading

mholt Nov 7, 2017 •

edited

Loading