-
Notifications
You must be signed in to change notification settings - Fork 72
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
PyPI live json API #306
PyPI live json API #306
Conversation
Please squash the commits, and change the formatting of the issue tag so that the automation will recognize it. I think it needs to be in the form
|
f9f2999
to
2d5d088
Compare
@dralley This PR is almost ready. I left some comments in the code I added about parts of the JSON I left out or thought could be different. There's lots of fields in the JSON that aren't really necessary to have, but could be included in the Python content if we wanted. Let me know what you think. |
819af76
to
e3a8136
Compare
pulp_python/app/utils.py
Outdated
""" | ||
# Would have been nice to use a serializer, | ||
# but I couldn't figure out how to pass in the request context | ||
info = dict(latest_content.__dict__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH, I think it might be easier (and cleaner) to just manually create your dict with the attributes you want to add, rather than grabbing the internal __dict__
and removing stuff.
d19beb2
to
0aae380
Compare
@@ -33,6 +37,41 @@ class PythonDistribution(PublicationDistribution): | |||
|
|||
TYPE = 'python' | |||
|
|||
def content_handler(self, path): | |||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Someone more familiar with the content app / live APIs should take a look at this also, maybe Ina or Dennis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the correct use of the content_handler() interface. I read about how PurePath.match() works and this looks like a good use of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually i was wondering, why the content_handler
wasn't using an async
interface, but it is declared like that in pulpcore. Not much you can do about it here.
pulp_python/app/models.py
Outdated
version=version), | ||
headers=headers) | ||
finally: | ||
pass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to keep this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially it use to be a bare exception catch, but the linter keep yelling so I switched it to finally. I don't think it's necessary to wrap it in a try/except block, but there are two database queries so I am not sure if they have potential for errors.
pulp_python/app/utils.py
Outdated
""" | ||
releases = {} | ||
for content in content_query: | ||
list_packagetypes = releases.setdefault(content.version, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think defaultdict
would be cleaner or is there a particular reason not to use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultdict does make it cleaner, I just didn't think of using it.
from packaging.version import parse | ||
|
||
PYPI_LAST_SERIAL = "X-PYPI-LAST-SERIAL" | ||
PYPI_SERIAL_CONSTANT = 1000000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe i am missing the point, but hardcoding a serial number feels wrong.
edit: I saw the corresponding comment in that other file. Maybe add something here, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a comment for that, thanks.
Pulp now supports PyPi's json API with the new content endpoint '/pypi/{package-name}/json'. This will enable basic syncing from other Pulp instances. Also package classifiers are now included in Python content. fixes: #2886 https://pulp.plan.io/issues/2886 fixes: #3627 https://pulp.plan.io/issues/3627
0aae380
to
c252306
Compare
@gerrod3 This LGTM, I'm just waiting on a review from someone with experience writing a live API for the content app. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please open a story for tracking the content 'last serial'. Do we also need to add a story to add the missing fields listed in python_content_to_info?
@gerrod3 Thanks for this PR! Excellent work :) As Dennis mentioned, we should file a task/issue/story for any missing features that would be useful for us to implement |
Still working on implementing more tests, but there are now two new content endpoints to support PyPi's json API. '/pypi/{package-name}/json' and '/pypi/{package-name}/{version}/json' which live generate the package's PyPi json based on the package's published content. This endpoint will now allow basic syncing from other Pulp instances.