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 <bdbr...@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.ma...@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.ma...@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 > >>> > >>> > > > >
