-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
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! The DAG works locally.
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.
Wow, I'm pleased it was such a small change 😄 I tested using your instructions and everything matched 💯
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.
Glad that it is just a small change! The docstring also needs an update of the endpoint for preventing any confusion in the future.
Good catch, thank you @krysal! I updated the docstring to link to the documentation for the new endpoint. |
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.
Thank you!
Fixes
Fixes WordPress/openverse#1727 by @dravadhis
Description
This PR updates the Europeana DAG to use the new API endpoint under https://api.europeana.eu/ as noted in the docs.
Happily I believe no additional changes were necessary. The API documentation is quite good and I was able to confirm all the fields we're using are present and unchanged, including the cursor based pagination.
Testing Instructions
Tests should all continue to pass.
Try running the Europeana DAG locally and verify the script still works. For more thorough testing I did the following:
INGESTION_LIMIT
of 2002022-11-04
had 200+ rows.main
and repeat steps 1-3 with the old API endpoint. Make sure you run ingestion for the same day. You can trigger a manual DagRun with a conf set to{"date": "2022-11-04"}
Checklist
Update index.md
).main
) ora parent feature branch.
errors.
Developer Certificate of Origin
Developer Certificate of Origin