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

Implement Alternate Repository Location for PEP 708 #15716

Merged

Conversation

cofiem
Copy link
Contributor

@cofiem cofiem commented Apr 3, 2024

Adds per-project Alternate Repository Location as per PEP 708.

  • Implements the UI and database changes.
  • Includes changes to the html and json APIs.
  • Includes tests.
  • The alternate repository location information is on the project settings page.
  • There is a form to add, and a list to show and delete existing entries.

Notes:

  • The 'tracks' metadata does not make sense for PyPI, as PyPI does not extend other repositories.

Relates to #15431

Previews:

HTML: curl http://localhost/simple/[project_name]/

<!DOCTYPE html>
<html>
  <head>
    <meta name="pypi:repository-version" content="1.1">
    <meta name="pypi:alternate-locations" content="https://example.org">
    <title>Links for [project_name]</title>
  </head>
...

JSON: curl -H "Accept: application/vnd.pypi.simple.v1+json" http://localhost/simple/[project_name]/

{"alternate-locations":["https://example.org"], ... }

image

@cofiem cofiem marked this pull request as ready for review June 30, 2024 05:32
@cofiem cofiem requested a review from a team as a code owner June 30, 2024 05:32
@ewdurbin
Copy link
Member

ewdurbin commented Jul 9, 2024

Hi @cofiem and thank you for your contribution! I'm going to ask @dstufft to take a look as PEP author.

Copy link
Member

@ewdurbin ewdurbin left a comment

Choose a reason for hiding this comment

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

This is a really strong and comprehensive start, thank you! I left some notes after an initial review.

warehouse/packaging/models.py Show resolved Hide resolved
warehouse/templates/api/simple/detail.html Outdated Show resolved Hide resolved
warehouse/api/simple.py Show resolved Hide resolved
@cofiem
Copy link
Contributor Author

cofiem commented Jul 10, 2024

Thanks very much for the initial review, @ewdurbin! I'll have a look at the cache keys for the model.

@dstufft
Copy link
Member

dstufft commented Aug 22, 2024

Thanks for picking this up by the way! Really jazzed to see it getting worked on.

@ewdurbin
Copy link
Member

Thanks for reviewing @dstufft! @cofiem, I believe the comments from Donald should address the outstanding open questions.

Once this is updated I think it is ready for final review and merge!

…ository-location

# Conflicts:
#	tests/unit/api/test_simple.py
#	warehouse/locale/messages.pot
- route_path -> route_url to get full URL rather than path
- move self reference to the _simple_detail helper
@ewdurbin
Copy link
Member

@cofiem thank you! I pushed a commit to fix tests and include the self-reference in JSON API as well.

@dstufft I think this is ready to go, but I am curious if we should always send the self-reference in alternate-repositories value... or only when an alternate has been specified.

@ewdurbin ewdurbin requested a review from dstufft August 23, 2024 12:15
@cofiem
Copy link
Contributor Author

cofiem commented Aug 26, 2024

@ewdurbin That commit to fix tests and make the JSON & HTML apis match makes sense!

@ewdurbin
Copy link
Member

I'm going to hold off for a couple days, but if I don't hear from @dstufft on the two remaining open questions, I will remove the self-reference in alternate-repositories values and merge. The PEP reads as though they can be implied "When using alternate locations, clients MUST implicitly assume that the url the response was fetched from was included in the list."

The PEP reads as though they can be implied "When using alternate locations, clients MUST implicitly assume that the url the response was fetched from was included in the list."
ewdurbin added a commit to ewdurbin/peps that referenced this pull request Sep 19, 2024
PyPI's existing meta elements follow a `pypi:` naming convention. Implementation in pypi/warehouse#15716 will follow this convention.
@ewdurbin
Copy link
Member

Added a small callout block that we should remove when PEP 708 provisional acceptance becomes acceptance acceptance.
Screenshot 2024-09-19 at 9 46 34 AM

@ewdurbin ewdurbin merged commit d3ed6e0 into pypi:main Sep 19, 2024
18 checks passed
@cofiem
Copy link
Contributor Author

cofiem commented Sep 20, 2024

Excellent, thanks @ewdurbin!

Comment on lines +464 to +470
def _update_context(context, content_type, renderer_override):
if renderer_override != "json" or content_type in [
simple.MIME_TEXT_HTML,
simple.MIME_PYPI_SIMPLE_V1_HTML,
]:
return _valid_simple_detail_context(context)
return context
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but I think this helper is currently causing the test_with_files_quarantined_omitted_from_index test to be skipped (since it's an instance method, but it's currently indented into the helper.) I'll send a PR to fix it.

Copy link
Member

Choose a reason for hiding this comment

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

Opened #16777 with the fix.

woodruffw added a commit to trail-of-forks/warehouse that referenced this pull request Sep 23, 2024
di pushed a commit that referenced this pull request Sep 23, 2024
finswimmer added a commit to finswimmer/poetry that referenced this pull request Sep 23, 2024
PyPI implemented alternate location for PEP 708. (pypi/warehouse#15716)
finswimmer added a commit to python-poetry/poetry that referenced this pull request Sep 23, 2024
PyPI implemented alternate location for PEP 708. (pypi/warehouse#15716)
@di di mentioned this pull request Sep 26, 2024
description=form.description.data,
)
self.request.db.add(alt_repo)
self.request.db.add(
Copy link
Member

Choose a reason for hiding this comment

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

Ope, totally missed this in review, but this is pretty incorrect and the cause of #12933 (comment)

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.

4 participants