shahar1 commented on code in PR #76346:
URL: https://github.com/apache/airflow/pull/76346#discussion_r4477360970
##########
providers/smtp/src/airflow/providers/smtp/utils/emailer.py:
##########
@@ -128,7 +128,7 @@ def send_email_smtp(
>>> send_email("[email protected]", "foo", "<b>Foo</b> bar", ["/dev/null"],
dryrun=True)
"""
- smtp_mail_from = conf.get("smtp", "SMTP_MAIL_FROM")
+ smtp_mail_from = conf.get("smtp", "SMTP_MAIL_FROM",
fallback="[email protected]")
Review Comment:
**nit:** With this fallback `conf.get` can't return `None`, so the `else`
below and its `ValueError` can't run, and `from_email` is always ignored.
That's already true today because core sets a default, so behaviour doesn't
change here. Please either drop the branch, or decide in the follow-up whether
`[email] from_email` should beat the built-in default.
##########
providers/smtp/docs/connections/smtp.rst:
##########
@@ -233,11 +233,10 @@ connection via **CLI**:
}'
.. note::
- The ``[smtp]`` section in ``airflow.cfg`` is used by the **core**
- e-mail helper slated for deprecation.
- When you switch to :class:`~airflow.providers.smtp.hooks.smtp.SmtpHook`
- *and* supply a ``smtp_conn_id``, the hook's connection settings take
- precedence and the global ``[smtp]`` options may be ignored.
+ The ``[smtp]`` section in ``airflow.cfg`` is used only by the email backend
in
Review Comment:
**minor:** The provider supports `apache-airflow>=2.11.0`, but
`airflow.utils.email` in 2.11.2, 3.1.8 and 3.3.2 reads `[smtp]` itself, so
"used only by" is wrong on those versions.
```suggestion
Within this provider, the ``[smtp]`` section is used only by the email
backend in
```
##########
providers/smtp/src/airflow/providers/smtp/utils/emailer.py:
##########
@@ -236,12 +236,13 @@ def send_mime_email(
:param conn_id: The ID of the SMTP connection to use.
:param dryrun: If True, the email will not be sent, but a log message will
be generated.
"""
- smtp_host = conf.get_mandatory_value("smtp", "SMTP_HOST")
- smtp_port = conf.getint("smtp", "SMTP_PORT")
- smtp_starttls = conf.getboolean("smtp", "SMTP_STARTTLS")
- smtp_ssl = conf.getboolean("smtp", "SMTP_SSL")
- smtp_retry_limit = conf.getint("smtp", "SMTP_RETRY_LIMIT")
- smtp_timeout = conf.getint("smtp", "SMTP_TIMEOUT")
+ # Fallbacks are the defaults core used to define, for cores that no longer
have an [smtp] section.
Review Comment:
**nit:** "core used to define" reads as if it already happened, but core
still defines these options.
> Keep necessary explanations short, precise and at the source of truth.
([AGENTS.md, Coding
Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#coding-standards))
```suggestion
# Same defaults as core's [smtp] section, so email keeps working once
core stops defining it.
```
##########
providers/smtp/tests/unit/smtp/utils/test_emailer.py:
##########
@@ -523,3 +522,46 @@ def test_send_mime_without_starttls(self, mock_smtp):
emailer.send_mime_email("from", "to", MIMEMultipart())
assert not mock_smtp.return_value.starttls.called
assert mock_smtp.return_value.sendmail.called
+
+ @pytest.fixture
+ def core_without_smtp_section(self, monkeypatch):
+ """Drop core's ``[smtp]`` defaults, like a core release that no longer
defines the section."""
+ defaults = copy.deepcopy(conf._default_values)
+ defaults.remove_section("smtp")
+ monkeypatch.setattr(conf, "_default_values", defaults)
+ monkeypatch.delenv("AIRFLOW_CONN_SMTP_DEFAULT", raising=False)
+ with conf_vars({("smtp", "smtp_mail_from"): None}):
+ yield
+
+ @mock.patch("smtplib.SMTP_SSL")
+ @mock.patch("smtplib.SMTP")
+ def test_send_without_core_smtp_section_uses_built_in_defaults(
+ self, mock_smtp, mock_smtp_ssl, core_without_smtp_section
+ ):
+ emailer.send_email_smtp("to", "subject", "content")
+ mock_smtp.assert_called_once_with(host="localhost", port=25,
timeout=30)
+ assert not mock_smtp_ssl.called
+ assert mock_smtp.return_value.starttls.called
+ assert mock_smtp.return_value.sendmail.call_args.args[0] ==
"[email protected]"
+
+ @mock.patch("smtplib.SMTP")
+ def test_retry_without_core_smtp_section_uses_built_in_limit(self,
mock_smtp, core_without_smtp_section):
+ mock_smtp.side_effect = SMTPServerDisconnected()
+ with pytest.raises(SMTPServerDisconnected):
+ emailer.send_mime_email("from", "to", MIMEMultipart())
+ assert mock_smtp.call_count == 5 + 1
+
+ @mock.patch("smtplib.SMTP")
+ def test_send_without_core_smtp_section_uses_configured_values(
+ self, mock_smtp, core_without_smtp_section
+ ):
+ with conf_vars(
Review Comment:
**nit:** These values are fixed, so `conf_vars` should be a decorator here.
> Use `conf_vars` (from `tests_common.test_utils.config`) for Airflow config
overrides — as a decorator when the value is fixed, as a context manager when
it varies via `@pytest.mark.parametrize`.
([AGENTS.md, Testing
Standards](https://github.com/apache/airflow/blob/main/AGENTS.md#testing-standards))
--
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]
