airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Feng Lu <fen...@google.com.INVALID>
Subject Re: ImportError: No module named sendgrid
Date Wed, 25 Oct 2017 23:00:23 GMT
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.manit@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 <criccomini@apache.org>
> wrote:
>
>> 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