Skip to content
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

Allow the pip.conf to override the default index source. #5297

Merged
merged 7 commits into from
Feb 18, 2023

Conversation

matteius
Copy link
Member

No description provided.

@matteius matteius requested a review from oz123 January 26, 2023 04:16
Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For those who didn't follow the discussion, I suggest we add a news snippet and explicit documentation.

@kalebmckale
Copy link
Contributor

kalebmckale commented Jan 28, 2023

NOTE: Original reply quoted below and removed.

I've played around with it a bit. What do you think of this?

# Load Pip configuration and get items
self.configuration = Configuration(isolated=False, load_only=None)
self.configuration.load()
configuration_items = self.configuration.items()
# Get unique set of index-url values from Pip configuration
index_urls = set(
    value
    for section_key, value in configuration_items
    if "index-url" in section_key.split(".", 1)
)
if len(index_urls) == 1:
    pip_configured_index = index_urls.pop()
else:
    # NOTE: Either no index-url or ambiguous result 
    pip_configured_index = None
# Get unique set of trusted-host values from Pip configuration
trusted_hosts = set(
    chain.from_iterable(
        value.split(" ")
        for section_key, value in configuration_items
        if "trusted-host" in section_key.split(".", 1)
    )
)
if pip_configured_index:
    self.default_source = {
        "url": pip_configured_index,
        "verify_ssl": any(
            trusted_host in pip_configured_index
            for trusted_host in trusted_hosts
        ) or "http://" in pip_configured_index,
        "name": "pip_conf_index",
    }
elif self.s.PIPENV_TEST_INDEX:
    self.default_source = {
        "url": self.s.PIPENV_TEST_INDEX,
        "verify_ssl": True,
        "name": "custom",
    }
else:
    self.default_source = {
        "url": "https://pypi.org/simple",
        "verify_ssl": True,
        "name": "pypi",
    }

It requires this additional import line:

from itertools import chain

@matteius I got a chance to download your branch and test it in my work environment. It definitely worked as expected, which is exciting, and I have a couple notes.

When getting index-url, I suggest checking not only global.index-url but also install.index-url. Previously, I only had index-url defined under [install], and I expect a number of other users might also.

Instead of assigning verify-ssl=True, I suggest also pulling the configuration value trusted-host (probably should check both [global] and [install] here, too). Since this value can have multiple hosts, I was thinking something like this:

try:
    pip_configured_trusted_host = configuration.get_value(
        "install.trusted-host"
    )
except ConfigurationError:
    pip_configured_trusted_host = None
else:
    for trusted_host in pip_configured_trusted_host.split(" "):
        # XXX: might also be worth checking if URL is http://
        if trusted_host in pip_configured_index:
            verify_ssl = False
            break
    else:
        verify_ssl = True

@kalebmckale
Copy link
Contributor

@matteius Just checking in. Any updates?

@matteius
Copy link
Member Author

matteius commented Feb 5, 2023

@kalebmckale I think your changes make sense; I spent some time trying to consider if we could handle the case where there is more than one index configured, and also if you want to be a committer you could branch off this and make a PR pointing at this one. I still have some documentation to do for it as well. We wanted this past release to be just bug fixes, the plan is to include this in the upcoming release that also includes pip 23.0.

@matteius
Copy link
Member Author

@kalebmckale I ended up refactoring your code and took a stab at supporting multiple sources defined, also I think there was a logical error in how verify_ssl was being set with respect to trusted-hosts and https -- either that or it doesn't do what I think it does. Either way, could you take this branch for a test spin again?

@kalebmckale
Copy link
Contributor

Will do, @matteius! Thanks for making the changes. I have been trying to set up a test environment at home so I can confidently contribute. I will test this at work and let you know.

@kalebmckale
Copy link
Contributor

kalebmckale commented Feb 11, 2023

Ahhh… are you talking about the for…else loop?

It’s a Python thing. If the loop completes without hitting break, it runs the else statement.

If trusted-host is defined, it can be one or more domains space-separated. So, looping over these domains, check if each is contained (substring) in the pip_configured_index. If so, set verify_ssl=False since it’s trusted. If none of the trusted-host domains are a substring of the pip_configured_index, then verify_ssl=True (this is the else branch of the for loop).

UPDATE: Looks like I was commenting on the previous version and not talking about the same thing.

You were talking about this:

if pip_configured_index:
    self.default_source = {
        "url": pip_configured_index,
        "verify_ssl": any(
            trusted_host in pip_configured_index
            for trusted_host in trusted_hosts
        ) or "http://" in pip_configured_index,
        "name": "pip_conf_index",
    }

Specifically,

any(trusted_host in pip_configured_index for trusted_host in trusted_hosts ) or "http://" in pip_configured_index

First, it probably would have been better for me to use

urllib.parse.urlsplit(pip_configured_index).scheme == "http"`

than just do the string comparison. Putting that aside, I did provide the negation of my intended logic. At home, I run the code in my head while typing on my iPad, so I tend to make mistakes, it seems. 😋

Thanks for catching that and applying DeMorgan’s Law!

@matteius
Copy link
Member Author

@kalebmckale Any other feedback on this -- I want to get the documentation fixed up and ideally a release this weekend ...

Copy link
Contributor

@oz123 oz123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@oz123 oz123 merged commit 2b0dae3 into main Feb 18, 2023
@oz123 oz123 deleted the pip_conf_index-url branch February 18, 2023 14:00
@kalebmckale
Copy link
Contributor

Hey @matteius! Thanks for patience! Yes, the feature worked as expected with the pip.conf-specified index-url and evaluating the trusted-host correctly from there, whether listed under global section or under the install section.

The thing still pending, which is covered under another issue about the specifying --index option, which currently duplicates in the Pipfile for each package that follows and also doesn't use the trusted-host information from either the pip.conf file or the environment variable PIP_TRUSTED_HOST.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants