fix stmp email callback to use email_conn_id#65072
Open
wjddn279 wants to merge 1 commit intoapache:mainfrom
Open
fix stmp email callback to use email_conn_id#65072wjddn279 wants to merge 1 commit intoapache:mainfrom
wjddn279 wants to merge 1 commit intoapache:mainfrom
Conversation
wjddn279
requested review from
amoghrajesh,
ashb,
hussein-awala and
kaxil
as code owners
henry3260
approved these changes
Apr 14, 2026
Contributor
henry3260
left a comment
henry3260
left a comment
There was a problem hiding this comment.
Thanks for the PR! LGTM!
Contributor
Author
|
Can you check this pr? and this question related issue |
amoghrajesh
reviewed
Apr 17, 2026
Contributor
amoghrajesh
left a comment
amoghrajesh
left a comment
There was a problem hiding this comment.
It's a real bug, thanks for solving this.
I have some comments.
| super().__init__(**kwargs) | ||
| else: | ||
| super().__init__() | ||
| self.smtp_conn_id = smtp_conn_id |
Contributor
There was a problem hiding this comment.
Can we instead do this during instantiation time? Like this
Suggested change
| self.smtp_conn_id = smtp_conn_id or conf.get("email", "email_conn_id", fallback=SmtpHook.default_conn_name) |
| mime_charset: str = "utf-8", | ||
| custom_headers: dict[str, Any] | None = None, | ||
| smtp_conn_id: str = SmtpHook.default_conn_name, | ||
| smtp_conn_id: str = conf.get("email", "email_conn_id", fallback=SmtpHook.default_conn_name), |
Contributor
There was a problem hiding this comment.
Suggested change
| smtp_conn_id: str = conf.get("email", "email_conn_id", fallback=SmtpHook.default_conn_name), | |
| smtp_conn_id: str | None = None, |
Comment on lines
+226
to
+245
| def test_notifier_default_smtp_conn_id_from_config(self): | ||
| """Test that smtp_conn_id defaults to email.email_conn_id from config.""" | ||
| import importlib | ||
|
|
||
| import airflow.providers.smtp.notifications.smtp as smtp_mod | ||
|
|
||
| from tests_common.test_utils.config import conf_vars | ||
|
|
||
| with conf_vars({("email", "email_conn_id"): "config_smtp_conn"}): | ||
| importlib.reload(smtp_mod) | ||
| try: | ||
| notifier = smtp_mod.SmtpNotifier( | ||
| to=TEST_RECEIVER, | ||
| from_email=TEST_SENDER, | ||
| subject=TEST_SUBJECT, | ||
| html_content=TEST_BODY, | ||
| ) | ||
| assert notifier.smtp_conn_id == "config_smtp_conn" | ||
| finally: | ||
| importlib.reload(smtp_mod) |
Contributor
There was a problem hiding this comment.
This test is masking the instantiation problem I mentioned earlier. You have to reload the module for things to take affect.
Instead, after the fix, the test would be as simple as:
def test_notifier_default_smtp_conn_id_from_config(self):
"""Test that smtp_conn_id defaults to email.email_conn_id from config."""
with conf_vars({("email", "email_conn_id"): "config_smtp_conn"}):
notifier = SmtpNotifier(
to=TEST_RECEIVER,
from_email=TEST_SENDER,
subject=TEST_SUBJECT,
html_content=TEST_BODY,
)
assert notifier.smtp_conn_id == "config_smtp_conn"| subject=subject, | ||
| html_content=html_content, | ||
| from_email=conf.get("email", "from_email", fallback="airflow@airflow"), | ||
| smtp_conn_id=conf.get("email", "email_conn_id", fallback=SmtpHook.default_conn_name), |
Contributor
There was a problem hiding this comment.
With my above comment, the default will be handled in provider code, so no need to pass fallback here?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes: #65012
Updated to read
email_conn_idas the default connection ID for SmtpHook, in order to maintain backward compatibility with the existing functionality.Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.