-
Notifications
You must be signed in to change notification settings - Fork 278
Fix and better test abstract files and directories support #1034
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix and better test abstract files and directories support #1034
Conversation
@@ -550,6 +557,168 @@ def test_get_filepaths_in_directory(self): | |||
|
|||
|
|||
|
|||
def test_writeall_abstract_storage(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, @joshuagl! I have 2 minor requests inline, but this is good to go regardless.
FYI I ran the new tests excluding your last commit and they successfully catch the then missing AbstractFileStorage support in generate_timestamp_metadata
. 💯
@contextmanager | ||
def get(self, filepath): | ||
file_object = open(filepath + '.tst', 'rb') | ||
yield file_object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇 Finally we have a yield
in python-tuf. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙇
file_object = open(filepath + '.tst', 'rb') | ||
yield file_object | ||
file_object.close() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind adding a blank line for consistency with the rest of TestStorageBackend
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eagle eyed review. Fixed in a force-pushed update to the patch, thank you.
tests/test_repository_tool.py
Outdated
class TestStorageBackend(securesystemslib.storage.StorageBackendInterface): | ||
""" | ||
An implementation of securesystemslib.storage.StorageBackendInterface | ||
which mutates filenames on put()/get(), such that trying to read the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice way of testing this. Would you mind adding one sentence that elaborates on the mutation, i.e. filename
in memory vs. filename + ".tst"
on disk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to the following in a force-pushed change
An implementation of securesystemslib.storage.StorageBackendInterface which mutates filenames on put()/get(), translating filename in memory to filename + '.tst' on-disk, such that trying to read the expected/canonical file paths from local storage doesn't find the TUF metadata files.
Clarify, through the docstrings and code comments, the expected behaviour of generate_targets_metadata() and the interactions of the use_existing_fileinfo and write_consistent_targets parameters. Signed-off-by: Joshua Lock <[email protected]>
Add a class implementing StorageBackendInterface for testhing which mutates filenames on put()/get(), such that trying to read the expected file paths for TUF metadata from the local filesystem doesn't find the files. Use this class when creating a repository and writing metadata to test abstract files and directories support for metadata writing. Signed-off-by: Joshua Lock <[email protected]>
This was erroneously absent in PR 1024, which added support for abstract files and directories. Resolve by adding a storage_backend argument to generate_timestamp_metadata() and using it so that the fileinfo (hashes and length) for the snapshot file can be generated for a snapshot metadata file on any supported storage. Signed-off-by: Joshua Lock <[email protected]>
77dfe0b
to
5e5c598
Compare
Fixes issue #: N/A or a continuation of #1009
Description of the changes being introduced by the pull request:
generate_targets_metadata()
generate_timestamp_metadata()
Please verify and check that the pull request fulfills the following
requirements: