SameerMesiah97 commented on code in PR #74605:
URL: https://github.com/apache/airflow/pull/74605#discussion_r4251495848
##########
task-sdk/src/airflow/sdk/execution_time/task_runner.py:
##########
@@ -2236,7 +2239,11 @@ def _send_error_email_notification(
to=to_emails,
subject=subject,
html_content=html_content,
- from_email=conf.get("email", "from_email",
fallback="airflow@airflow"),
+ # SmtpNotifier resolves the sender itself, preferring the SMTP
connection's from_email
+ # extra over the [email] from_email config; passing a value here
would hide the extra.
+ from_email=None
+ if using_smtp_notifier
+ else conf.get("email", "from_email", fallback="airflow@airflow"),
Review Comment:
nit: I would move the resolution of `from_email` before the calling
`notifier_class`:
```
from_email = None
if not using_smtp_notifier:
from_email = conf.get("email", "from_email", fallback="airflow@airflow")
notifier = notifier_class(
to=to_emails,
subject=subject,
html_content=html_content,
from_email=from_email,
)
```
##########
providers/smtp/tests/unit/smtp/notifications/test_smtp.py:
##########
@@ -238,6 +238,53 @@ def test_notifier_with_nondefault_connection_extra(
**DEFAULT_EMAIL_PARAMS,
)
+ @pytest.mark.parametrize(
+ ("connection_from_email", "config_from_email", "expected"),
+ [
+ pytest.param(TEST_SENDER, None, TEST_SENDER, id="connection-only"),
+ pytest.param(None, TEST_SENDER, TEST_SENDER, id="config-only"),
+ pytest.param(TEST_SENDER, f"config_{SENDER_EMAIL_SUFFIX}",
TEST_SENDER, id="connection-wins"),
+ ],
+ )
+ @mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook")
+ def test_notifier_from_email_precedence(
+ self,
+ mock_smtphook_hook,
+ create_dag_without_db,
+ connection_from_email,
+ config_from_email,
+ expected,
+ ):
+ """Without an explicit from_email, the connection extra wins over the
[email] config."""
+ mock_smtphook_hook.return_value.__enter__.return_value.from_email =
connection_from_email
+
mock_smtphook_hook.return_value.__enter__.return_value.subject_template = None
+
mock_smtphook_hook.return_value.__enter__.return_value.html_content_template =
None
+
+ with conf_vars({("email", "from_email"): config_from_email}):
+ notifier = SmtpNotifier(to=TEST_RECEIVER, subject=TEST_SUBJECT,
html_content=TEST_BODY)
+ notifier({"dag": create_dag_without_db(TEST_DAG_ID)})
+
+
mock_smtphook_hook.return_value.__enter__().send_email_smtp.assert_called_once_with(
+ from_email=expected,
+ to=TEST_RECEIVER,
+ subject=TEST_SUBJECT,
+ html_content=TEST_BODY,
+ smtp_conn_id=SMTP_CONN_ID,
+ **DEFAULT_EMAIL_PARAMS,
+ )
+
+ @mock.patch("airflow.providers.smtp.notifications.smtp.SmtpHook")
+ def test_notifier_without_any_from_email_raises(self, mock_smtphook_hook,
create_dag_without_db):
+ """With no sender anywhere, the notifier still reports it rather than
inventing one."""
+ mock_smtphook_hook.return_value.__enter__.return_value.from_email =
None
+
mock_smtphook_hook.return_value.__enter__.return_value.subject_template = None
+
mock_smtphook_hook.return_value.__enter__.return_value.html_content_template =
None
+
+ with conf_vars({("email", "from_email"): None}):
+ notifier = SmtpNotifier(to=TEST_RECEIVER, subject=TEST_SUBJECT,
html_content=TEST_BODY)
+ with pytest.raises(ValueError, match="You should provide
`from_email`"):
+ notifier({"dag": create_dag_without_db(TEST_DAG_ID)})
+
Review Comment:
You could assert this too:
`mock_smtphook_hook.return_value.__enter__.return_value.send_email_smtp.assert_not_called()`
##########
providers/smtp/src/airflow/providers/smtp/notifications/smtp.py:
##########
@@ -122,9 +125,11 @@ def hook(self) -> SmtpHook:
def _build_email_content(self, smtp: SmtpHook, context: Context):
fields_to_re_render = []
if self.from_email is None:
- if smtp.from_email is not None:
- self.from_email = smtp.from_email
- else:
+ # The connection is the more specific setting, so it wins over the
[email] from_email
+ # config. Callers that resolve the config themselves and pass the
result in keep that
+ # value; see the sender precedence in the class docstring.
+ self.from_email = smtp.from_email or conf.get("email",
"from_email", fallback=None)
+ if self.from_email is None:
raise ValueError("You should provide `from_email` or define it
in the connection")
Review Comment:
The error message could be better. Please see the below:
```
raise ValueError(
"You should provide `from_email`, define it in the connection, "
"or configure [email] from_email"
)
```
--
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]
