-
Notifications
You must be signed in to change notification settings - Fork 990
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
Create utility to render and store transactional snapshot of simple detail page for a project #8586
Conversation
b4f35ca
to
b967eba
Compare
b967eba
to
581e20e
Compare
581e20e
to
c76e4e7
Compare
c76e4e7
to
3745775
Compare
b142122
to
27e862a
Compare
27e862a
to
1c16dbe
Compare
@di can I get a review and sanity check here before I go about implementing tests? |
cc @woodruffw |
1c16dbe
to
33a7b76
Compare
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.
LGTM! One tiny question about when simple detail files are being stored 🙂
33a7b76
to
2de0757
Compare
Ready for review @di and @woodruffw. |
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.
LGTM!
}, | ||
) | ||
|
||
return (content_hash, simple_detail_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.
Should we return content
here instead, so the endpoint can directly render the simple page rather than having to fetch it from storage?
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, this is implemented so that we render simple detail pages in the same way that we currently do and this utility is available for TUF.
We'll need some transition plan before we start serving simple pages directly from object storage... which I consider material for a follow on PR.
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.
Also as is, we don't ever fetch back simple pages from storage from warehouse.
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.
Also as is, we don't ever fetch back simple pages from storage from warehouse.
Should we ever? Serving the request (and storing the rendered & hashed detail page) should happen every time cache is purged and a request hits the backend. The worse case is that we occasionally overwrite an existing file with itself.
For the hashed pages, we would need to serve those from storage of course.
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.
👍🏼
I'm not sure what the best next step is for this PR. It seems we have two concerns at play in review:
Initially this work is intended to provide only the first outcome, to unblock TUF integration into warehouse. For the TUF concern, there appears to only be a decision regarding path names for files stored in the object store. For the second one, I think we need the following:
|
52635fd
to
7a94d77
Compare
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.
I am working on the following implementation PR on top of this PR (related to PEP 458, TUF).
I noticed that the simple HTML files generated by render_simple_details
are empty (see my comment on warehouse/packaging/utils
).
…scussed also store unhashed index as normal
7a94d77
to
a5c60e0
Compare
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.
I think this is good to go, it puts in place the mechanism for TUF to invoke render_simple_detail
and get the hash/path for signing/service.
For now we'll remove serving all simple pages from storage from scope, and get this merged so TUF work can proceed when contributors are ready.
I'll merge this after configuring the new SIMPLE_BACKEND for test.pypi.org/pypi.org.
This commit enables the simple hash project index during the project upload file. This change will store the simple storage with the project hash index for new project uploads. This PR is a follow-up on PR pypi#8586, where it uses in the legacy file upload the render simple details as part of PEP 458. Signed-off-by: Kairo de Araujo <[email protected]>
…etail page for a project (pypi#8586) * helper function to render and hash simple detail for a specific project * use the existing Jinja2 environment * file storage * format with black * store hashed project indexes at `/simple/<HASH>.<PROJECT_NAME>` as discussed also store unhashed index as normal * add 'simple.backend' config for tests * add a jinja renderer to pyramid_request fixture * restore tests for existing functionality * test new SimpleStorage services * license * test render_simple_index utility * Update warehouse/packaging/utils.py * Update warehouse/packaging/utils.py * fix tests, store last serial information on metadata of simple files * reformat/lint * Remove print statement * Add flush * Fix tests * Simpflify duplication in storage services Co-authored-by: Dustin Ingram <[email protected]>
Needed for #8487.
Exposes
packaging.utils.render_simple_detail
for use in TUF integration.Returns
(content_hash, simple_detail_path)
The
store
flag can be used to control wether the resulting rendered/hashed simple detail page is pushed to our storage service.