-
Notifications
You must be signed in to change notification settings - Fork 54
Conversation
d9727dd
to
9ebe5e1
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.
Yay for deleted code! 🥳 I have one note, and it looks like the --extended
tests are failing 🤔
super().__post_init__() | ||
# Override the dag_id | ||
self.dag_id = f"{self.provider_name}_reingestion_workflow" | ||
return | ||
|
||
super().__post_init__() |
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.
Do we need to call super().__post_init__()
twice? 😮
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.
Yep, unfortunately. If a dag_id
wasn't provided in the configuration, we want to override it. We need to call super first in order to set the provider_name
(which is used in building the dag_id), and then we can override it. We can't do this check after calling super, because dag_id
will have been set (to the regular _workflow
name) during the super call. So we must call super within the branch.
But if dag_id
was provided, we still want to call super(), so we also have to do it outside the branch. The return on L53 prevents it from actually being run twice.
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.
Oh, oops! Okay I see, we're not actually calling it twice as you point out. Thanks!
a3291db
to
5ad46cc
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.
🚀 Woohoo!
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.
If you're looking for a thrill set INGESTION_LIMIT to 10 and turn them all on 🥳
Indeed, I am, so I turned on many of the DAGs 😆 awesome work!
Fixes
Fixes WordPress/openverse#1398 by @AetherUnbound
Description
Note: #848 should be merged first and this will need to be rebased to pull in Europeana changes.
Not included:
It may be time to revisit the
generate_tsv_filenames
logic, which I have not touched in this PR beyond deleting fully-unused legacy branches. Should we open a new issue for this?Testing Instructions
just test
Test some DAGs! Make sure to try at least one audio-only, one image-only, one audio-and-image, and one reingestion workflow. If you're looking for a thrill set
INGESTION_LIMIT
to 10 and turn them all on 🥳Checklist
Update index.md
).main
) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin