Add general-purpose "notifier" concept to DAGs #28569
Conversation
Taragolis
left a comment
Taragolis
left a comment
There was a problem hiding this comment.
Love the idea. Just couple of findings
ephraimbuddy
force-pushed
the
notifier
branch
2 times, most recently
from
db0a827 to
ed5d175
Compare
|
What I'm finding out now is that this solution is not backward-compatible for providers |
Maybe we could mark it as Optional feature by raise |
Awesome. Didn't know that existed. Thanks |
ephraimbuddy
force-pushed
the
notifier
branch
2 times, most recently
from
013dbb9 to
afc3732
Compare
ephraimbuddy
force-pushed
the
notifier
branch
3 times, most recently
from
fb2658d to
ee928bb
Compare
ephraimbuddy
force-pushed
the
notifier
branch
from
ee928bb to
5c329c8
Compare
|
Good candidate to be added to #28300 - I will add it there once merged. |
|
@jedcunningham @uranusjr @dstandish, can you also take a look |
There was a problem hiding this comment.
Can we put this default value to a module-level constant?
There was a problem hiding this comment.
Also referencing the main branch is probably a bad idea; we want something more permanent.
There was a problem hiding this comment.
Linked to a previous release, how's that?
There was a problem hiding this comment.
Probably better linking to a commit, but I guess we have enough dev infra around this a tag is immutable-ish enough.
ephraimbuddy
force-pushed
the
notifier
branch
3 times, most recently
from
e5cb596 to
3a0b79b
Compare
This makes it easy for users to setup notifications for their DAGs using on_*_callbacks It's extensible and we can add it to more providers. Implemented a SlackNotifier in this phase. In the course of this, I extracted a 'Templater' class from AbstractBaseOperator and have both the Notifier & ABO inherit from it. This is necessary in other to avoid code repetition.
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
docs/apache-airflow-providers-slack/notifications/slack_notifier_howto_guide.rst
Show resolved
Hide resolved
kaxil
left a comment
kaxil
left a comment
There was a problem hiding this comment.
Should we add docs for this feature @ephraimbuddy ? Currently this feature is a "hidden" one
Agree we should. |
Seeing this now. There's a doc on it: http://apache-airflow-docs.s3-website.eu-central-1.amazonaws.com/docs/apache-airflow-providers-slack/latest/notifications/slack_notifier_howto_guide.html |
What I think @kaxil (I for sure) mean is that this adds "base" notifier and "generic" approach that could be extended by anyone. Similarly as other "core extensions" - I think we need some generic description on how one can extend and write their own notifiers (or at the very least information that one CAN actually do it) Similar docs are here for logging, extra links, logging, secret backends - look here (there are links to generic airlfow how-tos from there) https://airflow.apache.org/docs/apache-airflow-providers/core-extensions/index.html |
Fixed: #29191 |
6 participants
This makes it easy for users to setup notifications for their DAGs using on_*_callbacks It's extensible and we can add it to more providers. Implemented a SlackNotifier in this phase.
In the course of this, I extracted a 'Templater' class from AbstractBaseOperator and have both the Notifier & ABO inherit from it. This is necessary in other to avoid code repetition.
Closes: #28405