Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-1723] make sendgrid a plugin #2727

Closed
wants to merge 1 commit into from

Conversation

fenglu-db
Copy link
Contributor

@fenglu-db fenglu-db commented Oct 26, 2017

Dear Airflow maintainers,

Please accept this PR. I understand that it will not be reviewed until I have checked off all the steps below!

JIRA

Description

  • Here are some details about my PR, including screenshots of any UI changes:
    Add sendgrid email support as a plugin (instead of core component).

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    tests.contrib.utils.test_sendgrid

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

@fenglu-db
Copy link
Contributor Author

@msumit @criccomini PTAL. Re: sendgrid config, I am neutral to environment var or a new sendgrid connection type, kindly let me know which one would be preferred.

@codecov-io
Copy link

codecov-io commented Oct 26, 2017

Codecov Report

Merging #2727 into master will increase coverage by 0.19%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2727      +/-   ##
==========================================
+ Coverage   72.46%   72.65%   +0.19%     
==========================================
  Files         154      154              
  Lines       11836    11824      -12     
==========================================
+ Hits         8577     8591      +14     
+ Misses       3259     3233      -26
Impacted Files Coverage Δ
airflow/utils/email.py 97.4% <ø> (+13.14%) ⬆️
airflow/logging_config.py 100% <0%> (ø) ⬆️
airflow/jobs.py 79.66% <0%> (+0.44%) ⬆️
airflow/utils/helpers.py 56.32% <0%> (+2.87%) ⬆️
airflow/task_runner/bash_task_runner.py 100% <0%> (+6.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2abead7...83a084a. Read the comment docs.

@bolkedebruin
Copy link
Contributor

can you add some documentation on how to use it?

response = sg.client.mail.send.post(request_body=mail_data)
# 2xx status code.
if response.status_code >= 200 and response.status_code < 300:
log.info('The following email with subject %s is successfully sent to sendgrid.' % subject)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it makes sense, but instead of saying that successfully sent to sendgrid can we say successfully sent to following recipients <recipients>?

@msumit
Copy link
Contributor

msumit commented Oct 26, 2017

@fenglu-g I am fine with the current env variable way. Also, +1 to Bolke's suggestion to provide example/documentation on how to use this backend.

@fenglu-db
Copy link
Contributor Author

fenglu-db commented Oct 27, 2017

@msumit revised log message with detailed recipient information, PTAL.
@bolkedebruin added usage info in the method doc string, PTAL.

Here's how a local integration test look like

airflow test tutorial email 20171025
/usr/local/lib/python2.7/site-packages/airflow/configuration.py:540: DeprecationWarning: This method will be removed in future versions. Use 'parser.read_file()' instead.
self.readfp(StringIO.StringIO(string))
[2017-10-27 06:12:02,652] {init.py:57} INFO - Using executor CeleryExecutor
/usr/local/lib/python2.7/site-packages/airflow/www/app.py:23: FlaskWTFDeprecationWarning: "flask_wtf.CsrfProtect" has been renamed to "CSRFProtect" and will be removed in 1.0.
csrf = CsrfProtect()
[2017-10-27 06:12:03,471] {models.py:168} INFO - Filling up the DagBag from /home/airflow/gcs/dags
[2017-10-27 06:12:03,836] {models.py:1128} INFO - Dependencies all met for <TaskInstance: tutorial.email 2017-10-25 00:00:00 [failed]>
[2017-10-27 06:12:03,846] {models.py:1128} INFO - Dependencies all met for <TaskInstance: tutorial.email 2017-10-25 00:00:00 [failed]>
[2017-10-27 06:12:03,846] {models.py:1334} INFO -

Starting attempt 1 of 2

[2017-10-27 06:12:03,847] {models.py:1358} INFO - Executing <Task(EmailOperator): email> on 2017-10-25 00:00:00
[2017-10-27 06:12:04,232] {sendgrid.py:82} INFO - Email with subject hello test is successfully sent to recipients: [{'cc': [{'email': 'xxx@xxx'}], 'to': [{'email': 'xxx@xxx'}], 'bcc': [{'email': 'xxx@xxx'}]}]

email_backend = airflow.contrib.utils.sendgrid.send_email
2. configure Sendgrid specific environment variables at all Airflow instances:
SENDGRID_MAIL_FROM={your-mail-from}
SENDGRID_API_KEY={your-sendgrid-api-key}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe a 0th bullet abt installing sendgrid module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

@msumit
Copy link
Contributor

msumit commented Oct 27, 2017

lgtm

@criccomini
Copy link
Contributor

@fenglu-g can you have a look at the travis failure?

@fenglu-db
Copy link
Contributor Author

fenglu-db commented Oct 27, 2017

@criccomini could be this be travis flakiness?

It failed on one of the settings with the following error message:
airflow.exceptions.AirflowException: dag_id could not be found: SchedulerJobTest.test_execute_task_instances_limit. Either the dag did not exist or it failed to parse.
./run_unit_tests.sh: line 82: 9719 Segmentation fault (core dumped) nosetests $nose_args
ERROR: InvocationError: '/home/travis/build/apache/incubator-airflow/scripts/ci/run_tests.sh'
___________________________________ summary ____________________________________
ERROR: py34-cdh-airflow_backend_postgres: commands failed

Link below: https://travis-ci.org/apache/incubator-airflow/builds/293688713?utm_source=github_status&utm_medium=notification.

@fenglu-db fenglu-db closed this Oct 27, 2017
@fenglu-db
Copy link
Contributor Author

Accidentally closed this PR, re-opened.

@fenglu-db fenglu-db reopened this Oct 27, 2017
@fenglu-db
Copy link
Contributor Author

@criccomini travis run looks OK now.

@criccomini
Copy link
Contributor

LGTM +1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants