Skip to content

Conversation

@ecerulm
Copy link
Contributor

@ecerulm ecerulm commented Sep 13, 2021

closes: #17930


^ 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.

@ecerulm ecerulm force-pushed the airflow-17930 branch 8 times, most recently from 5719b55 to 5151c03 Compare September 14, 2021 19:01
@ecerulm ecerulm requested a review from kaxil September 14, 2021 19:12
@uranusjr
Copy link
Member

This conficts with #18042. We should figure out a conhesive plan on what exactly to do here.

@ecerulm
Copy link
Contributor Author

ecerulm commented Sep 15, 2021

I didn't realized that someone else was working on a PR, I opened issue #17930 on 2021-08-31 and that issue was assigned to me by @potiuk so I when I got around to preparing this PR on 2021-09-13 I didn't even think of searching for other PRs. So feel free to close this one.

@uranusjr
Copy link
Member

I feel this PR is still valuable though, and the two PRs’ different implementations should be combined together into one cohesive solution. The previous PR (#18042) fills out the email field with the SMTP email config, which I’ve never been completely comfortable with, but the new email field introduced by this PR makes the Airflow config contain two distinct sets of email configurations, which would be very confusing to users. Perhaps the best way to combine the two would be to

  1. Add the new email_from configs (but I don’t think it’s necessary to have separate fields from_name nad from_email; the email field can already contain a display name by default, e.g. John Smith <john.smith@example.org>, so we only need one field (maybe with a better example and documentation so users know how to use it better).
  2. Make smtp_mail_from optional and default to the from_email setting; the user can still set smtp_mail_from if needed to override this default only for SMTP.
  3. Use email_from in the SES implementation.

This is only my initial thought and other ideas are definitely welcomed. @ecerulm and @ignaski you two can also discuss which one of you should take the lead on this one and /or how to split work between.

@ecerulm
Copy link
Contributor Author

ecerulm commented Sep 15, 2021

For me it's ok if @ignaski takes over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Emails notifications with AWS SES not working due to missing "from:" field

3 participants