-
Notifications
You must be signed in to change notification settings - Fork 272
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
client refactor: remove mirrors #1368
client refactor: remove mirrors #1368
Conversation
Keep the current API and mirrors configuration but use only the first mirror from the list for metadata download. Target files download remains unchanged. Signed-off-by: Teodora Sechkova <[email protected]>
Updater now uses only a single url for metadata download. Target files download use either a default url or an optional one for each file passed by the caller. Signed-off-by: Teodora Sechkova <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies for quick review, but I wanted to share some quick impressions. This is a pleasing diff stat. I think we might be able to reduce it further: the _build_full_url()
/ try:
, download()
, verify()
loop is repeated a few times. Might it generalise to a helper function?
metadata_url: str, | ||
default_target_url: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these are prefixes or bases, not full urls – should we call them metadata_base_url
and default_target_base_url
?
Or perhaps default_base_url
and targets_base_url
?
# Should we accept metadata url as a default for targets or | ||
# targets_url should be provided either in this constructor | ||
# or as a download_target parameter? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to default to using the same host/url combination for all downloads, unless told otherwise via the targets_base_url
or the optional function parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it works though: metadata base url directory should contain metadata (it's like current url_prefix+metadata_path
). I would be surprised if someone wants to store targets in the same directory. I think it's reasonable to expect either a default target url prefix or the individual target download url prefixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Jussi, I agree with you, but I think Joshua simply means the following: there is a metadata_base_url
parameter that must always be explicitly set. However, there is also a targets_base_url
that defaults to metadata_base_url
unless also explicitly set to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understood: I'm just wondering if anyone would want it to default to metadata_base_url?
if default_target_url is None: | ||
self._default_target_url = metadata_url | ||
else: | ||
self._default_target_url = default_target_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd probably write this as
if default_target_url is None: | |
self._default_target_url = metadata_url | |
else: | |
self._default_target_url = default_target_url | |
self._default_target_url = metadata_url | |
if default_target_url is not None: | |
self._default_target_url = default_target_url |
self, | ||
targetinfo: Dict, | ||
destination_directory: str, | ||
target_url: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: target_url_base
?
full_url, targetinfo["fileinfo"]["length"], self._fetcher | ||
) | ||
_check_file_length(temp_obj, targetinfo["fileinfo"]["length"]) | ||
temp_obj.seek(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder whether _check_file_length()
should do the seek(0)
, what do you think?
temp_obj.close() | ||
# TODO: do we reraise a NoWorkingMirrorError or just | ||
# let exceptions propagate? | ||
raise exceptions.NoWorkingMirrorError({full_url: e}) from e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(here and below) does NoWorkingMirrorError
still make sense, given we're removing mirrors?
# Are these steps enough? Or too much? Is this the right place? | ||
filepath = parse.quote(filepath) | ||
# Assuming that base_url ends with a '/' character, otherwise parse.urljoin | ||
# omits (correcly) the last part of the base URL path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# omits (correcly) the last part of the base URL path | |
# omits (correctly) the last part of the base URL path |
Absolutely: e.g. we can now get rid of almost all manual file object handling |
Since Teodora is away for the week I'll take this and continue with suggested fixes (and removing loads of temp file handling). I think I'll do a new PR and close this (but will keep these commits so review can "continue") |
superseded by #1373 |
Addresses #1307
Description of the changes being introduced by the pull request:
Drop "mirrors" support.
Updater no longer supports downloading each metadata file from a list of user-provided mirrors. All downloads are performed from a single metadata url.
Target files are downloaded either from a single default url or an optional url is passed for each target file on download.
More info in: comment, comment
API is changed the following way:
Edit: The exact
metdata_url
andtarget_url
format needs either better documentation for the user or better normalization functions provided by TUF.Please verify and check that the pull request fulfills the following
requirements: