Return absolute path without resolving symlinks #4341
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
After the rewrite of util.abs_path to use pathlib instead of os.path the function now resolves symlinks in the given path whereas it did not do before. This breaks setups where the molecule.yml file in a scenario is a symlink. Unfortunately pathlib is not a drop-in replacement for os.path (see https://docs.python.org/3/library/pathlib.html#comparison-to-the-os-and-os-path-modules) Path.absolute does not resolve symlink but does not reduce /../ segments in the path. Path.resolve does reduce /../ path elements, but also resolves symlinks. There is no method or combination of methods in the Path object that can deliver the same functionality as os.path.abspath. Hence the code reverts back to using os.path.abspath, but keeps accepting Path objects as input and returns the absolute path as such in these cases. Signed-off-by: Ruediger Pluem <[email protected]>
Qalthos
requested changes
Dec 12, 2024
This is unavoidable due to incompatibilities between abspath and what pathlib can support
Qalthos
approved these changes
Dec 12, 2024
|
Thanks for finding this! |
|
This was the fastest PR review and approval I ever faced. Thank you very much 😆 |
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.
After the rewrite of
util.abs_pathto usepathlibinstead ofos.path(000d200, c20dc08) the function now resolves symlinks in the given path whereas it did not do before.This breaks setups where the
molecule.ymlfile in a scenario is a symlink. E.g./common-files/molecule.yml
/roles/role1/molecule/default/molecule.yml -> /common-files/molecule.yml
/roles/role1/molecule/some-scenario/molecule.yml -> /common-files/molecule.yml
/roles/role2/molecule/default/molecule.yml -> /common-files/molecule.yml
Where a common
molecule.ymlcan be used for multiple roles / scenarios and where changes to this file should reflect everywhere.In this case
molecule testexecuted in/roles/role1returnsScenario 'default' not found. Exiting.ormolecule test -s some-scenarioexecuted in/roles/role1returnsScenario 'some-scenario' not found. Exiting.(seemolecule/src/molecule/scenarios.py
Line 127 in f58ac30
Unfortunately
pathlibis not a drop-in replacement foros.path(see https://docs.python.org/3/library/pathlib.html#comparison-to-the-os-and-os-path-modules).Path.absolute() does not resolve symlink but does not reduce
/../segments in the path. Path.resolve() does reduce/../path elements, but also resolves symlinks.There is no method or combination of methods in the
Pathobject that can deliver the same functionality as os.path.abspath(). Hence the code reverts back to usingos.path.abspath(), but keeps acceptingPathobjects as input and returns the absolute path as such in these cases.