Expand env variables in volume paths in configuration file #192
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
For more context on this change, see #191.
While working on this feature, I hit a case where the 'warnings' module was referenced, but this caused a NameError because the module was never imported. AFAICT this issue has been present since the code was introduced, but it was never noticed because it was behind a conditional that was never satisfied.
xanarin
force-pushed
the
191-expand-vars-in-volumes
branch
from
3a85fce
to
14019c8
Compare
JonathonReinhart
requested changes
Jan 10, 2022
Responding to a comment on GH-192, the code now calls string.Template.substitute, which has more correct behavior because it won't skip expanding any unset environmental variables. Now, the behavior is to raise an exception, similar to bash's nounset (set -u) option. This code was placed in 'utils.py' so that it can be used elsewhere in Scuba and not just config-parsing code.
JonathonReinhart
requested changes
Jan 12, 2022
Also, change all instances of 'environmental variables' to 'environment variables', as that is the accepted terminology (which I didn't know up to this point!)
This includes mocking the environment variables instead of modifying the global environment during unit tests, and adding extra tests for the underlying expansion function in `utils.py`.
|
I added documentation for this functionality change in 850f6b0. The only backward-incompatability issue we may have is that if any users have the |
JonathonReinhart
approved these changes
Jan 12, 2022
|
Thanks a lot @xanarin for your patience and the high-quality code! |
| @@ -166,6 +166,19 @@ of volume options: | |||
| hostpath: /host/foo | |||
| options: ro,cached | |||
|
|
|||
| The keys used in volume mappings can contain environment variables **that are | |||
There was a problem hiding this comment.
It's actually not just the keys (container path), but also the host path, which can contain environment vars (as shown in the example)
| volumes: | ||
| $TEST_HOME/.config/application1: $TEST_HOME/.config/application1 | ||
|
|
||
| Note that because variable expansion is now applied to all volume keys, if one |
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.
For more context on this change, see #191.