WIP: Split/regroup client components

[sechkova]

Addresses: #1307, #1308

Description of the changes being introduced by the pull request:

Updater is split in three classes (names are tentative):

• Updater: public API
• MetadataUpdater
• TargetsUpdater

Regrouping of client-related modules:

• moved components related to file download inside the client_rework directory
• applied black and isort on the modules with the old code style
• download.py is deleted and its functionality is merged with mirrors.py which is now mirrors_download.py
• code is wrapped in Mirrors class

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

[sechkova]

@jku @MVrachev Can I get a quick feedback on these changes before continuing with fixing the linter issues (many "line-too-long" since black seems to skip text in comments)?
At this point the split into so-called MetadataUpdater and TargetsUpdater doesn't do much, let me know what do you think.

[jku]

Personally I think this validates the idea of separating (at least some of these) things:

• Mirrors seems like a good idea (not sure about the name and there's a lot in the download implementation that could be improved/simplified now that we do this -- stuff that we avoided improving in the Fetcher work to keep changes minimal)
• separating out TargetUpdater makes it clear how its functionality is unrelated to almost everything else (it only needs the Mirrors object) -- at least for me that was not as clear before this. Whether this is a good place for a Mixin or a separate object is up for debate but this seems like a reasonable direction to me
• MetadataUpdater does not quite look like what I expected (also the patch format really makes it hard to figure out)... I'll have to checkout locally to see how it looks, maybe I'll also try something myself: I'll comment later on this
• if anyone is worried about the large lines of code change: that's because some code got moved from tu/ to tuf/client/ but was not removed from tuf/ to keep the existing client working -- I think the move is largely correct (client code should be in tuf/client/) but does present some questions:
  • since we decided to work in a branch should we just replace the tuf/client/ with this code? Then we don't need to leave old code lying around
  • should the implementation details be actually moved to tuf/client/_internal/ ? I'd like it if we tried harder to show what is public API and what is not

As a general comment: I think splitting the PR might be useful: Mirrors, MetadataUpdater and TargetUpdater are all kind of separate ideas that we should be able to discuss separately. Also I think there's a lot in the code we could improve in each case -- if we start doing it in this PR this becomes a very long discussion again and I think we should rather aim for smaller faster merges. I get that the changes can be a bit tricky to work on at the same time... but it might be worth it

I've got more notes written about this: I can write them down once you decide how you want to proceed (don't want to fill this PR with details unless you want to handle them here).

[MVrachev]

Personally I think this validates the idea of separating (at least some of these) things:

• Mirrors seems like a good idea (not sure about the name and there's a lot in the download implementation that could be improved/simplified now that we do this -- stuff that we avoided improving in the Fetcher work to keep changes minimal)

Agree that we should think about the naming.

• separating out TargetUpdater makes it clear how its functionality is unrelated to almost everything else (it only needs the Mirrors object) -- at least for me that was not as clear before this. Whether this is a good place for a Mixin or a separate object is up for debate but this seems like a reasonable direction to me

Agree, this change made was good.

• MetadataUpdater does not quite look like what I expected (also the patch format really makes it hard to figure out)... I'll have to checkout locally to see how it looks, maybe I'll also try something myself: I'll comment later on this

• if anyone is worried about the large lines of code change: that's because some code got moved from tu/ to tuf/client/ but was not removed from tuf/ to keep the existing client working -- I think the move is largely correct (client code should be in tuf/client/) but does present some questions:

  • since we decided to work in a branch should we just replace the tuf/client/ with this code? Then we don't need to leave old code lying around
  • should the implementation details be actually moved to tuf/client/_internal/ ? I'd like it if we tried harder to show what is public API and what is not

I am also thinking about that.
Besides moving the internal code into an _internal folder I noticed through the code many places where private functions are not
marked as private.

As a general comment: I think splitting the PR might be useful: Mirrors, MetadataUpdater and TargetUpdater are all kind of separate ideas that we should be able to discuss separately. Also I think there's a lot in the code we could improve in each case -- if we start doing it in this PR this becomes a very long discussion again and I think we should rather aim for smaller faster merges. I get that the changes can be a bit tricky to work on at the same time... but it might be worth it

I was going to write the same.
I think the addition ofMetadataUpdater and TargetUpdater and the removal of Updater should be in the same pr.
Those changes are logically connected and we would be able to discuss what exactly to put into each of the classes.

The creation of the Mirrors class, the move of the download modules inside the client directory and the merge of mirrors.py and download.py into one could be another pr.

[sechkova]

I opened a separate PR for mirrors/download redesign (#1331) and I will follow up with another one related to splitting the Updater class. After that I plan to close this one.

  * since we decided to work in a branch should we just replace the tuf/client/ with this code? Then we don't need to leave old code lying around

I keep the old duplicated code because we still plan to merge the experimental-client branch in the main one and keep two implementations (TUF 1.0.0 and TUF 0.x.x) in parallel. Maybe we should revise this decision or maybe deleting some modules here won't conflict with a future merge?

  * should the implementation details be actually moved to tuf/client/_internal/ ? I'd like it if we tried harder to show what is public API and what is not

I agree but can we do that at the end when we have defined what is what?

[jku]

Sounds great thank you.

I keep the old duplicated code because we still plan to merge the experimental-client branch in the main one and keep two implementations (TUF 1.0.0 and TUF 0.x.x) in parallel. Maybe we should revise this decision or maybe deleting some modules here won't conflict with a future merge?

Maybe -- The balancing act is this: I don't want us to maintain two client implementations at the same time if we can avoid it, yet I don't want to delay merging this to develop any longer than necessary. I'd say let's keep doing what you're doing for now now but keep the option open and re-evaluate after a few weeks: maybe at some point it becomes clear that the refactored client is going to be clearly better and the choice is then easy.

  * should the implementation details be actually moved to tuf/client/_internal/ ? I'd like it if we tried harder to show what is public API and what is not

I agree but can we do that at the end when we have defined what is what?

Sure!

[sechkova]

Thanks for the feedback. The same commits from this PR are split in two separate PRs:

• Mirrors: WIP: Mirrors redesign #1331
• MetadataUpdater and TargetsUpdater WIP: Split Updater  #1338

Let's continue the separate discussions there. I am closing this one.