-
Notifications
You must be signed in to change notification settings - Fork 278
Update ngclient to return loaded metadata #1680
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
Update ngclient to return loaded metadata #1680
Conversation
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.
I left some small suggestions.
Will review it tomorrow more precisely.
Pull Request Test Coverage Report for Build 1476029676Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
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 pretty good... in addition to the annotation comment Martin made (that applies everywhere where we know the contained type):
- it looks like we'll only use the return values from update_delegated_targets() but I agree it still makes sense to make all
TrustedMetadataSet.update_*()
methods return values in the same way: it keeps the TrustedMetadataSet API consistent TrustedMetadataSet.update_targets()
does not return a value yet, it should- I wonder about
Updater._load_*()
functions though -- they are internal to Updater... maybe it does not make sense to modify a function unless we actually want to use the return value. So this would mean we'd only modify _load_targets(), and not the other methods. I'm fine with other opinions in this one though
Thanks for the reviews! The reason I left |
We agreed with @jku on returning metadata from all |
f778d36
to
ff5ddce
Compare
I realized that probably there is no sense in annotating the return type as |
We should return Metadata[T] and it seems we do, I don't see a problem. If we returned Root from |
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, looks nice. Quite a bit of work to avoid one single dict lookup, but I think the end result is a better TrustedMetadataSet API (and might be useful in the repository work later).
I just had one nit about the return value docstring
Apologize for my review here. I made a wrong assumption. |
ff5ddce
to
a0d412f
Compare
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 addressing my comments!
LGTM!
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.
This looks good to me. Left a really minor comment but no need to update just for that
This changes `TrustedMetadataSet` to return new trusted Metadata on successful calls of the `update_<role>` functions and also changes `Updater._load_targets` to return loaded metadata as well Signed-off-by: Ivana Atanasova <iyovcheva@iyovcheva-a02.vmware.com>
a0d412f
to
9c2bf6e
Compare
This changes
TrustedMetadataSet
to return new trusted Metadataon successful calls of the
update_<role>
functions and alsochanges
Updater._load_targets
to return loaded metadata as wellFixes #1507