-
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
Allow email field to be templated #35546
Allow email field to be templated #35546
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
@gdavoian Thanks for the PR! :) I just wanted to understand this better, why |
@utkarsharma2 I've just updated the description of the PR with the use case of our team. |
Can you also update/add test case (see the example test case in #29821 as an example. |
@gdavoian No strong objections here, but I'm a little concerned that if it is a specific case of your team then the downside of this is we add an overhead of rendering this field for all operators in airflow for everyone who uses it. Also, I checked the code, and |
@utkarsharma2 But the Honestly, I don't know how that caching works, but we use templating heavily (not only for email lists but also for things like the links to docker images, etc.), and currently, the only thing we can't really template is the |
Yeah. It looks like a useful case - mostly because of the specific place where it is used. Actually this case is I think largerly superseeded by Notifiers, but having this specific exception for email does not seem to have a side-effect, and I can easily imagine how it might be useful. We've been discussing the reasoning for that with @gdavoian and @hussein-awala in #29821 (comment) and it seems like it would not hurt to add it |
Nice! |
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Since email is the only field we can exclude, I wonder if the user really needs it. We have an alternative way to achieve that without using the email, email_on_retry, and email_on_failure params, where the user can use on_failure_callback and on_retry_callback with a SmtpNotifier which support templated email. WDYT? |
I think that having an alternative (and more customizable) way of sending email notifications via callbacks is a good thing, but in most cases, the basic mechanism implemented via the Speaking about if the user needs it, I think yes. For example, we have multiple separate Airflow environments, with hundreds of DAGs overall, and we heavily rely on the |
@hussein-awala Crossed my mind too actually ... I'd even say maybe a better overal solution would be to convert the email into SMTPNotifier under the hood (and treat it as syntactic sugar rather than field in the operator? Maybe then we could get rid of it being a field and get rid of the exclusion. This does look like a special case and the notifiers are nicely addressing the problem.
I see the need, but likely in this case it does not have to be FIELD in the operator, simply if it is set in the constructor we could - under-the-hood, configure notifiers automatically. That would not change anything in the API (you'd stil set templated email as |
@gdavoian I just wanted to thank you for this contribution. I spent a couple of days on and off trying to figure out if this is possible. I've upgraded to v2.8.0 and am trying this functionality now, but am not able to get it working so far. Maybe I am doing something wrong, but I have my default_args listed as so:
However, this is resulting in this error:
Am I doing something wrong here? I have my Variable defined in the Airflow UI, and the value looks like this: Any help is greatly appreciated. |
@tlochner95 it's nice to hear that I'm not alone :) First of all, I would join the items of your email list with commas, as it's rendered as a string, not as a list (unless you set
Secondly, the fact that
If you mostly use built-in or third-party operators, another option is to implement a utility function that monkey-patches operators in the current scope on the fly, which is exactly what we're doing. I can't share the source code, but here's a hint on how you could implement it yourself: start with |
Just to add to it. It's actually rather easy to update template_field in your Dag - you do not need to recursively update BaseOperator. from .... import AnyOperator
AnyOperator.template_fields = ('email',)
task = AnyOperator(....) That one should do the job. |
Thank you both very much for your responses. I was able to get a working example using both of your help. Unfortunately for us, we are using AWS MWAA to host Airflow and will need to wait until they support v2.8.0 to be able to use the feature, but until then thank you! |
The current serialization logic treats all of the
BaseOperator
fields as non-templateable by default. However, theemail
field may considered an exception to that rule and may be allowed to be templated.For example, our team heavily relies on templating this field (e.g.,
"{{ var.value.some_email_list }}"
) as thedefault_args["email"]
for each and every DAG we have, and we don't want to useVariable.get("some_email_list")
on the module level of our scripts instead because it's actually an anti-pattern (opening a DB connection and making a SQL query under the hood at parsing time and not at execution time).See the discussion under #29821 for more details.
^ 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.rst
or{issue_number}.significant.rst
, in newsfragments.