Fix two import problems: snippets and import literals. #2205
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Two problems are fixed by this code simplification: 1. Snippets defined in one import file are strangely not available in another. 2. If an imported file had a directive with an argument "import", then the rest of the tokens on the line would be converted to absolute filepaths. An example of caddyserver#2 would be the following directive in an imported file: basicauth / import secret In this case, the password would actually be an absolute path to the file 'secret' (whether or not it exists) in the directory of the imported Caddyfile. The problem was the blind token processing to fix import paths in the imported tokens without considering the context of the 'import' token. My first inclination was to just add more context (detect 'import' tokens at the beginning of lines and check the value tokens against defined snippets), however I eventually realized that we already do all of this in the parser, so the code was redundant. Instead we just use the current token's File property when importing. This works fine with imported tokens since they already have the absolute path to the imported file! Fixes caddyserver#2204
augustoroman
force-pushed
the
snippet-fixes
branch
from
853a60b to
def73ff
Compare
|
Ooo, nice! I'm looking forward to going over these changes. Of course, I invite any other collaborators to give it a review too, since they'll probably be able to sooner than I am. |
theodesp
reviewed
Jun 16, 2018
caddyfile/parse_test.go
Outdated
| } | ||
|
|
||
| func TestImportedFilesIgnoreNonDirectiveImportTokens(t *testing.T) { | ||
| file2 := writeStringToTempFileOrDie(t, ` |
There was a problem hiding this comment.
Can you name it just fileName for readability?
This makes it more clear how the import side effect is detrimental.
theodesp
approved these changes
Jun 20, 2018
|
ping? Is this ready to merge? |
|
Thanks @augustoroman -- I love how the solution was basically to delete code and add tests. If you'd like to be a collaborator on the project to help review PRs and issues and get push access, let me know. :) |
admpub
added a commit
to admpub/caddy
that referenced
this pull request
Jul 1, 2018
caddyfile: Fix multi-file snippets and import literals. (caddyserver#2205)
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two problems are fixed by this code simplification:
another.
the rest of the tokens on the line would be converted to absolute
filepaths.
An example of 2 would be the following directive in an imported file:
In this case, the password would actually be an absolute path to the
file 'secret' (whether or not it exists) in the directory of the imported
Caddyfile.
The problem was the blind token processing to fix import paths in the
imported tokens without considering the context of the 'import' token.
My first inclination was to just add more context (detect 'import' tokens
at the beginning of lines and check the value tokens against defined
snippets), however I eventually realized that we already do all of this
in the parser, so the code was redundant. Instead we just use the current
token's File property when importing. This works fine with imported tokens
since they already have the absolute path to the imported file!
Fixes #2204