WIP: Mirrors redesign #1331
WIP: Mirrors redesign #1331
Conversation
sechkova
force-pushed
the
mirrors_redesign
branch
from
6682659
to
f534b6e
Compare
sechkova
added
the
experimental-client
There was a problem hiding this comment.
The only questionable choice I see is the one with the naming of the class Mirrors.
Of the 6 methods in the Mirrors class, 4 of them have the word download in them.
I think this is a good sign that the class name is better to be just Downloader.
Yes, for download were using mirrors, but that seems like an API detail giving
that we don't have a non-mirrors downloader class.
One would say that Fetcher and Downloader are close names but calling the class
Mirrors just hides that proximity.
Other than that, I have some small comments, but nothing too serious.
tuf/client_rework/mirrors.py
Outdated
| @@ -22,174 +22,162 @@ | |||
| of the file with respect to the base url. | |||
| """ | |||
|
|
|||
|
|
|||
| # Help with Python 3 compatibility, where the print statement is a function, an | |||
| # implicit relative import is invalid, and the '/' operator performs true | |||
| # division. Example: print 'hello world' raises a 'SyntaxError' exception. | |||
There was a problem hiding this comment.
Do we want to copy those comments and the future imports too?
I know we are going to remove them when resolving this issue #1297,
but do we want to add them in new files as well?
tuf/client_rework/mirrors.py
Outdated
| tuf.formats.MIRRORDICT_SCHEMA.check_match(mirrors_dict) | ||
| self._config = mirrors_dict | ||
|
|
||
| def _get_list_of_mirrors(self, file_type, file_path): |
There was a problem hiding this comment.
Do you plan to add type annotations to the old functions as well which were moved or only on the newly added ones?
tuf/client_rework/mirrors.py
Outdated
|
|
||
| import securesystemslib | ||
| import six | ||
|
|
||
| import tuf | ||
| import tuf.client_rework.download as download | ||
| import tuf.formats | ||
| from tuf.requests_fetcher import RequestsFetcher |
There was a problem hiding this comment.
I think you should import the FetcherInterface as well, because later on in the Mirrors.__init__ you use the "FetcherInterface" as a type annotation.
Locally I see warnings because of this.
| except Exception as exception: | ||
| # pylint cannot figure out that we store the exceptions | ||
| # in a dictionary to raise them later so we disable | ||
| # the warning. This should be reviewed in the future still. |
There was a problem hiding this comment.
Maybe add a TODO if you expect us to review this in the future?
The modules performing network download are used only by the client side of TUF. Move them inside the client directory for the refactored client. Move the _mirror_*download functions from Updater to mirrors.py. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
The two functions safe/unsafe_download differ only by setting a single boolean flag. Remove them and call directly _download_file instead. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Wrap the functionality from the mirrors.py module inside a Mirrors class. Apply black and isort over the old-style mirrors code. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Make fetcher a member of Mirrors class. Move the instantiation of the default fetcher object to the Mirrors constructor. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Move the functionality from the download module inside Mirrors class. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Apply manually black and isort to fetcher.py and request_fetcher.py Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Black does not automatically wrap lines in comments sections at 80 characters. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Replaced the log messages with f-strings. Formatted the messages outside the log function to avoid logging-fstring-interpolation warnings from pylint. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Fixing various pylint issues after merging mirrors and download and applying the new pylint config. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
Fix no-else-raise errors after running the new pylint configuration on the merged mirrors and download code. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
pylint cannot figure out that we store the exceptions in a dictionary to raise them later so we disable the warning. This should be reviewed in the future still. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
sechkova
force-pushed
the
mirrors_redesign
branch
from
f534b6e
to
deee5b2
Compare
Fix imports to be vendoring compatible. Signed-off-by: Teodora Sechkova <tsechkova@vmware.com>
sechkova
force-pushed
the
mirrors_redesign
branch
from
1b92b79
to
32011a7
Compare
Addresses: #1307
Description of the changes being introduced by the pull request:
Regrouping of client-related modules:
client_reworkdirectorydownload.pyis deleted and its functionality is merged withmirrors.pywhich is nowmirrors_download.pyMirrorsclassPlease verify and check that the pull request fulfills the following
requirements: