-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Amazon appflow #24057
Amazon appflow #24057
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.
Thanks for tackling this, it's a lot of work!
I have not used AppFlow, I assume you've tested these against the actual service and they work as you'd expect? I left some comments based on code conventions we use in the community and I'll have to finish looking at the Operators later. So far just a few smallish stylistic requests/suggestions.
@ferruzzi Yep! I developed this operator six months ago during an AWS Data Lab engagement alongside some AWS SAs. Since then, it has been running in production for |
That's cool, thanks for contributing it back to the community! Thanks for the changes, I know a lot of them were little nitpicks. I'll need to finish looking at the other half of the operators tomorrow. 👍 |
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.
Really nice contribution! Other than the minor notes from Dennis and Vincent this is a very solid PR with testing, docs, example dags, etc, all up to current standards 💯 Thanks for your work!
Thank you @o-nikolas @ferruzzi @vincbeck for all the great suggestions. I think I've already addressed all of them. Are we good to move on? 😄 |
Awesome work! LGTM! |
I haven't had time to look at the second half of the operators, but most of my comments were stylistic, so as long as you carried them forward I think we're fine. We'll still need a Committer to approve and merge it. |
Can we please change example dags/system tests in AIP-47 compliant way? You can see many of PRs doing it recently and I think we are at the verge of makign it "default" way of adding the examples. |
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.
Lots of great work here! Overall, some minor comments and suggestions.
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
Co-authored-by: Josh Fell <48934154+josh-fell@users.noreply.github.com>
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 great! 👍 Small comments re: using the functools compat module now and a question on the spelling wordlist change (if it's needed, I don't think it is).
Can you make sure to rebase when you push these changes too? We can merge after.
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease. |
Thank you @josh-fell! |
Nice work @igorborgest! @vincbeck @o-nikolas @ferruzzi @Taragolis Thanks for all the feedback along the way! |
Hi!
It's a proposal to add Amazon AppFlow support for the
apache-airflow-providers-amazon
package.This is an initial support tested only against two sources:
Salesforce
andZendesk
.Adding 1 new hook and 6 new operators + all related tests, docs and examples: