airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chris Riccomini <criccom...@apache.org>
Subject Re: ImportError: No module named sendgrid
Date Wed, 25 Oct 2017 16:47:47 GMT
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
> > >>>
> > >>>
> > >
> >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message