potiuk commented on code in PR #37701:
URL: https://github.com/apache/airflow/pull/37701#discussion_r1502522303
##########
airflow/providers/smtp/notifications/smtp.py:
##########
@@ -25,6 +25,24 @@
from airflow.notifications.basenotifier import BaseNotifier
from airflow.providers.smtp.hooks.smtp import SmtpHook
+try:
+ from airflow.settings import SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH,
SMTP_DEFAULT_TEMPLATED_SUBJECT
+except ImportError:
+ # This is a fallback for when the settings are not available - they were
only added in 2.8.1,
+ # so we should be able to remove it when min airflow version for the SMTP
provider is 2.9.0
+ # we do not raise deprecation warning here, because the user might be
using 2.8.0 and the new provider
+ # deliberately, and we do not want to upgrade to newer version of Airflow
so we should not raise the
+ # deprecation warning here. If the user will modify the settings in
local_settings even for earlier
+ # versions of Airflow, they will be properly used as they will be imported
above
+ SMTP_DEFAULT_TEMPLATED_HTML_CONTENT_PATH = (Path(__file__).parent /
"templates" / "email.html").as_posix()
+ SMTP_DEFAULT_TEMPLATED_SUBJECT = """
+{% if ti is defined %}
+DAG {{ ti.dag_id }} - Task {{ ti.task_id }} - Run ID {{ ti.run_id }} in State
{{ ti.state }}
+{% elif slas is defined %}
+SLA Missed for DAG {{ dag.dag_id }} - Task {{ slas[0].task_id }}
+{% endif %}
+"""
+
Review Comment:
Yes. Agree. But let's address the compatibility first. The problem is that
we already have this out and it's being used and possible to configure it in
2.8.1+ (and current version of provider does not work with 2.6.0 - 2.8.0 when
installed.
Agree we should likely change it in the future (as a new feature in smtp
provider) but I think we should first make a fix to fix this problem quickly
and possibly change how the settings are configured.
Now as I see it, it should be a new configuration in SMTP provider (in
provider.yaml) rather that via airflow settings (that was indeed bad idea and
we missed it). But this will be quite a bit bigger change and I would rather
first release a patchlevel fix and only after that release a new provider with
new MINOR level - deprecating settings and replacing them with configuration.
--
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]
