SGTM, thanks for the prompt reply! On Wed, Oct 25, 2017 at 7:38 PM, Sumit Maheshwari <sumeet.ma...@gmail.com> wrote:
> 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/apac >> he/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 >>>>> > >>> >>>>> > >>> >>>>> > > >>>>> > >>>>> > >>>>> >>>> >>>> >>> >> >
