Update ngclient to return loaded metadata

[ivanayov]

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

Fixes #1507

• The code follows the Code Style Guidelines
• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature - n/a

[MVrachev]

I left some small suggestions.
Will review it tomorrow more precisely.

[coveralls]

Pull Request Test Coverage Report for Build 1490182832

• 18 of 18 (100.0%) changed or added relevant lines in 2 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.002%) to 97.466%

---

Totals
Change from base Build 1476295870:	0.002%
Covered Lines:	3953
Relevant Lines:	4037

---

💛 - Coveralls

[jku]

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

[ivanayov]

Thanks for the reviews!

The reason I left _load_root unchanged is because it needs to collect metadata from all root versions and I'm not sure if we need such collection and if we actually want to create it. The only reason for creating one is just to make all _load_* functions unified, which I don't find strong enough argument, but I'd be happy to hear feedback if it really makes sense.

[ivanayov]

We agreed with @jku on returning metadata from all update_* functions to keep the API consistent and return it only from _load_delegated_targets within the Updater, as the output from others is never used.

[MVrachev]

I realized that probably there is no sense in annotating the return type as Metadata[Root], Metadata[Timestamp]or otherMetadata[T]when we actually returnRoot, Timestampfromupdate_root, update_timestamp`, etc.
Do you agree with me @jku?

[jku]

I realized that probably there is no sense in annotating the return type as Metadata[Root], Metadata[Timestamp]or otherMetadata[T]when we actually returnRoot, Timestampfromupdate_root, update_timestamp`, etc. Do you agree with me @jku?

We should return Metadata[T] and it seems we do, I don't see a problem.

If we returned Root from update_root() mypy should complain as well.

[jku]

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

[MVrachev]

I realized that probably there is no sense in annotating the return type as Metadata[Root], Metadata[Timestamp]or otherMetadata[T]when we actually returnRoot, Timestampfromupdate_root, update_timestamp`, etc. Do you agree with me @jku?

Apologize for my review here. I made a wrong assumption.
You should annotate the return type as Metadata[Root], instead of Root as in update_root you are returning new_root which is initialized as new_root = Metadata[Root].from_bytes(data).
It's the same situation for the other cases I discuss here as well.

[MVrachev]

Thanks for addressing my comments!
LGTM!

[jku]

This looks good to me. Left a really minor comment but no need to update just for that