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

Sub-PR of 846: Modification of Updater and tests as part of #660 #868

Conversation

adityasaky
Copy link
Collaborator

Part of the #660 effort, a sub-PR of #846. This is part of step 3 of the plan outlined here: #846 (comment)

@awwad
Copy link
Contributor

awwad commented May 2, 2019

@adityasaky
Of the 14 test failures:

  • One or two were handled when I added a commit just now that made sure that tuf.SPECIFICATION_VERSION is set correctly at the start of each test, so that if the test that modifies it fails before it can switch it back to the original value, things don't break in other tests.
  • Some failures are related to Updater.targets_of_role and Updater.all_targets. I'll handle those functions (and their tests), either in this sub-PR or in 846 itself.
  • Many of the tests use repository_tool to do test set up. repository_tool fixes will not be included in this sub-PR. While we could alter these tests to use static metadata or otherwise avoid repository_tool use, this is probably more effort than it's worth, so I'll eyeball the underlying functionality to make sure nothing obvious is missing. Then, we can see what the test results are after the repository_tool sub-PR.
  • It looks like some of them are related to Repository.load_repository not loading all roles the way it now should. That we can leave for the next sub-PR, too. (test_5_targets_of_role, test_8_remove_obsolete_targets)

@awwad
Copy link
Contributor

awwad commented May 2, 2019

Now that roledb has been cleansed with fire, it looks more and more like updater shouldn't use roledb at all: it already has its own store of role metadata (self.metadata), and I don't think it's helping anyone to try to keep that synched. Some things check self.metadata, and some check roledb.... Further, Updater instances keep two versions of metadata: the currently trusted as well as the previously trusted, so roledb isn't adequate for its purposes anyway. (It could even cause weird issues if you're running a few different things in one process -- a client and repo tool, or two clients, say.) I have to think through that just a little bit more. It's probably still reasonable for updater to use keydb.... I'll figure this out in the parent PR after this is merged. I'm going to clean up some tests and updater code here, though.

It also looks like class Updater should swallow class MultiRepoUpdater, but that's definitely issue for another day.

@joshuagl
Copy link
Member

Due to imminent refactor efforts this code is unlikely to merge. Thanks for your efforts in the PR!

@joshuagl joshuagl closed this Sep 10, 2020
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