-
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
Move fivetran sync provider code to this repo #33
Conversation
Hi @phanikumv can please take a look to this PR? |
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 suggestion of merge it into one operator (on this repo) , but need to handle deprecation of Async class. There are existing users who use Async class.
fivetran_operator = FivetranOperator( | ||
task_id="fivetran-operator", | ||
fivetran_conn_id="fivetran_default", | ||
connector_id="{{ var.value.connector_id }}", |
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.
Are we testing dags in CI? or AstroCloud?
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.
manual testing is done on Astro cloud
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.
we havent automated it
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.
yes, we have manual testing. I have attached the dagrun screenshot
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.
Tests are missing for the implementation
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.
Would be nice to review once operators are merged and tests are added.
created a follow-up GH issue for depreciation and merging both operator/sensor #35 |
d086d3a
to
aeed44a
Compare
Co-authored-by: Ankit Chaurasia <[email protected]>
closes: #30
Move code from https://github.com/fivetran/airflow-provider-fivetran to this repo
Example DAG