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 <[email protected]> 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 <[email protected]> > wrote: > >> Let's do it as a plugin. >> >> I merged it. >> >> On Wed, Oct 25, 2017 at 9:20 AM, Feng Lu <[email protected]> >> 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 <[email protected]> >>> 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 <[email protected]> >>> > 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 < >>> > [email protected]> >>> > >> 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 >>> > >>> >>> > >>> >>> > > >>> > >>> > >>> >> >> >
