-
Notifications
You must be signed in to change notification settings - Fork 16.3k
feat: add async discord notifier #56911
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
Conversation
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Show resolved
Hide resolved
2b61dc2 to
4394a97
Compare
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
1e45571 to
7e08be7
Compare
|
Looks like you have a handful of comments already, so I'll let things settle a bit before I review, but feel free to ping me when you are ready. As always, thanks for the work! |
Lee-W
left a comment
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 still need some work to make the CI green. Mark it as draft before it's ready to review again. Thanks!
a6ba5c0 to
94c90a4
Compare
|
CI is green now. In my opinion, it makes sense to continue with this feature if it looks good. Yes, the backwards compatible part is redundant with #57143, but in my opinion we could add it to our open items list and change all providers to use the WDYT ? @ferruzzi @ramitkataria |
I'm not thrilled with how many todos we're leaving hanging, but I guess that's life. Resolve the comments that are resolved and I'll make a pass tomorrow. |
|
I'll mark it as draft. But please let us know when it's ready again. Looking forward to this one! |
94c90a4 to
0ffe1fc
Compare
Let's reduce our list of open items. I have opened a PR for the Slack provider for use |
|
Thank you for all the patience and support. I'm opening it up for another round of reviews. |
To be clear, that wasn't a jab at you by any means. Just in case it came off the wrong way. I greatly appreciate your work on these and hope I didn't imply otherwise. |
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
0ffe1fc to
2de3d0c
Compare
Don't worry, I got it right. I like keeping things clean and I consider myself to have an intrinsic motivation to clean up my own things. |
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Show resolved
Hide resolved
0007227 to
be0b3ce
Compare
be0b3ce to
e349bef
Compare
|
Is there anything I can do to move forward with this PR? |
|
Will take a look tomorrow. |
Lee-W
left a comment
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.
mostly good, a nit and a typo.
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
providers/discord/tests/unit/discord/notifications/test_discord.py
Outdated
Show resolved
Hide resolved
c0ab5ec to
5598892
Compare
|
Approve as the remaining one is a nitpick. @dondaum, please click resolve for the comments you think you've already resolved :) |
5598892 to
ba67b84
Compare
|
I'll keep it open for a few days so that other can take a look. But overall looks good! Thanks! |
jason810496
left a comment
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 the PR! LGTM to me overall !
providers/discord/src/airflow/providers/discord/hooks/discord_webhook.py
Outdated
Show resolved
Hide resolved
ba67b84 to
1f02827
Compare
1f02827 to
280265c
Compare
Add an asynchronous version of
DiscordNotifier.related: #55237
I sucessfully tested the
DiscordNotifierwith the following Dag:^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.