Fix directory being empty in ensure_parent_dir #260
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
2 similar comments
MVrachev
force-pushed
the
fix-empty-directory
branch
from
48d2106 to
495346f
Compare
joshuagl
reviewed
Jul 21, 2020
There was a problem hiding this comment.
Thanks for finding and fixing this Martin. Looks like I removed similar logic to the change you implemented when I ported the util methods to the abstract storage backend in 8deb29f, only it wasn't a problem at the time because create_folder would silently handle an empty string.
I have added a comment to the test you've added for this inline, otherwise this looks good to me 👍
tests/test_util.py
Outdated
| # then the directory of that filepath will be an empty string. | ||
| # We want to make sure that securesyslib.storage.create_folder() | ||
| # won't be called with an empty string and thus raise an exception. | ||
| securesystemslib.util.ensure_parent_dir('a.txt') |
There was a problem hiding this comment.
I think it would be simpler if we just moved this test out of the for loop, rather than adding extra handling and a large comment to the for loop. What do you think?
MVrachev
force-pushed
the
fix-empty-directory
branch
from
495346f to
c7a4339
Compare
If you pass filename like "a.txt" to securesyslib.util.ensure_parent_dir function, then the directory of the file will be '' and when calling securesyslib.storage.create_folder() with '' argument an exception will be thrown because of the latest changes in securesyslib. Signed-off-by: Martin Vrachev <[email protected]>
MVrachev
force-pushed
the
fix-empty-directory
branch
from
c7a4339 to
247cf9b
Compare
|
I agree with your comment and I updated the pr with those changes. |
joshuagl
approved these changes
Aug 4, 2020
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.
Fixes issue #: None
Description of the changes being introduced by the pull request:
If you pass filename like "a.txt" to
securesyslib.util.ensure_parent_dir function, then the directory
of the file will be '' and when calling
securesyslib.storage.create_folder() with '' argument
an exception will be thrown because of the latest changes
in securesyslib.
Please verify and check that the pull request fulfils the following
requirements: