ephraimbuddy commented on code in PR #31887:
URL: https://github.com/apache/airflow/pull/31887#discussion_r1231227599
##########
airflow/notifications/basenotifier.py:
##########
@@ -79,15 +79,30 @@ def notify(self, context: Context) -> None:
"""
...
- def __call__(self, context: Context) -> None:
+ def __call__(self, *args) -> None:
"""
Send a notification.
:param context: The airflow context
"""
- context = self._update_context(context)
- self.render_template_fields(context)
+ # Currently, there are two ways a callback is invoked
+ # 1. callback(context) - for on_*_callbacks
+ # 2. callback(dag, task_list, blocking_task_list, slas, blocking_tis)
- for sla_miss_callback
+ # we have to distinguish between the two calls so that we can prepare
the correct context,
+ # comparing len(args) is one straightforward way of checking this.
+ if len(args) == 1:
+ _context = args[0]
Review Comment:
This will still causes issues. What if the first arg is `dag`?.
Suggestion:
Let's have a method that implements sla callback in this base notifier. eg.
`sla_notify`. It should take all the necessary args that normal sla callback
takes. The notifier `__call__ `should expect an arg called `is_sla` which when
true would call the `sla_notify` instead of notify
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
