-
Notifications
You must be signed in to change notification settings - Fork 50
Fix issue due to page_query_param
not being a string
#723
Conversation
API Developer Docs Preview: Ready https://wordpress.github.io/openverse-api/_preview/723 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.
Fixes the error that prevents the docs from rendering!
@@ -142,6 +142,10 @@ jobs: | |||
- name: Start API, ingest and index test data | |||
run: just init | |||
|
|||
- name: Smoke test ReDoc site |
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.
🔥
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.
When would this fail? On 500 error from the server? Would it fail on main?
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.
It fails for any non-success status code from the endpoint. So yes, it fails on main as curl
returns exit code 22 if the response status code is 500.
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.
Hmm, I'm trying this command against api-dev.openverse.engineering
(which is currently failing to load) and it's passing just fine 😟 When I add the -L
flag for curl
it fails as you describe. Do we need that flag here too?
Update: It does not! I tried this locally on main and it failed as expected. I think it's because we aren't using a proxy in this case.
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.
Love the addition of the smoke test!
Fixes
Fixes #722 by @AetherUnbound
Description
This PR fixes an error introduced in #699 that prevented the ReDoc pages from showing up. This is because the value of the field
page_query_param
is expected to be a string.By setting pagination params to an empty list, this problem goes away.
Testing Instructions
http://localhost:8000/v1
.Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin