[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r737502743
##
File path: tests/providers/sendgrid/utils/test_emailer.py
##
@@ -64,6 +64,12 @@ def setUp(self):
'name': 'Foo Bar',
'email': 'f...@foo.bar',
}
+# sender from conf
+self.expected_mail_data_conf_sender =
copy.deepcopy(self.expected_mail_data)
+self.expected_mail_data_conf_sender['from'] = {
+'name': 'Foo Conf',
+'email': 'f...@conf.com',
+}
Review comment:
This is not used at all?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r737501347
##
File path: tests/providers/amazon/aws/utils/test_emailer.py
##
@@ -16,27 +16,32 @@
# specific language governing permissions and limitations
# under the License.
#
-
-from unittest import mock
+from unittest import TestCase, mock
from airflow.providers.amazon.aws.utils.emailer import send_email
-@mock.patch("airflow.providers.amazon.aws.utils.emailer.SESHook")
-def test_send_email(mock_hook):
-send_email(
-to="t...@test.com",
-subject="subject",
-html_content="content",
-)
-mock_hook.return_value.send_email.assert_called_once_with(
-mail_from=None,
-to="t...@test.com",
-subject="subject",
-html_content="content",
-bcc=None,
-cc=None,
-files=None,
-mime_charset="utf-8",
-mime_subtype="mixed",
-)
+class TestSendEmailSes(TestCase):
+def setUp(self):
+pass
Review comment:
Why is this class needed if there's no setup and teardown needed
whatsoever?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r731411083 ## File path: airflow/config_templates/config.yml ## @@ -1358,6 +1358,14 @@ example: "/path/to/my_html_content_template_file" default: ~ see_also: ":doc:`Email Configuration `" +- name: from_email + description: | +Email address that will be used as sender address. +It can either be raw email or the complete address in a format ``Sender Name `` + version_added: 2.2.0 + type: string + example: "\"Airflow \"" Review comment: Also this unfortunately missed 2.2.0 so we need to change `version_added` to 2.3 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r731410907 ## File path: airflow/config_templates/config.yml ## @@ -1358,6 +1358,14 @@ example: "/path/to/my_html_content_template_file" default: ~ see_also: ":doc:`Email Configuration `" +- name: from_email + description: | +Email address that will be used as sender address. +It can either be raw email or the complete address in a format ``Sender Name `` + version_added: 2.2.0 + type: string + example: "\"Airflow \"" Review comment: ```suggestion example: "Airflow " ``` I don't think the extra quotes are needed? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r713571942
##
File path: airflow/utils/email.py
##
@@ -87,8 +91,10 @@ def send_email_smtp(
"""
smtp_mail_from = conf.get('smtp', 'SMTP_MAIL_FROM')
+mail_from = smtp_mail_from or from_email
Review comment:
Yes, but I don’t think it’s a bad thing since those functions are
fundamentally different. It also allows a new backend to choose to *not*
respect the `email_from` config if that makes sense.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r710852660
##
File path: airflow/utils/email.py
##
@@ -87,8 +91,10 @@ def send_email_smtp(
"""
smtp_mail_from = conf.get('smtp', 'SMTP_MAIL_FROM')
+mail_from = smtp_mail_from or from_email
Review comment:
If a user calls this function directly (not via `send_email`), I feel
they should still automatically get `FROM_EMAIL` if `SMTP_MAIL_FROM` is not
set, instead of needing to pass it in manually.
(This also reminds me, should we change the former to `EMAIL_FROM` instead
for consistency? Or maybe even `DEFAULT_FROM` or just `FROM`?)
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r710841797
##
File path: airflow/utils/email.py
##
@@ -87,8 +91,10 @@ def send_email_smtp(
"""
smtp_mail_from = conf.get('smtp', 'SMTP_MAIL_FROM')
+mail_from = smtp_mail_from or from_email
Review comment:
Should we automatically read the `from_email` configuration here as well?
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r710841062 ## File path: airflow/config_templates/config.yml ## @@ -1351,6 +1351,13 @@ example: "/path/to/my_html_content_template_file" default: ~ see_also: ":doc:`Email Configuration `" +- name: from_email + description: | +Email address that will be used as sender address. + version_added: 2.2.0 + type: string + example: "airf...@example.com" + default: ~ Review comment: We should mention this can be either just the “raw” email address, or the “complete” email address format, to avoid users getting confused. And the example should probably use something like `Airflow ` as well. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r710803944 ## File path: airflow/config_templates/config.yml ## @@ -1351,6 +1351,20 @@ example: "/path/to/my_html_content_template_file" default: ~ see_also: ":doc:`Email Configuration `" +- name: email_from_email + description: | +Email address that will be used as sender address. + version_added: 2.2.0 + type: string + example: "airf...@example.com" + default: ~ +- name: email_from_name Review comment: It doesn’t, Sendgrid accepts the `Display ` form natively, the `Email(addr, name)` format is for backward compatibility. https://github.com/sendgrid/sendgrid-python/blob/main/sendgrid/helpers/mail/email.py#L16-L40 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r710803944 ## File path: airflow/config_templates/config.yml ## @@ -1351,6 +1351,20 @@ example: "/path/to/my_html_content_template_file" default: ~ see_also: ":doc:`Email Configuration `" +- name: email_from_email + description: | +Email address that will be used as sender address. + version_added: 2.2.0 + type: string + example: "airf...@example.com" + default: ~ +- name: email_from_name Review comment: It doesn’t, Sendgrid accepts the `Display ` form natively, the `Email(addr, name)` format is actually for compatibility. https://github.com/sendgrid/sendgrid-python/blob/main/sendgrid/helpers/mail/email.py#L16-L40 -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r710774643 ## File path: airflow/config_templates/config.yml ## @@ -1351,6 +1351,20 @@ example: "/path/to/my_html_content_template_file" default: ~ see_also: ":doc:`Email Configuration `" +- name: email_from_email + description: | +Email address that will be used as sender address. + version_added: 2.2.0 + type: string + example: "airf...@example.com" + default: ~ +- name: email_from_name Review comment: Also, as I mentioned in the other PR, `from_name` is not needed at all since `from_email` can just be set to `Display Name `. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r703040821 ## File path: airflow/providers/amazon/aws/utils/emailer.py ## @@ -16,13 +16,15 @@ # specific language governing permissions and limitations # under the License. """Airflow module for email backend using AWS SES""" - +from email.utils import formataddr from typing import List, Optional, Union from airflow.providers.amazon.aws.hooks.ses import SESHook def send_email( +from_email: str, +from_name: str, Review comment: Cool. -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042: URL: https://github.com/apache/airflow/pull/18042#discussion_r702984017 ## File path: airflow/providers/amazon/aws/utils/emailer.py ## @@ -16,13 +16,15 @@ # specific language governing permissions and limitations # under the License. """Airflow module for email backend using AWS SES""" - +from email.utils import formataddr from typing import List, Optional, Union from airflow.providers.amazon.aws.hooks.ses import SESHook def send_email( +from_email: str, +from_name: str, Review comment: Not sure if we need to make these optional for backward compatibility. Does this function work before this PR? -- 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: commits-unsubscr...@airflow.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r702983479
##
File path: airflow/utils/email.py
##
@@ -49,10 +49,17 @@ def send_email(
"""Send email using backend specified in EMAIL_BACKEND."""
backend = conf.getimport('email', 'EMAIL_BACKEND')
backend_conn_id = conn_id or conf.get("email", "EMAIL_CONN_ID")
+from_email = (
+conf.get('email', 'email_from_email') if conf.has_option('email',
'email_from_email') else None
+)
+from_name = conf.get('email', 'email_from_name') if
conf.has_option('email', 'email_from_name') else None
Review comment:
I see. Since this is essentially the “last resort” I think using one
single value (pair) makes sense.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r702940430
##
File path: airflow/utils/email.py
##
@@ -49,10 +49,17 @@ def send_email(
"""Send email using backend specified in EMAIL_BACKEND."""
backend = conf.getimport('email', 'EMAIL_BACKEND')
backend_conn_id = conn_id or conf.get("email", "EMAIL_CONN_ID")
+from_email = (
+conf.get('email', 'email_from_email') if conf.has_option('email',
'email_from_email') else None
+)
+from_name = conf.get('email', 'email_from_name') if
conf.has_option('email', 'email_from_name') else None
Review comment:
IIRC the previous version added support for some config options under
`ses` and `sendgrid` (or something). Are those still respected?
##
File path: airflow/providers/sendgrid/utils/emailer.py
##
@@ -68,7 +68,9 @@ def send_email(
mail = Mail()
from_email = kwargs.get('from_email') or
os.environ.get('SENDGRID_MAIL_FROM')
+
from_name = kwargs.get('from_name') or
os.environ.get('SENDGRID_MAIL_SENDER')
+
Review comment:
Please revert these two lines so this file would be untouched.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r702753430
##
File path: airflow/utils/email.py
##
@@ -85,7 +85,18 @@ def send_email_smtp(
>>> send_email('t...@example.com', 'foo', 'Foo bar', ['/dev/null'],
dryrun=True)
"""
-smtp_mail_from = conf.get('smtp', 'SMTP_MAIL_FROM')
+from_email = kwargs.get('from_email') or os.environ.get('SMTP_MAIL_FROM')
+if not from_email and conf.has_option('smtp', 'smtp_mail_from'):
+from_email = conf.get('smtp', 'smtp_mail_from')
+
+from_name = kwargs.get('from_name') or os.environ.get('SMTP_MAIL_SENDER')
+if not from_name and conf.has_option('smtp', 'smtp_mail_sender'):
+from_name = conf.get('smtp', 'smtp_mail_sender')
+
+if from_email:
+smtp_mail_from = formataddr((from_name, from_email))
+else:
+smtp_mail_from = None
Review comment:
Sorry I was unclear. I was referring to this almost exact logic being
repeated here and in both the aws and sendgrid providers, and it may be better
to refactor them to put the logic into `build_mime_message` or a separate
function called in those places instead (by parametrizing the config keys).
Something like
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r702733006
##
File path: airflow/utils/email.py
##
@@ -85,7 +85,18 @@ def send_email_smtp(
>>> send_email('t...@example.com', 'foo', 'Foo bar', ['/dev/null'],
dryrun=True)
"""
-smtp_mail_from = conf.get('smtp', 'SMTP_MAIL_FROM')
+from_email = kwargs.get('from_email') or os.environ.get('SMTP_MAIL_FROM')
+if not from_email and conf.has_option('smtp', 'smtp_mail_from'):
+from_email = conf.get('smtp', 'smtp_mail_from')
+
+from_name = kwargs.get('from_name') or os.environ.get('SMTP_MAIL_SENDER')
+if not from_name and conf.has_option('smtp', 'smtp_mail_sender'):
+from_name = conf.get('smtp', 'smtp_mail_sender')
+
+if from_email:
+smtp_mail_from = formataddr((from_name, from_email))
+else:
+smtp_mail_from = None
Review comment:
This repetition seems wrong. Maybe we should only read conf here and
keep callers passin in `None`.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [airflow] uranusjr commented on a change in pull request #18042: Fixing ses email backend
uranusjr commented on a change in pull request #18042:
URL: https://github.com/apache/airflow/pull/18042#discussion_r702731319
##
File path: airflow/providers/amazon/aws/utils/emailer.py
##
@@ -35,9 +37,22 @@ def send_email(
**kwargs,
) -> None:
"""Email backend for SES."""
+from_email = kwargs.get('from_email') or os.environ.get('SES_MAIL_FROM')
+if not from_email and conf.has_option('ses', 'ses_mail_from'):
+from_email = conf.get('ses', 'ses_mail_from')
+
+from_name = kwargs.get('from_name') or os.environ.get('SES_MAIL_SENDER')
+if not from_name and conf.has_option('ses', 'ses_mail_sender'):
+from_name = conf.get('ses', 'ses_mail_sender')
+
+if from_email:
+mail_from = formataddr((from_name, from_email))
+else:
+mail_from = None
+
hook = SESHook(aws_conn_id=conn_id)
hook.send_email(
-mail_from=None,
+mail_from=mail_from,
Review comment:
Wouldn’t think have the exact same error if none of the above were set?
I feel we should raise an exception right here if `mail_from` is None so it’s
moer obvious to debug.
--
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: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
