Feng, IMO the correct place could be under *airflow/contrib/plugins *or at *airflow/contrib/utils/sendgrid.py. *Also instead of keeping sendgrid api key in the config, it should be kept as a connection.
On Thu, Oct 26, 2017 at 4:30 AM, Feng Lu <fen...@google.com> wrote: > Understand, thanks for the explanation ;) > > More than happy to make a PR to revert and move it to plugins. > Does this look like the right place? https://github.com/ > apache/incubator-airflow/tree/master/airflow/contrib/plugins > Or should we keep it to ourselves based on https://airflow.apache.org/ > plugins.html? > > On Wed, Oct 25, 2017 at 11:02 AM, Sumit Maheshwari <sumeet.ma...@gmail.com > > wrote: > >> 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 >>>> > >>> >>>> > >>> >>>> > > >>>> > >>>> > >>>> >>> >>> >> >
