Skip to content

Conversation

@ipeluffo
Copy link
Contributor

This PR adds a new hook for Amazon Simple Email Service (SES).


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

@boring-cyborg boring-cyborg bot added the provider:amazon AWS/Amazon - related issues label Jul 25, 2020
@mik-laj
Copy link
Member

mik-laj commented Jul 25, 2020

Are you interested to add email backend for SES? This will allow a similar goal to be achieved with more flexibility.
https://airflow.readthedocs.io/en/latest/howto/email-config.html

@ipeluffo
Copy link
Contributor Author

ipeluffo commented Jul 26, 2020

Are you interested to add email backend for SES? This will allow a similar goal to be achieved with more flexibility.
https://airflow.readthedocs.io/en/latest/howto/email-config.html

Hi @mik-laj , does implementing that means I should close this PR or I should create a new one or add that functionality within this PR?
I'm not familiar with the email backend implementation so I don't know how much work that requires, I'll try to have a look and evaluates if I can work on a solution in a reasonable amount of time.
Thanks

@mik-laj
Copy link
Member

mik-laj commented Jul 26, 2020

All solutions are correct :-D

@ipeluffo
Copy link
Contributor Author

All solutions are correct :-D

Cool, if you don't mind, probably I'd like to merge this PR first and make another PR later to keep both features separated.

@ipeluffo ipeluffo marked this pull request as ready for review July 26, 2020 22:56
@JeffryMAC
Copy link

@feluelle @mik-laj can review ? This hook will be very useful to my team

@feluelle
Copy link
Member

feluelle commented Aug 6, 2020

Cool, if you don't mind, probably I'd like to merge this PR first and make another PR later to keep both features separated.

SGTM 👍

Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks very good to me :)

Only a minor suggestion I have.

@feluelle feluelle merged commit f06fe61 into apache:master Aug 10, 2020
@potiuk
Copy link
Member

potiuk commented Aug 10, 2020

I am reverting this one as it broke master. Backport Packages were failing and it was merged by mistake. @ipeluffo Can you please re-do the ticket and fix the failing import ;)

potiuk added a commit to PolideaInternal/airflow that referenced this pull request Aug 10, 2020
potiuk added a commit that referenced this pull request Aug 10, 2020
@potiuk
Copy link
Member

potiuk commented Aug 10, 2020

Reverted. Please re-open @ipeluffo and re-submit the PR as a new one. We want to have the SES hook in :)

@feluelle
Copy link
Member

I am sorry @ipeluffo - my fault. Thanks Jarek.

@potiuk
Copy link
Member

potiuk commented Aug 10, 2020

No worries @feluelle :). It happens. I think it does not help that we might have the quarantined build fail occasionally and we still merge the changes, this makes us into the uncomfortable habit of merging not-entrly green build. I hope in a few weeks we will handle the quarantined tests and squash them (Still gathering stats) and eventually we will even disable the merge of non-fully green builds (or at least think hard before doing it).

@potiuk
Copy link
Member

potiuk commented Aug 13, 2020

Hey @ipeluffo - do you want to reopen this one ? We are about to release 1.10.12 and it will unblock Backport Package release #10014 so I am likey to work on it early next week. I know you wanted to make sure that the new backport packages include this new hook, so better re-adding it this week :)

@JeffryMAC
Copy link

@ipeluffo ping :-) can you PR this again?

@ipeluffo
Copy link
Contributor Author

Sorry for the delay responding, I've been off for a few days. I'll try to make a new PR between today and tomorrow morning. Thanks

@ipeluffo
Copy link
Contributor Author

New PR created: #10391

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

Labels

provider:amazon AWS/Amazon - related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants