Make hashes, length and delegations optional + improvements #1367
Conversation
There was a problem hiding this comment.
Thanks. The actual API changes look fine to me
- length+hashes commit message is misleading in that this does not seem like a breaking change to me (looks like de/serialize probably works fine right now). Am I missing something?
- I prefer no TODO comments for existing issues (without a very good reason). If the issue is closed as wontfix, who fixes the TODOs?
- is there a benefit to using restructuredtext in docstrings -- do you (or are we planning to) run this through sphinx somewhere?
As a process comment I think multiple PRs instead of a single one might have increased the throughput here: now we may be stuck discussing a comment about _type when we could just merge the API changes.
| self.meta["snapshot.json"] = {"version": version} | ||
|
|
||
| if length is not None: | ||
| self.meta["snapshot.json"]["length"] = length | ||
|
|
||
| if hashes is not None: | ||
| self.meta["snapshot.json"]["hashes"] = hashes |
There was a problem hiding this comment.
nit: Not a huge fan of three separate lookups of "snapshot.json" key... it's not a major deal here, just pointing it out
There was a problem hiding this comment.
I imagine this would be something we would change when we fix #1333.
There was a problem hiding this comment.
I don't think that's related: you don't need more than a single lookup even now but it is a small detail
MVrachev
force-pushed
the
improvements
branch
2 times, most recently
from
19cb525 to
89ee462
Compare
|
Rebased because of conflicts with develop.
|
|
I think it looks good but I wanted to check if the documentation change is intentional? it's not mentioned in the commit message and the opposite change was done quite recently I think |
Add a use case for the root class to be tested in test_generic_read and test_read_write_read_compare tests in test_apy.py Signed-off-by: Martin Vrachev <[email protected]>
As per the specification (v1.0.1) length and hashes fields in timestamp and snapshot metadata are optional. We have implement this in the older API (see theupdateframework#1031) and we should implement it in the new API. Signed-off-by: Martin Vrachev <[email protected]>
From the reST/sphinx docs: https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks I added new lines and an identation where it was missed. Signed-off-by: Martin Vrachev <[email protected]>
According to the spec, delegations in targets are marked as optional: https://theupdateframework.github.io/specification/latest/#file-formats-targets and a pr, clarifying that even more, is approved: theupdateframework/specification#157. This is a possible breaking change. Signed-off-by: Martin Vrachev <[email protected]>
Fixes: #1362
Description of the changes being introduced by the pull request:
Please note: this pr contains one breaking change:
delegationsoptional forTargetsThe idea of this pr came from improvements proposed/discussed in pr #1329.
Those improvements are proposed together (because they are small enough to be bundled) in a
separate pr and with that to simplify the review on pr #1329.
Please verify and check that the pull request fulfills the following
requirements: