-
Notifications
You must be signed in to change notification settings - Fork 50
Switch to Photon for thumbnail generation #1056
Conversation
API Developer Docs Preview: Ready https://wordpress.github.io/openverse-api/_preview/1056 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
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 looks great! Thanks for the testing instructions, I was able to get the image with query params to work exactly as expected 😄
I know you have a few more changes to make and I'm happy to re-review at that point if you'd like ✅ 🚀
@@ -82,6 +73,7 @@ services: | |||
- cache | |||
env_file: | |||
- api/env.docker | |||
- api/.env |
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.
Great idea! I like this for longer-lasting changes and I was always frustrated by having to jettison changes to api/env.docker
.
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, risky! Too easy to accidentally commit secrets if I put them in that file 😬
a91bb64
to
c5335dd
Compare
@sarayourfriend I rebased this PR so it could include the fix from #1059 and build properly. It looks like the new |
Thanks, Madison! |
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.
Sorry for late review. I tried running this PR yesterday, before Madison's fixes, and just recreate
was failing.
Today I managed to run the steps in testing
, and it worked fine. I tried to see what request we send to photon
in the Docker logs, but couldn't find anything. All I see is this:
2023-01-06 16:38:49 [2023-01-06 13:38:49,838 - log_request_id.middleware - 47][INFO] [627312109a744e3f93e755da58f53582] method=GET path=/v1/images/230c6951-7f83-4263-a0da-a151f9f67b21/thumb/ status=200
2023-01-06 16:38:49 [06/Jan/2023 13:38:49] "GET /v1/images/230c6951-7f83-4263-a0da-a151f9f67b21/thumb/ HTTP/1.1" 200 49764
Do we intentionally not log the photon request, or is it not necessary?
0d5bb39
to
f94ce4d
Compare
@obulat We only logged thumbnail requests as debug logs, as far as I could tell. I'm not entirely sure what the usefulness of them is unless we have a specific issue we're trying to diagnose. I merged the PR as is because it'd been open for quite some time, but we can iterate in a separate PR to add additional logging if desired. |
Fixes
Fixes #979 by @AetherUnbound
Description
Now that the WordPress.com Photon instance supports an authentication header to allow us to forward query params, we're able to use it for thumbnail generation.
Note: This PR is still missing:
catalog.api.utils.photon
moduleTesting Instructions
Checkout the branch and
just recreate
. Test thumbnails and confirm they work.If you have access to the Openverse secrets, you can add the
PHOTON_AUTH_KEY
variable to yourapi/.env
file to enable query forwarding.I am using the following additional sample data to be able to test the query forwarding:
This is a quick and dirty way to get a URL with a query parameter into the sample data. Go directly to the image URL using the identifier: http://localhost:50280/v1/images/230c6951-7f83-4263-a0da-a151f9f67b21/
It should work fine. Now click the "thumbnail" link and see that the thumbnail is as expected (compressed, resized version of the upstream image).
Checklist
Update index.md
).main
) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin