[PULP-715] WIP: Add JSON-based Simple API #956
Conversation
github-actions
bot
added
no-changelog
multi-commit
jobselko
force-pushed
the
close_625
branch
2 times, most recently
from
16f7486 to
03e61c7
Compare
github-actions
bot
removed
the
multi-commit
jobselko
force-pushed
the
close_625
branch
2 times, most recently
from
c7e19ec to
6647217
Compare
jobselko
force-pushed
the
close_625
branch
4 times, most recently
from
5f68a09 to
5ad38d9
Compare
| packagetype = models.TextField(choices=PACKAGE_TYPES) | ||
| python_version = models.TextField() | ||
| sha256 = models.CharField(db_index=True, max_length=64) | ||
| sha256_metadata = models.CharField(max_length=64) |
There was a problem hiding this comment.
This needs to be nullable since we will only get values for wheels and only if we download the artifact and extract the wheel. On-demand syncs won't be able to get this value since it's not in the PYPI JSON api (but maybe we can try the packages simple JSON api, probably in another PR).
There was a problem hiding this comment.
Should there be a management command capable of downloading the packages and updating the values from the packages?
There was a problem hiding this comment.
Maybe, we have a repair command and endpoint that could do this.
| # todo (from new PEPs): | ||
| # size (v1.1, PEP 700) | ||
| # upload-time (v1.1, PEP 700) | ||
| # core-metadata (PEP 7.14) | ||
| # provenance (v1.3, PEP 740) |
There was a problem hiding this comment.
Let's leave these for future PRs
There was a problem hiding this comment.
do you want the comments wholly removed? I can create new issues for them if they don't already exist
There was a problem hiding this comment.
Keep the comments, lets file new issues for them.
There was a problem hiding this comment.
Want links to the new issues in the comments themselves?
There was a problem hiding this comment.
No it's fine as long as the issue states the PEP.
| """TODO This serial constant is temporary until Python repositories implements serials""" | ||
| PYPI_SERIAL_CONSTANT = 1000000000 | ||
|
|
||
| PYPI_API_VERSION = "1.0" |
There was a problem hiding this comment.
Should this be named SIMPLE_API_VERSION? Also the meta tag in the simple_index_template should be updated to the tag they expect <meta name="pypi:repository-version" content="{SIMPLE_API_VERSION}">
There was a problem hiding this comment.
It could go either way, I hate the name "simple api" in general, especially now that it refers to completely and utterly different APIs... On the other hand "PyPI api" is maybe too specific?
I lean towards PYPI_API but it's marginal.
There was a problem hiding this comment.
I think the rename is better since this constant is for the simple api, both the html and json versions: https://packaging.python.org/en/latest/specifications/simple-repository-api/ and it falls under the /simple/ urls.
There is a separate PyPI json api (that we use for syncing) that is "unique" to PyPI (https://docs.pypi.org/api/json/) and falls under the /pypi/ urls, which we also implement so we can perform Pulp-to-Pulp syncing.
If you don't like SIMPLE_API you can also name it INDEX_API since that is also what it is called.
| {% for name, path, sha256 in project_packages %} | ||
| <a href="{{ path }}#sha256={{ sha256 }}" rel="internal">{{ name }}</a><br/> | ||
| {% for pkg in project_packages %} | ||
| <a href="{{ pkg.url }}#sha256={{ pkg.sha256 }}" rel="internal">{{ pkg.filename }}</a><br/> |
There was a problem hiding this comment.
What do you think of adding some of the link attributes like `data-yanked``? https://packaging.python.org/en/latest/specifications/simple-repository-api/#project-detail
There was a problem hiding this comment.
We could also do this in a future PR and add a todo item here.
There was a problem hiding this comment.
I'll look into how much work this will require, probably to be done in a separate PR, but I can file an issue for it separately
| metadata_relative_path = f"{project_dir}index.html" | ||
|
|
||
| with open(metadata_relative_path, "w") as simple_metadata: | ||
| # todo? |
There was a problem hiding this comment.
Let's leave this for the future. We might not implement the feature for publications if we are planning to remove them for Pulp 4.
There was a problem hiding this comment.
What actually is the TODO item here? Produce JSON for the publications yeah? In any case I agree so long as the current recommended workflow doesn't involve publications. If it does then that would be a little awkward
There was a problem hiding this comment.
Yes that is the todo, currently our docs doesn't mention that you don't need publications (https://pulpproject.org/pulp_python/docs/user/guides/publish/) or that new features are not implemented for publications.
There was a problem hiding this comment.
Well in that case we should probably file an issue to fix that documentation -- would you say that the publication workflow is actually deprecated already?
There was a problem hiding this comment.
No, it's not deprecated in the sense that we will still fix issues with it, but we will point users to use the new preferred method if they want all the latest features.
| "filename": release_package.filename, | ||
| "url": d_url, | ||
| "sha256": release_package.digests.get("sha256", ""), | ||
| # todo: more fields? |
There was a problem hiding this comment.
I think pypi-simple can extract all the fields so I would say yes, let's add them. We will also need to update pull_through_package_simple to take in the media type to see if we should respond with html or json.
| # todo: how to get these / should they be here? | ||
| # data["yanked"] = False | ||
| # data["yanked_reason"] = "" | ||
| # data["sha256_metadata"] = "" |
There was a problem hiding this comment.
| # todo: how to get these / should they be here? | |
| # data["yanked"] = False | |
| # data["yanked_reason"] = "" | |
| # data["sha256_metadata"] = "" |
Handling them in parse_project_metadata should be fine.
| "platform": content.platform or "", | ||
| "requires_dist": json_to_dict(content.requires_dist) or None, | ||
| "classifiers": json_to_dict(content.classifiers) or None, | ||
| # todo yanked |
There was a problem hiding this comment.
Since we have the field we can give the value now.
| if package["yanked"] and package["yanked_reason"] | ||
| else package["yanked"] | ||
| ), | ||
| # gpg-sig (not in warehouse) |
There was a problem hiding this comment.
@gerrod3 Do you know why this is on the list? GPG signature support was removed from warehouse, of course other indexes could support it, but is there a good reason to do that? PEP 740 seems to be a replacement?
There was a problem hiding this comment.
If you support it then you can put it in the response, but yeah we don't since warehouse doesn't so lets remove the comment.
3 participants
closes #625