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

Lazy load credentials for legacy repositories #4937

Closed
wants to merge 2 commits into from
Closed

Lazy load credentials for legacy repositories #4937

wants to merge 2 commits into from

Conversation

axxelG
Copy link

@axxelG axxelG commented Dec 25, 2021

Move loading credentials from LegacyRepository.init() separate function and call it from _get_page() and authenticated_url()
This avoids loading credentials for commands that don't use them (e. g. poetry env list). This helps to avoid annoying users that have to unlock their keyring and if such commands are called as background tasks (CI pipelines, VS-Code environment detection).

Questions and remarks from my side:

  • Because of the current implementation I assumed that legacy repositories always have basic auth. If authentication is optional, I would need help to figure out possible cases and to adapt (parameterize) the tests. My pytest knowledge is very limited.
  • Are you appreciating the more verbose docstring/comment on authenticated_url()? I'm fine with removing it.
  • I was not sure if _set_authentication_creds() should be a private or public function. Tell me if you prefer it to become set_authentication_creds()

Resolves: #4874

  • [ x ] Added changed tests for changed code.
  • Updated documentation for changed code.

@axxelG
Copy link
Author

axxelG commented Dec 25, 2021

pre-commit.ci autofix

axxelG and others added 2 commits March 15, 2022 10:53
Move loading credentials from LegacyRepository.__init__() separate function and call it from _get_page() and authenticated_url()
This avoids loading credentials for commands that don't use them (e. g. poetry env list)
if not self._session.auth:
return self.url
"""
Set authentication credentials and return a URL with basic auth
Copy link
Member

Choose a reason for hiding this comment

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

This code path is only used for uploads not for requests etc. see Authenticator for details.

@abn
Copy link
Member

abn commented May 12, 2022

I suspect the spirit of this change has been implemented as a side-effect of #5518. Please comment if I am missing something.

@abn abn closed this May 12, 2022
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 Feb 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.

Only access keyring credentials on demand
2 participants