-
Notifications
You must be signed in to change notification settings - Fork 275
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
mirrors: Make targets_path and metadata_path optional #1153
mirrors: Make targets_path and metadata_path optional #1153
Conversation
Now clients can leave out targets_path or metadata_path if the client knows the mirror does not have that type of targets. This is backwards compatible: old mirror configs continue to work. Fixes theupdateframework#1079 Signed-off-by: Jussi Kukkonen <[email protected]>
In case it's relevant, these are the configs I keep currently switching in pip depending on what I'm downloading: config 1 (for downloading index file targets from pypi.org):
config 2 (for downloading distribution target files from pythonhosted.org):
|
Actually, it's possible that even a very expressive directory confinement might not be able to handle the warehouse setup in a single config... I'd have to really think the solution through (but maybe it's not needed for this PR) |
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.
Аside from my comment LGTM.
metadata_path = SCHEMA.Optional(RELPATH_SCHEMA), | ||
targets_path = SCHEMA.Optional(RELPATH_SCHEMA), |
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.
Prior to this change a check_match()
call will ensure that both metadata_path
and targets_path
are set. But following this change it's possible to pass an object conformant to MIRROR_SCHEMA
with neither value set.
I'm not sure whether we should worry too much about this, it should be fairly obvious to a client if they misconfigure by not setting either value?
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.
This is true, does not seem like a major issue to me either.
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.
LGTM, thanks!
Now clients can leave out targets_path or metadata_path if the
client knows the mirror does not have that type of targets.
This is backwards compatible: old mirror configs continue to work.
Fixes #1079.
Signed-off-by: Jussi Kukkonen [email protected]
PR notes:
This is a minor improvement in the warehouse case where one mirror has metadata and targets, and the other mirror has only targets.
Note that this leaves the issue of using os.path.join() on URLs as is: Fixing #1077 in a provably backwards compatible way seems tricky.
I believe this is a good if minor change... but I'll make the case against merging as well:
Unfortunately improving directory confinement is likely to be a breaking change.
Please verify and check that the pull request fulfills the following
requirements: