-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fixing SES backend - source (mail_from) must be string. #16166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The Source in send_raw_email must be a String type. mail_from=None is failing the SES backend for airflow alerts.
Error -
File "/Users/username/Library/Python/3.8/lib/python/site-packages/airflow/models/taskinstance.py", line 1852, in email_alert
send_email(self.task.email, subject, html_content)
File "/Users/username/Library/Python/3.8/lib/python/site-packages/airflow/utils/email.py", line 52, in send_email
return backend(
File "/Users/username/Library/Python/3.8/lib/python/site-packages/airflow/providers/amazon/aws/utils/emailer.py", line 39, in send_email
hook.send_email(
File "/Users/username/Library/Python/3.8/lib/python/site-packages/airflow/providers/amazon/aws/hooks/ses.py", line 95, in send_email
return ses_client.send_raw_email(
File "/Users/username/Library/Python/3.8/lib/python/site-packages/botocore/client.py", line 357, in _api_call
return self._make_api_call(operation_name, kwargs)
File "/Users/username/Library/Python/3.8/lib/python/site-packages/botocore/client.py", line 648, in _make_api_call
request_dict = self._convert_to_request_dict(
File "/Users/username/Library/Python/3.8/lib/python/site-packages/botocore/client.py", line 696, in _convert_to_request_dict
request_dict = self._serializer.serialize_to_request(
File "/Users/username/Library/Python/3.8/lib/python/site-packages/botocore/validate.py", line 293, in serialize_to_request
raise ParamValidationError(report=report.generate_report())
botocore.exceptions.ParamValidationError: Parameter validation failed:
Invalid type for parameter Source, value: None, type: <class 'NoneType'>, valid types: <class 'str'>
Refer - https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ses.html#SES.Client.send_raw_email
| """Email backend for SES.""" | ||
| hook = SESHook(aws_conn_id=conn_id) | ||
|
|
||
| mail_from = conf.get('smtp', 'SMTP_MAIL_FROM') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please guide if this is right approach or should we update the EmailOperator to have input for "from".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks good. Can you add tests to avoid regression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mik-laj, I will update the Pr with tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@satishmittal111 Are you able to update this PR with tests?
|
I think we should just require the user to pass in |
While this is likely not perfect, I think it's better to make it consistent. This is what sendgrid uses currently: It allows for both kwargs and environment variables set, but with standard emails sent by airflow (alert/sla) the kwargs are not used anyway. However the documentation in https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/email-config.rst does not mention those env vars. It mentions few other '[email]' options but it does not mention any of the [smtp] options which are used by the standard "send_email_smtp" backend. So we have a bit of a mess to be honest. Proposal: How about making it consistent and update the documentation at the same time between the three backends. For all backends we could use: SMTP: SENDGRID: SES: And update the email documentation about it (also mentioning that you need the corresponding providers to use it? |
|
Hey @satishmittal111 - are you going to improve the PR ? I am preparing to release the next wave of providers and it would be great to get it merged before. |
|
hello? |
|
@potiuk Sorry for the late response. The proposal looks good. Changes -
I will be able to complete the PR by end of this week. Will it work for you? |
Sure. I plan to release providers before, but I can also be released separately/next time :) |
|
@potiuk @satishmittal111 @mik-laj Any updates here? I just ran into this issue. Seems like the fix is done? Any idea when this might be released? |
|
Not yet - seems that @satishmittal111 has to complete it yet :) |
|
This PR seems to be stuck :( |
Maybe you would like to take it over @ignaski ? Seems the solution is there, only some tests need to be added. |
|
Since we’re very close to 2.2 now, anyone interested please feel free to open a separate PR inheriting works in here. I’m also adding the pending-response label so @satishmittal111 please respond if you are able to finish this soon. |
I will try to. I've never contributed to an open source project, thus it will take time to setup and get familiar. |
|
Apologies. I was not able to take it further due to personal priority. Please take over the changes. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
|
Continuing in #18042. |
The Source in send_raw_email must be a String type. mail_from=None is failing the SES backend for airflow alerts.
Currently SES Backend is failing with error -
Refer - https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/ses.html#SES.Client.send_raw_email
Bug fixing for SES backend for the emails sent by Airflow.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.