Clarify generate_targets_metadata() #1033
Conversation
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 <jlock@vmware.com>
It should be possible to request consistent targets metadata when using existing fileinfo, because the metadata itself doesn't differ. Consistent targets, copies of the file with the file's hash prepended to the target's file name, should exist in the targets directory but are not listed in the targets metadata. Throwing an exception here prevents enabling consistent metadata to be written when using existing fileinfo structures, instead we log that the consistent copies of the target files are expected to exist in the repository's targets directory and continue assuming they are present. Signed-off-by: Joshua Lock <jlock@vmware.com>
| for target, fileinfo in six.iteritems(target_files): | ||
|
|
||
| # Ensure all fileinfo entries in target_files have a non-empty hashes dict | ||
| if not fileinfo.get('hashes', None): | ||
| raise securesystemslib.exceptions.Error('use_existing_hashes option set' | ||
| ' but no hashes exist in roledb for ' + repr(target)) | ||
|
|
||
| if fileinfo.get('length', -1) < 0: | ||
| # and a non-empty, non-zero, length | ||
| if fileinfo.get('length', -1) < 1: |
There was a problem hiding this comment.
I think it doesn't make sense to have zero length targets files, but maybe this should be restored to the prior check of: if len < 0 ?
There was a problem hiding this comment.
I think it doesn't make sense to have zero length targets files, but maybe this should be restored to the prior check of:
if len < 0?
@joshuagl, the classical __init__.py file is a good example for a target file with zero length.
There was a problem hiding this comment.
You're right, thanks for pointing out that case @lukpueh. In which case I should restore this to the prior if len < 0. Will force push a fix later today.
There was a problem hiding this comment.
Oh, I already restored the behaviour for checking len < 0 in #1034, which contains an updated version of this change. Apologies for the confusion.
There was a problem hiding this comment.
No need to apologize. I have no business commenting on closed PRs. :D
There was a problem hiding this comment.
It looks like setting both write_consistent_targets and use_existing_fileinfo just ignores the write_consistent_targets, which I'm not sure would be the expected behavior. If the caller is responsible for writing the consistent files, it might make more sense for them to set write_consistent_targets to false here, then write the consistent files afterwards. But I might be misunderstanding the goal.
| @@ -1240,9 +1251,12 @@ def generate_targets_metadata(targets_directory, target_files, version, | |||
| securesystemslib.formats.BOOLEAN_SCHEMA.check_match(write_consistent_targets) | |||
| securesystemslib.formats.BOOLEAN_SCHEMA.check_match(use_existing_fileinfo) | |||
|
|
|||
| # If the caller requested consistent targets *and* supplied their own fileinfo | |||
There was a problem hiding this comment.
| # If the caller requested consistent targets *and* supplied their own fileinfo | |
| # If the caller requested consistent targets *and* supplied their own fileinfo, |
|
@lukpueh already resolved this issue at the right level by modifying Apologies for the noise. I will close this PR and include the first patch in a later PR. |
Fixes issue #: N/A
Description of the changes being introduced by the pull request:
generate_targets_metadata(), in the docstring and code comments, to make theuse_existing_fileinfoandwrite_consistent_targetsarguments, and their interactions, clearer.generate_targets_metadata()so that bothuse_existing_fileinfoandwrite_consistent_fileinfocan be used together, as required by Warehouse for PEP 458Please verify and check that the pull request fulfills the following
requirements: