Normalization of configuration paths #19708
Merged
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
Previously we were doing this rather inconsistently: some commands would do it and others would not. By doing it here we ensure we always apply the same normalization, regardless of which operation we're running. This normalization is mostly for cosmetic purposes in error messages, but it also ends up being used to populate path.module and path.root and so it's important that we always produce consistent results here so that we don't produce flappy changes as users work with different commands. The fact that thus mutates a data structure as a side-effect is not ideal but this is the best place to ensure it always gets applied without doing any significant refactoring, since everything after this point happens in the backend package where the normalizePath method is not available.
Previously we used the native slash type for the host platform, but that
leads to issues if the same configuration is applied on both Windows and
non-Windows systems.
Since Windows supports slashes and backslashes, we can safely return
always slashes here and require that users combine the result with
subsequent path parts using slashes, like:
"${path.module}/foo/bar"
Previously the above would lead to an error on Windows if path.module
contained any backslashes.
This is not really possible to unit test directly right now since we
always run our tests on Unix systems and filepath.ToSlash is a no-op on
Unix. However, this does include some tests for the basic behavior to
verify that it's not regressed as a result of this change.
This will need to be reported in the changelog as a potential breaking
change, since anyone who was using Terraform _exclusively_ on Windows may
have been using expressions like "${path.module}foo\\bar" which they will
now need to update.
This fixes #14986.
mildwonkey
approved these changes
Dec 19, 2018
jbardin
approved these changes
Dec 19, 2018
|
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
ghost
locked and limited conversation to collaborators
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This PR includes two adjustments to how we normalize configuration paths.
The first is general and applies both to the paths we show in diagnostic messages and the paths we return from
path.rootandpath.module: previously we were normalizing these to relative paths in some commands but not others, which is both cosmetically weird and potentially problematic sincepath.modulemight return something different depending on which Terraform command you are running. We will now normalize these to be relative paths in all cases.The second applies only to the
path.references in configuration, making them always use forward slashes even on Windows systems. This addresses #14986 by relying on the fact that Windows supports both slash types as long as they are used consistently within a path. This means that moving forward all constructed paths in Terraform configurations should use forward slashes, like"${path.module}/foo/bar", which will produce correct results on all supported platforms.These two changes compliment each other by allowing paths to remain consistent across platforms. Making
path.moduleandpath.rootappear relative avoids them ending up containing platform-specific prefixes likec:\or/homethat would cause churn when applying the same configuration on different hosts.Since the slash normalization behavior applies only to Windows and we run our tests on Unix there's no direct unit testing of that behavior, but I added tests for the more general behavior of
path.moduleandpath.rootnonetheless, to show that they still work as before on Unix systems.