Refactor update_snapshot & _run_online_roles_bump #432
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MVrachev
force-pushed
the
small-refactoring
branch
from
306da54 to
349f841
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #432 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 1001 1015 +14
=========================================
+ Hits 1001 1015 +14 ☔ View full report in Codecov by Sentry. |
|
I am planning to use the changes of this pr when I prepare the worker changes required for: repository-service-tuf/repository-service-tuf-api#399 |
|
@kairoaraujo, the pr is ready for review. |
MVrachev
force-pushed
the
small-refactoring
branch
from
16a250b to
e0be720
Compare
The goal of the "update_snapshot" refactoring is to: 1. make a better separation between the two usages of "update_snapshot" - the first is to update snapshot and all roles that were given with the "targets_roles" argument - the second is to update snapshot and ALL bin target roles In the end, I wanted to be able to call "update_snapshot" with "bump_all" without having to pass the "targets_roles" argument. 2. fix a bug where when calling "update_snapshot" the resulting new snapshot meta didn't contain updated information for targets.json This resulted in a bug that when calling "_run_online_roles_bump", which updated targets.json, we saved the new targets.json, but the new snapshot.json didn't reference it. The second part of this refactoring is about "_run_online_roles_bump". Again there were two goals of this refactoring: 1. make sure we call "update_snapshot" correctly. This means when we force an update of all target bins we shouldn't also pass the "targets_roles" list. 2. make it clearer how we use the "force" argument: - when "force = True" we force a bump of all bins and targets.json - when "force = False" we bump only those bins and targets.json which have expired. Finally, I did some small tweaks on our tests with the idea to improve them our asserts and checks. Signed-off-by: Martin Vrachev <[email protected]>
In this specific "if" check we don't need to check the value of the "force" argument as that case is already handled from a previous check where if "force" is True we call update_snapshot with "bump_all". Signed-off-by: Martin Vrachev <[email protected]>
Currently, if the check if snapshot expiration is below a certain timedelta and "force" are True we wouldn't taken into account the value of force. With this fix, this is changed. Signed-off-by: Martin Vrachev <[email protected]>
Signed-off-by: Martin Vrachev <[email protected]>
We decided that it's best to not handle a StorageError exception and instead allow the test to fail. Fast API will fail if there is an exception from the worker and given that the StorageError exception has enough details there is no need for us to handle it. Additionally, this allowed us to remove the boolean return value from _run_online_roles_bump and bump_snapshot. Signed-off-by: Martin Vrachev <[email protected]>
MVrachev
force-pushed
the
small-refactoring
branch
from
e0be720 to
8aede98
Compare
|
This time for real @kairoaraujo, the pr is ready for review. |
kairoaraujo
approved these changes
Jan 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The goal of the "update_snapshot" refactoring is to:
The second part of this refactoring is about "_run_online_roles_bump". Again there were two goals of this refactoring:
Finally, I did some small tweaks on our tests with the idea to improve our asserts and checks.
Besides unit tests, it was tested locally with a worker instance and triggered the automatic online roles bump.
Signed-off-by: Martin Vrachev [email protected]