Mailing-List: contact dev-help@airflow.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@airflow.incubator.apache.org
MIME-Version: 1.0
In-Reply-To: <CACZXaYv7-VK9T081hCQ_zSPrgDjZ9Ljc5KfQLza=aEyK0hq8Uw@mail.gmail.com>
References: <CAGeMxTfuzHDysuFe03x25G7tSB6zms_KBU3Fni1UNJP1_X65tw@mail.gmail.com>
 <CAGeMxTcqVfAm4hwFJLZ2c9UNmnARo8GRFFZ7c0Yr_s2zYjG+5w@mail.gmail.com>
 <2B5CDACF-4281-4D93-AD8B-754FC49F3434@firemirror.com> <FEAE483C-090C-4103-A88D-E0A7C8470E17@gmail.com>
 <CACZXaYv7-VK9T081hCQ_zSPrgDjZ9Ljc5KfQLza=aEyK0hq8Uw@mail.gmail.com>
From: Chris Riccomini <criccomini@apache.org>
Date: Wed, 25 Oct 2017 09:47:47 -0700
Message-ID: <CABYbY7cn2Aeh78ViodkJdVwV+pg6UWEwsRLZELwYWcPydP=vdA@mail.gmail.com>
Subject: Re: ImportError: No module named sendgrid
To: dev@airflow.incubator.apache.org
Cc: sumeet.manit@gmail.com, Bolke de Bruin <bdbruin@gmail.com>
Content-Type: multipart/alternative; boundary="001a113b06daca8c58055c61d01b"
archived-at: Wed, 25 Oct 2017 16:47:58 -0000

--001a113b06daca8c58055c61d01b
Content-Type: text/plain; charset="UTF-8"

Let's do it as a plugin.

I merged it.

On Wed, Oct 25, 2017 at 9:20 AM, Feng Lu <fenglu@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 <bdbruin@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.manit@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.manit@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
> > >>>
> > >>>
> > >
> >
> >
>

--001a113b06daca8c58055c61d01b--