Let's do it as a plugin. I merged it.
On Wed, Oct 25, 2017 at 9:20 AM, Feng Lu <[email protected]> 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 <[email protected]> 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 <[email protected]> > > 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 < > > [email protected]> > > >> 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 > > >>> > > >>> > > > > > > > >
