Skip to content
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

Move all TUF mgmt code to RepositoryService (WIP) #1

Conversation

lukpueh
Copy link
Collaborator

@lukpueh lukpueh commented Jun 25, 2022

Alternative design for pypi#10870
Motivated by pypi#10870 (comment)


Reduce complexity and lines of code by implementing all current TUF
management code inside RepositoryService, and thus removing one
level of abstraction, previously implemented by the
MetadataRepository class.

NOTE: This patch is marked WIP, as it has not removed all
references to MetadataRepository, nor adopted the tests.
Moreover, it still needs review in terms of correctness wrt PEP458.
But the reduced complexity should make this easier.

NOTE: (2) There is more potential for DRY code, see reoccurring
_bump_version; _bump_expiration; _sign; _persist; and
_update_snapshot; _update_timestamp; call chains. For this
iteration of the patch, I chose verbosity/explicitness over saving
a few more lines. But maybe both can be achieved.

Signed-off-by: Lukas Puehringer [email protected]

Reduce complexity and lines of code by implementing all current TUF
management code inside `RepositoryService`, and thus removing one
level of abstraction, previously implemented by the
`MetadataRepository` class.

NOTE: This patch is marked WIP, as it has not removed all
references to `MetadataRepository`, nor adopted the tests.
Moreover, it still needs review in terms of correctness wrt PEP458.
But the reduced complexity should make this easier.

NOTE: (2) There is more potential for DRY code, see reoccurring
`_bump_version; _bump_expiration; _sign; _persist;` and
`_update_snapshot; _update_timestamp;` call chains. For this
iteration of the patch, I chose verbosity/explicitness over saving
a few more lines. But maybe both can be achieved.

Signed-off-by: Lukas Puehringer <[email protected]>
Copy link

@joshuagl joshuagl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the direction this is going. The LOC reduction is compelling alone, the reduction in abstractions feels helpful too.

I think I am at least partially responsible for the decision to have a two-tier abstraction – I thought it might help identify repository API for python-tuf. Mea culpa.

NOTE: This patch is marked WIP, as it has not removed all
references to MetadataRepository, nor adopted the tests.
Moreover, it still needs review in terms of correctness wrt PEP458.
But the reduced complexity should make this easier.

Reviewing against the PEP is hard, not least of all because the PEP is ~39 pages printed which includes a mixture of TUF overview and TUF+Warehouse architecture. I wonder if we can extract the technical details the implementation needs to match into an ancillary document?

NOTE: (2) There is more potential for DRY code, see reoccurring
_bump_version; _bump_expiration; _sign; _persist; and
_update_snapshot; _update_timestamp; call chains. For this
iteration of the patch, I chose verbosity/explicitness over saving
a few more lines. But maybe both can be achieved.

I generally favour DRY, but let's capture the technical details that the PEP requires before we commit to anything here?

Comment on lines +406 to +407
# FIXME: Possible performance gain by updating 'snapshot' right here, to
# omit creation of massive list and iterating over all 'bin-n' roles twice.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is worthy of generating some numbers to understand the performance. The counter-argument is that we don't want to bump version & sign as we add each bin-n role. Perhaps the right approach is to do the operation in phases so that we can a) add the MetaFile as we iterate over bins here b) _bump_expiry(), _bump_version(), _sign(), _persist() only once after the loop.

@kairoaraujo kairoaraujo merged commit a9ea204 into kairoaraujo:refactoring_pr_tuf_initialization Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants