Skip to content

caddyfile: Fix import replacing unrelated placeholders#4129

Merged
mholt merged 2 commits intocaddyserver:masterfrom
francislavoie:fix-import-placeholders
Apr 23, 2021
Merged

caddyfile: Fix import replacing unrelated placeholders#4129
mholt merged 2 commits intocaddyserver:masterfrom
francislavoie:fix-import-placeholders

Conversation

@francislavoie
Copy link
Member

See https://caddy.community/t/snippet-issue-works-outside-snippet/12231

So it turns out that NewReplacer() gives a replacer with some global defaults (like {env.*} and some system and time placeholders), which is not ideal when running import because we just want to replace {args.*} only, and nothing else.

See https://caddy.community/t/snippet-issue-works-outside-snippet/12231

So it turns out that `NewReplacer()` gives a replacer with some global defaults (like `{env.*}` and some system and time placeholders), which is not ideal when running `import` because we just want to replace `{args.*}` only, and nothing else.
@francislavoie francislavoie added the bug 🐞 Something isn't working label Apr 22, 2021
@francislavoie francislavoie added this to the v2.4.0 milestone Apr 22, 2021
@francislavoie francislavoie requested a review from mholt April 22, 2021 18:19
@francislavoie
Copy link
Member Author

I should probably add a test for this, but it can be merged without. It's on my TODO.

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.

Interesting. Thanks for the patch. Can't think of a better way to do it, so let's roll with it.

@mholt mholt merged commit a8d4527 into caddyserver:master Apr 23, 2021
@francislavoie francislavoie deleted the fix-import-placeholders branch April 23, 2021 03:04
@Dulanic
Copy link

Dulanic commented Apr 28, 2021

Not sure that this corrected it. It is passing the env variable name not the env variable value. I looked at the autosave.json and all the other env variables show the value, but this same one just shows the name instead.

image

@francislavoie
Copy link
Member Author

That's working as intended. The {env.*} are replaced at runtime, and only if the module being configured passes it through the replacer. The {$ENV} style are replaced at Caddyfile adapt time, i.e. before it's transformed into JSON, and will work anywhere, but will leak the values to the autosave.json

@francislavoie
Copy link
Member Author

FYI @greenpau make sure you're running the replacer on all config values that are relevant either during ServeHTTP (if it makes sense for the value to be dynamic) or during Provision (if it only makes sense to be a single value for the lifetime of the server).

@Dulanic
Copy link

Dulanic commented Apr 28, 2021

OK, I'll work /w @greenpau under his issue ticket since I'm still seeing errors. https://github.com/greenpau/caddy-auth-portal/issues/122#issuecomment-828481342

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug 🐞 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants