Feng, genuinely I am not against Sendgrid or any other third party integration with Airflow. Infact we ourselves use Sendgrid for all mailing purposes. But as being part of an open source community, we must try to remain as neutral as possible.
I still remember of opposing integration of one particular SAML provider long long back, as there are 100s of such SAML providers out there, and we just can't let the config file filled with empty configurations settings. Anyway I am eagerly waiting to use sendgrid email backend :) On Wed, Oct 25, 2017 at 10:17 PM, Chris Riccomini <criccom...@apache.org> wrote: > Let's do it as a plugin. > > I merged it. > > On Wed, Oct 25, 2017 at 9:20 AM, Feng Lu <fen...@google.com.invalid> > wrote: > >> Sorry for the glitch. >> >> I am the author of this commit, I agree that if sendgrid is not used, it >> doesn't need to be installed. >> That being said, I can submit a quick fix to dynamically load this module. >> >> Re: core vs plugin, I feel it really depends on whether the sendgrid email >> integration is needed by >> users other than us. As I mentioned in the corresponding JIRA issue, the >> other email alternative >> in Airflow is SMTP, which requires username + password in plaintext (and >> these are not easily revokable). >> >> On the contrast, the current sendgrid integration in Airflow only needs an >> API key, which supports >> fine-grained permission control and can be easily revoked. >> >> >> >> On Wed, Oct 25, 2017 at 4:27 AM, Bolke de Bruin <bdbr...@gmail.com> >> wrote: >> >> > Code gets reviewed by a committer other than the author of the code. >> > Merging can happen by either. >> > >> > I also agree that the sendgrid dependency should be reverted and made >> into >> > a plugin instead. >> > >> > Bolke >> > >> > > On 25 Oct 2017, at 11:07, Ash Berlin-Taylor >> <ash_airflowlist@firemirror. >> > com> wrote: >> > > >> > > 1. PRs generally aren't "approved" in Github if a core contributor >> looks >> > at it and is happy >> > > 2. It was merged by criccomini, not the opener >> > https://github.com/apache/incubator-airflow/commit/ >> > 7cb818bbacb2a2695282471591a9e323d8efbf5c <https://github.com/apache/ >> > incubator-airflow/commit/7cb818bbacb2a2695282471591a9e323d8efbf5c> >> > > 3, 4, 5. I agree, and I made the same point >> https://issues.apache.org/ >> > jira/browse/AIRFLOW-1723 <https://issues.apache.org/ >> > jira/browse/AIRFLOW-1723> >> > > >> > > Either way this is a bug that it always tries to import >> unconditionally >> > and fails when the sendgrid mailer isn't used. >> > > >> > >> On 25 Oct 2017, at 09:48, Sumit Maheshwari <sumeet.ma...@gmail.com> >> > wrote: >> > >> >> > >> Sorry, mistakenly sent halfbaked: >> > >> >> > >> So, the concerns are: >> > >> >> > >> 1. Don't see any approver on the PR, neither +1s. >> > >> 2. The PR author and PR merging guy seem to be same, which I think is >> > >> against the general understanding. >> > >> 3. Why sendgrid got special privileges, and why not 100s of other >> mail >> > >> services? The Same concern was raised by Ash in JIRA as well. >> > >> 4. Why it was not coded as a plugin. >> > >> 5. Why there is a hard dependency to install Sendgrid module if I am >> not >> > >> using it. >> > >> >> > >> I think this commit needs to be reverted and a new PR should be >> raised, >> > >> which adds its as a plugin instead of a core feature. >> > >> >> > >> >> > >> >> > >> On Wed, Oct 25, 2017 at 2:12 PM, Sumit Maheshwari < >> > sumeet.ma...@gmail.com> >> > >> wrote: >> > >> >> > >>> When I fetched the latest master, installed it and ran webserver, it >> > >>> failed with this error: >> > >>> >> > >>> *ImportError: No module named sendgrid* >> > >>> >> > >>> On further investigation, I found that it has been introduced as >> part >> > of >> > >>> this PR <https://github.com/apache/incubator-airflow/pull/2695> and >> > this >> > >>> JIRA <https://issues.apache.org/jira/browse/AIRFLOW-1723>. >> > >>> Now couple of doubts: >> > >>> >> > >>> 1. >> > >>> >> > >>> >> > >>> >> > >>> >> > >>> Thanks, >> > >>> Sumit Maheshwari >> > >>> cell. 9632202950 >> > >>> >> > >>> >> > > >> > >> > >> > >
