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.

Sign up for GitHub

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

Jump to bottom

Implement Alternate Repository Location for PEP 708 #15716

Merged
merged 36 commits into from
Sep 19, 2024
Merged

Implement Alternate Repository Location for PEP 708 #15716

merged 36 commits into from
Sep 19, 2024

Conversation

cofiem
Copy link
Contributor

@cofiem cofiem commented Apr 3, 2024 •
edited
Loading

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 added 13 commits April 3, 2024 23:23
@cofiem
initial attempt at adding alternate repository location details
04e9ec1
@cofiem
Merge branch 'pypi:main' into feature/add-project-alternate-repositor…
837e464 
…y-location
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
401ed2b 
…ository-location

# Conflicts:
#	warehouse/packaging/models.py
@cofiem
implement per-project alternate locations metadata
6877707
@cofiem
starting to add tests
81cdf21
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
cf94d59 
…ository-location
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
6759b55 
…ository-location
@cofiem
starting to add tests
f21d788
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
8d53891 
…ository-location
@cofiem
added tests
cc9111b 
Fixed rendering for detail.html.
Moved api mimetypes to const vars.
Check delete confirmation name matches.
@cofiem
updated translations
3517006
@cofiem
satisfy test coverage
5323726
@cofiem
update translations
bfab8e3
@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.

ewdurbin
ewdurbin reviewed Jul 9, 2024
View reviewed changes
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.

@cofiem
Merge remote-tracking branch 'origin/feature/add-project-alternate-re…
ef9d6db 
…pository-location' into feature/add-project-alternate-repository-location

# Conflicts:
#	tests/unit/manage/test_views.py
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
987d129 
…ository-location

# Conflicts:
#	warehouse/locale/messages.pot
@cofiem
update translations
e6fdd04
@cofiem
register cache and purge keys for AlternateRepository objects
52542f9
@cofiem
change db migration down revision to most recent migration
e52f44e 
This allows the migrations to run.
@cofiem
update test after adding alternate repository cache and purge key
69c6362
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
95ee51a 
…ository-location
@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
60970a6 
…ository-location
@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!

@cofiem
Merge branch 'refs/heads/main' into feature/add-project-alternate-rep…
1849468 
…ository-location

# Conflicts:
#	tests/unit/api/test_simple.py
#	warehouse/locale/messages.pot
cofiem and others added 4 commits August 23, 2024 20:34
@cofiem
change db migration down revision to most recent migration
f7f9244 
This allows the migrations to run.
@cofiem
name is already normalized
1df1394
@cofiem
update translations
415b85f
@ewdurbin
match functionality between JSON and HTML simple API
64fa33b 
- 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."

@ewdurbin
Merge branch 'main' into feature/add-project-alternate-repository-loc…
3fffca8 
…ation
@ewdurbin
remove self-reference from Simple HTML and JSON
ebe29e8 
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
@ewdurbin
PEP 708: update reference to match implementation
303a04f 
PyPI's existing meta elements follow a `pypi:` naming convention. Implementation in pypi/warehouse#15716 will follow this convention.
@ewdurbin ewdurbin mentioned this pull request Sep 19, 2024
Open
@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!

woodruffw
woodruffw reviewed Sep 23, 2024
View reviewed changes
tests/unit/api/test_simple.py
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
@woodruffw
test_simple: fix accidentally skipped test
6a9271d 
See pypi#15716 (review)

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw mentioned this pull request Sep 23, 2024
Merged
di pushed a commit that referenced this pull request Sep 23, 2024
@woodruffw
test_simple: fix accidentally skipped test (#16777)
0d7cb20 
See #15716 (review)

Signed-off-by: William Woodruff <william@trailofbits.com>
finswimmer added a commit to finswimmer/poetry that referenced this pull request Sep 23, 2024
@finswimmer
fix(test): update pypi fixtures
29c14f2 
PyPI implemented alternate location for PEP 708. (pypi/warehouse#15716)
@finswimmer finswimmer mentioned this pull request Sep 23, 2024
Merged
finswimmer added a commit to python-poetry/poetry that referenced this pull request Sep 23, 2024
@finswimmer
fix(tests): update PyPI fixtures (#9704)
150ff09 
PyPI implemented alternate location for PEP 708. (pypi/warehouse#15716)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ewdurbin ewdurbin ewdurbin approved these changes

@woodruffw woodruffw woodruffw left review comments

@dstufft dstufft Awaiting requested review from dstufft

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cofiem @ewdurbin @dstufft @woodruffw