Skip to content

Conversation

@abn
Copy link
Owner

@abn abn commented Apr 29, 2022

No description provided.

abn added 2 commits April 29, 2022 15:00
This change refactors HTTP repository source implementations. The
following changes have been made.

- CacheControl cache now lives within Authenticator.
- Authenticator manages unique sessions for individual netloc.
- CacheControl usage now respects disable cache parameter in repos.
- Certificate and authentication logic is now managed solely within
  Authenticator for source repositories taking advantage of recent
  enhancements.

These changes should allow for better handling of cases like those
described in python-poetry#3041. Additionally, this forms the foundation for
unifying HTTP specific logic within the code base and possibly allowing
for migration of requests etc. if/when required.
@abn abn force-pushed the http-repo-cleanup-multi-path branch from ca4d840 to a8e3fc8 Compare April 29, 2022 22:30
# repositories setting is special for now
repositories = self._get_environment_repositories()
if repositories:
return repositories
Copy link
Owner Author

Choose a reason for hiding this comment

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

This should probably be merged with global config.

)
def _get_http_auth(
self, repository: AuthenticatorRepositoryConfig
) -> dict[str, str | None] | None:
Copy link

@Darsstar Darsstar Apr 30, 2022

Choose a reason for hiding this comment

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

We might want to lookup repository.url in self._credentials in this method. (And populate said cache entry at the end of this method if credentials were found)

Comment on lines +347 to +352
if len(candidates_netloc_only) > 1:
logger.debug(
"Multiple source configurations found for %s - %s",
parsed_url.netloc,
", ".join(map(lambda c: c.name, candidates_netloc_only)),
)
Copy link

@Darsstar Darsstar May 1, 2022

Choose a reason for hiding this comment

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

Suggested change
if len(candidates_netloc_only) > 1:
logger.debug(
"Multiple source configurations found for %s - %s",
parsed_url.netloc,
", ".join(map(lambda c: c.name, candidates_netloc_only)),
)
if len(candidates_netloc_only) > 1:
from os.path import commonprefix
candidates_netloc_only.sort(key=lambda c: len(commonprefix([parsed_url.path, c.path])), reverse=True)
logger.debug(
"Multiple source configurations found for %s - %s",
parsed_url.netloc,
", ".join(map(lambda c: c.name, candidates_netloc_only)),
)

Apparently Azure DevOps Artifact Feeds seems to start preferring the guid at some point instead of the Feed's display name.

https://pkgs.dev.azure.com/<ORG NAME>/_packaging/<FEED DISPLAY NAME>/pypi/simple/<PACKAGE>

ends up downloading

https://pkgs.dev.azure.com/<ORG NAME>/_packaging/<FEED GUID>/pypi/simple/<PACKAGE>/<VERSION>/<WHEEL>#sha256=<HASH>

resulting in the str.startswith() check to return False.

My suggestion sort on the longest common prefix for the path component of the url as a best effort.

(Azure DevOps Personal Access Tokens are scroped to an organization, not to individual projects/feeds in an organization, so this works)

@abn abn force-pushed the http-repo-cleanup branch from ea6e9c7 to f67c9e1 Compare May 5, 2022 19:42
@abn
Copy link
Owner Author

abn commented May 5, 2022

@Darsstar thanks for the review; I have applied your suggestions and added the changes to python-poetry#5518. Another pass there would be appreciated.

@abn abn closed this May 5, 2022
@abn abn deleted the http-repo-cleanup-multi-path branch May 5, 2022 19:43
@github-actions
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 Mar 24, 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.

3 participants