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

utils: ensure Downloader defaults to Authenticator #9202

Merged
merged 1 commit into from
Mar 23, 2024

Conversation

abn
Copy link
Member

@abn abn commented Mar 22, 2024

This change allows correct usage of http credentials when downloading files.

Resolves: #1444

poetry config repositories.gh-release-your-org-project https://github.com/your-org/project/releases/download/
poetry config http-basic.gh-release-your-org-project username password
poetry add https://github.com/your-org/project//releases/download/1.1.2/project-1.1.2-py3-none-any.whl

Testing #1444

Modify commands to your specific case.

Using pipx

pipx install --suffix=@9202 'poetry @ git+https://github.com/python-poetry/poetry.git@refs/pull/9202/head'

Using a container (podman | docker)

podman run --rm -i --entrypoint bash docker.io/python:3.12 <<EOF
set -xe
python -m pip install --disable-pip-version-check -q git+https://github.com/python-poetry/poetry.git@refs/pull/9202/head

poetry config repositories.gh-release-your-org-project https://github.com/your-org/project/releases/download/
poetry config http-basic.gh-release-your-org-project username password

poetry new foobar
pushd foobar
poetry add [pycowsay](https://github.com/your-org/project//releases/download/1.1.2/project-1.1.2-py3-none-any.whl)
EOF

@abn abn requested a review from a team March 22, 2024 21:21
@abn abn force-pushed the util/downloader-default-session branch from bfe7be6 to 6cb7d0e Compare March 22, 2024 21:32
@abn abn marked this pull request as ready for review March 22, 2024 21:33
@abn abn force-pushed the util/downloader-default-session branch from 6cb7d0e to e3f0072 Compare March 22, 2024 23:10
@radoering
Copy link
Member

Actually, (without trying it) I suppose #1444 has already been solved some time ago (or this change does not solve it) because it looks like we always pass a session to Downloader so this might be a change only affecting tests. I wonder if we should just disallow to pass no session because we don't want to use it in practice or if we want to keep it just to simplify tests?

@radoering
Copy link
Member

Argh, I missed one dynamic usage that does not pass a session:

https://github.com/python-poetry/poetry/blob/main/src/poetry/packages/direct_origin.py#L80

However, we could think about changing that.

@abn
Copy link
Member Author

abn commented Mar 23, 2024

We do not always pass in a session.

link, strict=True, download_func=download_file

This is what caused #1444 afaict.

@abn
Copy link
Member Author

abn commented Mar 23, 2024

I don't think changing it is trivial. And we want to keep allowing the utility function to optionally pass in a session.

@radoering
Copy link
Member

I don't think changing it is trivial.

What about creating an Authenticator in DirectOrigin.__init__ and then

        download_func = functools.partial(download_file, session=self._authenticator)
        artifact = self._artifact_cache.get_cached_archive_for_link(
            link, strict=True, download_func=download_func
        )

And we want to keep allowing the utility function to optionally pass in a session.

We probably should avoid using it without passing a session for performance reasons but if you think it makes sense to keep it for convenience, I'm ok with it.

@abn abn force-pushed the util/downloader-default-session branch from e3f0072 to 3164958 Compare March 23, 2024 12:01
@abn
Copy link
Member Author

abn commented Mar 23, 2024

What about creating an Authenticator in DirectOrigin.init

Not opposed to that. But, I do wonder if that actually gains us much since the cache already will be set after the first download and the url never changes.

Reckon we should anyway?

We probably should avoid using it without passing a session for performance reasons but if you think it makes sense to keep it for convenience, I'm ok with it.

I am not so sure there is a discernable performance hit for one-shot downloads. But yes, I do think keeping it for convenience is worthwhile.

@radoering
Copy link
Member

But, I do wonder if that actually gains us much since the cache already will be set after the first download and the url never changes.

One DirectOrigin instance is used for all URL downloads during resolving so it's not just one URL. It's probably not that much of a gain but it shouldn't hurt since it will be similar to what Executor is doing.

@abn abn force-pushed the util/downloader-default-session branch from 3164958 to 8d79c4d Compare March 23, 2024 14:47
@abn
Copy link
Member Author

abn commented Mar 23, 2024

@radoering made trhe changes, used get_default_authenticator too.

@abn abn force-pushed the util/downloader-default-session branch from 8d79c4d to f68c0b9 Compare March 23, 2024 15:20
@radoering radoering force-pushed the util/downloader-default-session branch from f68c0b9 to 26efac1 Compare March 23, 2024 16:03
@radoering radoering enabled auto-merge (rebase) March 23, 2024 16:10
@radoering radoering merged commit 6180876 into python-poetry:main Mar 23, 2024
22 checks passed
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Installing *.whl from GitHub release url from private repo
2 participants