Snapshot abstraction #1603
Snapshot abstraction #1603
Conversation
This design was inspired by the StorageBackendInterface in securesystemslib. Signed-off-by: Marina Moore <[email protected]>
This commit moves the implementation of generate_snapshot_metadata to the snapshot backend. In the future, this implementation may be simplified by keeping track of the fileinfodict in the ManifestSnapshot class instead of re-computing these on each run. Signed-off-by: Marina Moore <[email protected]>
Adds roles to the snapshot backend when they are written to disk so that the backend may maintain current snapshot information, and removes delegated targets from the snapshot backend when they are deleted TODO: add tests, more uses of add_to_snapshot Signed-off-by: Marina Moore <[email protected]>
Pull Request Test Coverage Report for Build 1305274445
💛 - Coveralls |
| <Purpose> | ||
| Create the snapshot metadata | ||
|
|
||
| <Arguments> |
There was a problem hiding this comment.
A lot of these seem to me like the belong to the class (and specifically, should correspond to different classes in some cases). At least the following:
- consistent_snapshot
- use_length
- use_hashes
| __metaclass__ = abc.ABCMeta | ||
|
|
||
| @abc.abstractmethod | ||
| def add_to_snapshot(self, rolename : str): |
There was a problem hiding this comment.
naming nit: maybe just add() (and similar throughout)?
| SNAPSHOT_FILENAME = 'snapshot' + METADATA_EXTENSION | ||
| TIMESTAMP_FILENAME = 'timestamp' + METADATA_EXTENSION | ||
|
|
||
| class SnapshotInterface(): |
There was a problem hiding this comment.
Can we distinguish between:
- the repository state (i.e., all metadata about all current packages)
- the artifact that clients will download
I think that will help make the roles/names of various classes clearer.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
||
| def generate_timestamp_metadata(snapshot_file_path, version, expiration_date, |
There was a problem hiding this comment.
So, this stays here? That kind of makes sense, but is a little asymmetric.
| def add_to_snapshot(self, rolename : str): | ||
| """ | ||
| <Purpose> | ||
| Indicate to the snapshot interface that 'rolename' |
There was a problem hiding this comment.
I'm a little confused by this. What does it mean to add a role to a snapshot?
There was a problem hiding this comment.
IIUC from the PR description, this would allow increased efficiency.
|
|
||
| class SnapshotInterface(): | ||
| """ | ||
| <Purpose> |
There was a problem hiding this comment.
I'm not sure from the code/naming whether this should have state or it's just a way to swap out implementations.
| the snapshot metadata object. | ||
|
|
||
| <Side Effects> | ||
| The 'root.json' and 'targets.json' files are read. |
There was a problem hiding this comment.
IMO nicer to move the file i/o out of this method.
So the "SnapshotInterface" would be responsible for giving you a serializable snapshot object that you could distribute.
I'll try to do that later but I'll leave this feedback for now: I totally understand you are doing this in the legacy code base because it has loads and loads of repository side code in it and adding one more argument to a function call there is almost certainly the path of least resistance... but I also hope you understand that this makes reviewing much less interesting: It's hard to see the long term value. I think the project should aim to build repository side components on top of Metadata API -- I am not sure what exactly should be included in those components but a "snapshot update handler" sounds like a prime candidate and I'd definitely be interested in collaborating on something like that. |
|
Ping @mnm678, same questions as in #1113 (comment). |
|
Closing in favor of writing this on top of the new Metadata api. |
Pull Request Test Coverage Report for Build 1305274445Warning: 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 |
This moves generation of snapshot metadata into a new interface so that variations on snapshot generation like snapshot merkle trees in TAP 16 and snapshot RSA accumulators can be added to the interface and more easily swapped out.
This is a draft pr that is missing a few things:
add_to_snapshotandremove_from_snapshotin the implementation ofManifestSnapshot(for example, maintainingfileinfodictinManifestSnapshot, rather than re-computing the whole dict every time)I would appreciate any general feedback about the abstraction design, whether this is a good idea, or thoughts about what should/shouldn't be moved to this snapshot object.
cc @znewman01