Experimental client: use bundle#1396
Experimental client: use bundle#1396jku merged 3 commits intotheupdateframework:experimental-clientfrom
Conversation
|
The constants could use some thought: We definitely don't want to keep using tuf.settings, but I guess the question is, should these be instance variables in Updater instead of module level ones: Maybe it does not hurt if they were instance variables instead. They don't even need to be optional constructor arguments (as initialization does not use any of them: caller can initialize the Updater and then modify the values). |
228a01c to
05421e0
Compare
Use the MetadataBundle to verify metadata validity. * Updater now handles reading metadata files (from filesystem as well as network * Updater feeds bytes to MetadataBundle for verification * Updater persists data on disk after it had been verified Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
Add test for a refresh with just a local root.json. Remove unused code. Add docstrings for raised exceptions, add TODOs for the missing exception handling. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
05421e0 to
2bbf5bc
Compare
|
After I made |
sechkova
left a comment
There was a problem hiding this comment.
The constants could use some thought: We definitely don't want to keep using tuf.settings, but I guess the question is, should these be instance variables in Updater instead of module level ones:
MAX_ROOT_ROTATIONS = 32 MAX_DELEGATIONS = 32 DEFAULT_ROOT_MAX_LENGTH = 512000 # bytes DEFAULT_TIMESTAMP_MAX_LENGTH = 16384 # bytes DEFAULT_SNAPSHOT_MAX_LENGTH = 2000000 # bytes DEFAULT_TARGETS_MAX_LENGTH = 5000000 # bytesMaybe it does not hurt if they were instance variables instead. They don't even need to be optional constructor arguments (as initialization does not use any of them: caller can initialize the Updater and then modify the values).
I guess the fanciest option is that constructor takes an optional argument dataclass UpdaterOptions with those attributes ...
Given the proposed structure in #1397 (comment) where the idea is to expose only the Updater class in the client __init__ , probably the constants need to be inside the class (or the fancier options). However this can happen when solving #1397.
I left some minor comments but I in general I like how Updater looks now.
| f"{len(role_names)} roles left to visit, but allowed to ", | ||
| f"visit at most {MAX_DELEGATIONS} delegations.", | ||
| ) | ||
| logger.debug(msg) |
There was a problem hiding this comment.
This trick of mine with logging needs to be reverted now that we agreed on %s-strings in the logging calls.
But not in this PR. I'll note it for myself.
|
|
||
| return intermediate_targets | ||
| data = self._load_local_metadata(role) | ||
| self._bundle.update_delegated_targets(data, role, parent_role) |
There was a problem hiding this comment.
Using self._bundle.update_delegated_targets here leaves MetadataBundle.update_targets() unused.
Not a big deal but mentioning it.
|
Addressed the resolved discussions. Also checked that consistent_snapshots=None seems to work fine (using a falsy test) |
joshuagl
left a comment
There was a problem hiding this comment.
This looks very concise, nice. It took me a couple of reads to figure out why the fetching of metadata only happened in the exception handler of a load. That might want some attention in terms of comments and/or function names? 🤔
I also noticed there are some unhandled exceptions which I am not sure whether it is reasonable to leave those to a caller to handle or not. Would welcome thoughts on that.
Overall, very solid. LGTM.
| self._bundle.update_snapshot(data) | ||
| self._persist_metadata("snapshot", data) |
There was a problem hiding this comment.
Here, and other _load_*() functions, the exceptions raised by MetadataBundle.update_snapshot and the open and write calls in _persist_metadata become errors the user must handle. The former seems reasonable enough, I'm less certain about the latter. Especially when get_one_valid_targetinfo() -> _preorder_depth_first_walk() could result in a myriad of errors (download, file open/write, ???).
Probably a topic for #1312 ?
There was a problem hiding this comment.
yes I vote for handling in 1312. I think the three classes of exceptions (repository / download / file IO) are logical and also the granularity that I could see a serious client wanting to handle (or not handle as might be reasonable with e.g. file IO ).
Mostly remove comments that provide little value after all the changes. Also remove a unused variable from a test. Signed-off-by: Jussi Kukkonen <jkukkonen@vmware.com>
ad4e671 to
d35fc27
Compare
Use the MetadataBundle in the new Updater:
Fixes #1304
Fixes #1320
Fixes #1319