Keyring multi choice #11698
Keyring multi choice #11698
Conversation
Darsstar
force-pushed
the
keyring-multi-choice
branch
3 times, most recently
from
12c5206 to
527e43e
Compare
Darsstar
force-pushed
the
keyring-multi-choice
branch
3 times, most recently
from
412a5d9 to
230b588
Compare
|
I resolved all your comments in the refactoring I did. |
Darsstar
force-pushed
the
keyring-multi-choice
branch
from
9153a61 to
ef02ef2
Compare
github-actions
bot
added
the
needs rebase or merge
…import` or `subprocess` Refactored `_get_index_url()` to get integration tests for the subprocess backend working. Keyring support via the 'subprocess' provider can only retrieve a password, not a username-password combo. The username therefor MUST come from the URL. If the URL obtained from the index does not contain a username then the username from a matching index is used. `_get_index_url()` does that matching. The problem this refactoring solves is that the URL where a wheel or sdist can be downloaded from does not always start with the index url. Azure DevOps Artifacts Feeds are an example since it replaces the friendly name of the Feed with the GUID of the Feed. Causing `url.startswith(prefix)` to evaluate as `False`. The new behaviour is to return the index which matches the netloc and has the longest common prefix of the `path` property of the value returned by `urllib.parse.urlsplit()`. The behaviour for resolving ties is unspecified.
Darsstar
force-pushed
the
keyring-multi-choice
branch
from
ef02ef2 to
16bd6b7
Compare
|
I'm neutral. This seems like a reasonable solution, but I still have fundamental reservations about how keyring is integrated with pip, and this feels like it's a band aid over that fundamental problem. The original approach was to use keyring (via import) if it was installed. That has issues with the question of how you install keyring. At some point we added the option to use keyring via command line. Now we're making keyring opt-in, adding yet more options, and generally making the whole thing ever more complex. I frankly don't think this is the right way to evolve things here. Keyring support is a relatively niche requirement, aimed mostly at large organisations who have private, password-protected indexes. And yet we're spending increasing amounts of volunteer time and adding complexity to pip's code base for it. The same is true of a lot of pip's network stack. Realistically, we do little more than fetch URLs, and having all of the various options to control that process (including keyring, but going way beyond that with proxy settings, trusted URLs, timeouts and retries, etc) is overhead on top of our core job. What I'd love is for us to outsource the whole "get a URL" job onto a library dedicated to making that just happen - ideally, integrating with the standard platform settings. So we don't have to. But that's a much bigger topic, and as far as I know no such library exists, so I'll raise a separate issue for that one 🙂 TBH, I doubt we're going to get a better answer than this PR in the short term. And I don't want to block this just because we might be able to do better "someday". But I do think we need to keep aware of the long term implications. |
|
FWIW, as the RM for 23.0, I'm gonna say that this isn't going to be in this release. We can include it in the next one. |
github-actions
bot
added
the
needs rebase or merge
# Conflicts: # src/pip/_internal/network/auth.py
Darsstar
force-pushed
the
keyring-multi-choice
branch
from
9a0d319 to
6affad8
Compare
|
I’m going to merge this for 23.1 and see what happens. |
Fixes #8719.
Fixes #11020
See #8719 (comment) for the reasoning why this implementation, or a minor variant, is desired over a simpler on/off switch as implemented in #11215.
It is currently based on #11029, which is, as of writing this, a part of the 23.0 milestone since it would touch the same unit tests. Making a rebase inevitable either way.
Let me know if I should make it more standalone.