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 Thu, 26 Oct 2017 03:07:36 GMT
SGTM, thanks for the prompt reply!

On Wed, Oct 25, 2017 at 7:38 PM, Sumit Maheshwari <sumeet.manit@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 <fenglu@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.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