Skip to content
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

Modify SmtpNotifier to accept template with defaults #36226

Merged
merged 3 commits into from Dec 21, 2023

Conversation

vchiapaikeo
Copy link
Contributor

@vchiapaikeo vchiapaikeo commented Dec 14, 2023

closes: #35381

Creating a TemplatedSmtpNotifier class that simplifies sending emails based on the discussion in #35381. This class simply extends from the SmtpNotifier and defaults a number of the required arguments (like email_from, subject, and html_content). The latter two variables are pulled from airflow/settings.py.

On the template itself, I was initially thinking of having at least three distinct templates following #35381 (comment) but it's simple to add a condition to detect whether we are sending an SLA miss or a failure/success/retry email so I decided on using a single template. Happy to change that approach if the community thinks having distinct templates are cleaner.

Will work on tests after the approach is generally agreed upon.

Examples

Sample dag used to test emails -

import datetime

from airflow import DAG
from airflow.operators.bash import BashOperator
from airflow.providers.smtp.notifications.smtp import SmtpNotifier
from datetime import timedelta


with DAG(
    dag_id="my_dag",
    start_date=datetime.datetime(2021, 1, 1),
    schedule="*/5 * * * *",
    max_active_tasks=1,
    max_active_runs=1,
    sla_miss_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
):
    BashOperator(
        task_id="task",
        bash_command="sleep 180 & exit 0",
        retries=2,
        sla=timedelta(seconds=30),
        on_success_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
        on_failure_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
        on_retry_callback=SmtpNotifier(from_email=None, to="xyz@gmail.com"),
    )

on_failure_callback / on_success_callback / on_retry_callback -

Screenshot 2023-12-14 at 11 41 37 AM

sla_miss_callback -

Screenshot 2023-12-14 at 11 42 05 AM

^ 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.

@vchiapaikeo
Copy link
Contributor Author

cc @eladkal , @potiuk , @BasPH, @hussein-awala - would love to hear what you think when you have a moment

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done! I just thought about it after seeing your change, and I think doing what @eladkal suggests is not complicated; you can just add a default value for subject and html_content in the SmtpNotifier operator without needing to create a new one, and it will be fully backward compatible.

@vchiapaikeo vchiapaikeo changed the title Add TemplatedSmtpNotifier Modify SmtpNotifier to accept template with defaults Dec 18, 2023
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@potiuk potiuk merged commit a270654 into apache:main Dec 21, 2023
78 checks passed
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Jan 10, 2024
@stephenatgithub
Copy link

stephenatgithub commented Feb 26, 2024

Please notice that chicken-egg provider is encountered.

apache-airflow-providers-smtp 1.6.0 does not work with airflow 2.8.1
because modified code in airflow/settings.py is not yet released in airflow 2.8.1

I manually downgraded apache-airflow-providers-smtp to 1.5.0 and it worked fine with airflow 2.8.1 now

I was using this constraint file during installation.

Should change it to apache-airflow-providers-smtp==1.5.0 manually and then specify your own constraints

@potiuk
Copy link
Member

potiuk commented Feb 26, 2024

This is backward compatibility issue that should be fixed (it was not caught because the settings import was a local import and that's why it passed compatibility checks. Will make a PR fixing it in a moment.

potiuk added a commit to potiuk/airflow that referenced this pull request Feb 26, 2024
The apache#36226 introduced backwards compatibility check for the smtp
provider, where it relied pn airflow settings containig new settings.

This PR fixes it by falling back to default settings in case the
settings cannot be imported.
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 26, 2024
The apache#36226 introduced backwards compatibility check for the smtp
provider, where it relied pn airflow settings containig new settings.

This PR fixes it by falling back to default settings in case the
settings cannot be imported.
@potiuk
Copy link
Member

potiuk commented Feb 26, 2024

Fix in #37701

@eladkal eladkal added this to the Airflow 2.9.0 milestone Feb 26, 2024
potiuk added a commit that referenced this pull request Feb 26, 2024
The #36226 introduced backwards compatibility check for the smtp
provider, where it relied pn airflow settings containig new settings.

This PR fixes it by falling back to default settings in case the
settings cannot be imported.
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 26, 2024
Related apache#36226 and apache#37701 - where we had problems with SMTP
provider 1.6.0.
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 26, 2024
Related apache#36226 and apache#37701 - where we had problems with SMTP
provider 1.6.0.
potiuk added a commit to potiuk/airflow that referenced this pull request Feb 26, 2024
Related apache#36226 and apache#37701 - where we had problems with SMTP
provider 1.6.0.
potiuk added a commit that referenced this pull request Feb 26, 2024
Related #36226 and #37701 - where we had problems with SMTP
provider 1.6.0.
@stephenatgithub
Copy link

Wow... really quick responses and immediate actions. Thank you everyone in this airflow community.

abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
The apache#36226 introduced backwards compatibility check for the smtp
provider, where it relied pn airflow settings containig new settings.

This PR fixes it by falling back to default settings in case the
settings cannot be imported.
abhishekbhakat pushed a commit to abhishekbhakat/my_airflow that referenced this pull request Mar 5, 2024
ephraimbuddy pushed a commit that referenced this pull request Mar 6, 2024
Related #36226 and #37701 - where we had problems with SMTP
provider 1.6.0.

(cherry picked from commit 5d2a28e)
ephraimbuddy pushed a commit that referenced this pull request Mar 6, 2024
Related #36226 and #37701 - where we had problems with SMTP
provider 1.6.0.

(cherry picked from commit 5d2a28e)
@ephraimbuddy ephraimbuddy added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:misc/internal Changelog: Misc changes that should appear in change log labels Mar 20, 2024
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
The apache#36226 introduced backwards compatibility check for the smtp
provider, where it relied pn airflow settings containig new settings.

This PR fixes it by falling back to default settings in case the
settings cannot be imported.
utkarsharma2 pushed a commit to astronomer/airflow that referenced this pull request Apr 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) provider:smtp
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend Notifier interface
6 participants