Skip to content

Optimize the calculation of length and hashes - #1097

Merged
joshuagl merged 1 commit into
theupdateframework:developfrom
MVrachev:optimize-length-hashes
Aug 18, 2020
Merged

Optimize the calculation of length and hashes#1097
joshuagl merged 1 commit into
theupdateframework:developfrom
MVrachev:optimize-length-hashes

Conversation

@MVrachev

@MVrachevMVrachev commented Aug 4, 2020

Copy link
Copy Markdown
Collaborator

Fixes issue #: None

Description of the changes being introduced by the pull request:
After we had given the option to use or not hashes and length
for timestamp and snapshot roles, it's good to make sure we are
calculating them only when they are needed.

This optimization could be important for the bigger tuf adopters.

Please verify and check that the pull request fulfills the following
requirements:

  • 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

@MVrachev

MVrachev commented Aug 4, 2020

Copy link
Copy Markdown
CollaboratorAuthor

This pr uses the latest changes in securesyslib which are not yet released and that's why it fails on all python environments expect sslib-master.

Those changes don't need additional unit tests because there are multiple tests in tuf/tests/test_repository_lib.py which are already testing the different situations when length and hashes are/aren't used for timestamp and snapshot roles.

@joshuagljoshuagl added the securesystemslib Requires corresponding implementation in securesystemslib label Aug 4, 2020
@joshuagl

joshuagl commented Aug 4, 2020•
edited
Loading

Copy link
Copy Markdown
Member

We'll need to update the minimum required securesystemslib in setup.py to be whichever released version includes the util.get_file_details() split

@jku

jku commented Aug 4, 2020

Copy link
Copy Markdown
Member

What happened in the appveyor builds? The tests seem to take a couple of minutes on each run but the CI build won't finish until an hour later (which I think is appveyors timeout): 5 hours for the whole thing...

@trishankatdatadog

trishankatdatadog commented Aug 4, 2020

Copy link
Copy Markdown
Contributor

@MVrachev let's mark this as a draft PR until ready, please? Thanks!

@MVrachev
MVrachev marked this pull request as draft August 5, 2020 12:54
@MVrachev

MVrachev commented Aug 5, 2020

Copy link
Copy Markdown
CollaboratorAuthor

I converted it to a draft pr until a new release of securesyslib is released which includes the dependent functionality.

joshuagl
joshuagl approved these changes Aug 7, 2020
View reviewed changes

@joshuagljoshuagl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Great change, thanks @MVrachev. I'll try and ensure a securesystemslib release is made in the next week or so and then we should be good to merge this.

Comment threadtuf/repository_lib.py Outdated
Comment threadtuf/repository_lib.py Outdated
@MVrachev
MVrachevforce-pushed the optimize-length-hashes branch from 105d8bc to b0dda36CompareAugust 17, 2020 11:50
@MVrachev
MVrachev marked this pull request as ready for review August 17, 2020 11:56
@MVrachev

MVrachev commented Aug 17, 2020•
edited
Loading

Copy link
Copy Markdown
CollaboratorAuthor

I updated this pr with the latest changes introduced on the develop branch and the necessary functions from securesyslib version 0.16.0 are already available.

The pr is ready for reviews.

PS: Will address the comments from Joshua.

Martin Vrachev
Optimize the calculation of length and hashes
ba87454
After we had given the option to use or not hashes and length
for timestamp and snapshot roles, it's good to make sure we are
calculating them only when they are needed.
This optimization could be important for the bigger tuf adopters.
Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
@MVrachev
MVrachevforce-pushed the optimize-length-hashes branch from b0dda36 to ba87454CompareAugust 17, 2020 13:07
@MVrachev

MVrachev commented Aug 17, 2020

Copy link
Copy Markdown
CollaboratorAuthor

I rebased on the recent changes in the develop branch and the tests should no longer fail, but I don't know why tox -e py27 fails...
Can somebody from the maintainers restart Travis CI?

@jku

jku commented Aug 17, 2020•
edited
Loading

Copy link
Copy Markdown
Member

The TimeoutError was added by me, it was merged only today (#1096 ) ...

joshuagl
joshuagl approved these changes Aug 18, 2020
View reviewed changes

@joshuagljoshuagl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Lovely code cleanup, thanks @MVrachev !

@joshuagl
joshuagl merged commit be9944b into theupdateframework:developAug 18, 2020
@MVrachev
MVrachev deleted the optimize-length-hashes branch August 22, 2020 18:13
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joshuagljoshuagljoshuagl approved these changes

Assignees

No one assigned

Labels

securesystemslibRequires corresponding implementation in securesystemslib

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@MVrachev@joshuagl@jku@trishankatdatadog