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.

Sign up for GitHub

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

Jump to bottom

Optimize the calculation of length and hashes #1097

Merged
merged 1 commit into from
Aug 18, 2020
Merged

Optimize the calculation of length and hashes #1097

merged 1 commit into from
Aug 18, 2020

Conversation

MVrachev
Copy link
Collaborator

@MVrachev MVrachev commented Aug 4, 2020

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
Copy link
Collaborator Author

MVrachev commented Aug 4, 2020

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.

@joshuagl joshuagl added the securesystemslib Requires corresponding implementation in securesystemslib label Aug 4, 2020
@joshuagl
Copy link
Member

joshuagl commented Aug 4, 2020 •
edited
Loading

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
Copy link
Member

jku commented Aug 4, 2020

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
Copy link
Member

@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
Copy link
Collaborator Author

MVrachev commented Aug 5, 2020

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
Copy link
Member

@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.

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.

tuf/repository_lib.py Outdated Show resolved Hide resolved
tuf/repository_lib.py Outdated Show resolved Hide resolved
@MVrachev MVrachev force-pushed the optimize-length-hashes branch from 105d8bc to b0dda36 Compare August 17, 2020 11:50
@MVrachev MVrachev marked this pull request as ready for review August 17, 2020 11:56
@MVrachev
Copy link
Collaborator Author

MVrachev commented Aug 17, 2020 •
edited
Loading

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.

@MVrachev
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 MVrachev force-pushed the optimize-length-hashes branch from b0dda36 to ba87454 Compare August 17, 2020 13:07
@MVrachev
Copy link
Collaborator Author

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
Copy link
Member

jku commented Aug 17, 2020 •
edited
Loading

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

joshuagl
joshuagl approved these changes Aug 18, 2020
View reviewed changes
Copy link
Member

@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.

Lovely code cleanup, thanks @MVrachev !

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

@joshuagl joshuagl joshuagl approved these changes

Assignees
No one assigned
Labels
securesystemslib Requires 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