-
Notifications
You must be signed in to change notification settings - Fork 7
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
add stream_logs
parameter to trigger_sync
#28
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.
Looks nice! Can you share a screenshot/code snippet of what the output looks like?
Sample output when
|
Thanks for addressing the comments! Was wondering if this is ready for another review? |
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 like the direction that these changes takes us, but unfortunately there are some breaking changes. In order to support this I think we'll need to deprecate the current tasks and create a new ones to give users a smooth transition. The old tasks can emit a DeprecationWarning
and guide users to use the new tasks/flows. This will also be a good chance to introduce a block, since that might lead to some breaking changes as well.
Let me know if there's any part of this that you'd like to discuss and/or pair on!
Summary
This PR includes exposes a
stream_logs
parameter to thetrigger_sync
task to surface airbyte sync logs through the Prefect logger and adds associated tests / fixtures.This required refactoring
AirbyteClient.get_job_status
to conditionally return logs ifstream_output
is True.This PR also switches from the standard python logger to the logger fetched by
get_run_logger()
, which required a change to all associated unit tests that previously called theTask.fn()
method.Relevant Issue(s)
addresses #19
Checklist