airflow-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sumit Maheshwari <sumeet.ma...@gmail.com>
Subject Re: ImportError: No module named sendgrid
Date Thu, 26 Oct 2017 02:38:17 GMT
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/
> 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