[AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments #4475

feluelle · 2019-01-10T17:56:41Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3602
- In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.

Description

Here are some details about my PR, including screenshots of any UI changes:

This PR changes the ImapHook so it will fail when no attachments have been found.
It also:

fixes escaping chars and symlink tests not testing correctly
adds note for handling symlinks

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

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 (not including Jira issue reference)
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"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

Passes flake8

feluelle · 2019-01-12T11:04:58Z

PTAL @kaxil I would really like to have this in 1.10.2 if possible. Or what do you actually think of this PR?

kaxil · 2019-01-12T13:21:38Z

I think we can raise a warning but we shouldn't raise an exception.

feluelle · 2019-01-12T14:37:42Z

But then the task won't fail if there is no attachment.
And how do you know then if, for example, the transfer for this attachment works. I don't think it belongs to the transfer operator to check whether there is an attachment or not.

feluelle · 2019-01-14T21:28:27Z

It is similar to the S3Hook if you want to download a file from s3 you get an exception when there isn't a file with the given name. I think the same goes for a mail server and downloading its mail attachments. If you want to be sure that there is an attachment with the given name you can run a sensor task (ImapAttachmentSensor) before the actual download.

feluelle · 2019-01-16T09:13:37Z

@Fokko @feng-tao @ashb what do you guys think of this?

codecov-io · 2019-01-16T10:04:25Z

Codecov Report

Merging #4475 into master will increase coverage by 63%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4475      +/-   ##
==========================================
+ Coverage   11.03%   74.03%     +63%     
==========================================
  Files         421      421              
  Lines       27652    27656       +4     
==========================================
+ Hits         3051    20475   +17424     
+ Misses      24601     7181   -17420

Impacted Files	Coverage Δ
airflow/contrib/hooks/imap_hook.py	`100% <100%> (+100%)`	⬆️
airflow/plugins_manager.py	`86.66% <0%> (+0.95%)`	⬆️
airflow/executors/dask_executor.py	`2% <0%> (+2%)`	⬆️
airflow/exceptions.py	`100% <0%> (+2.85%)`	⬆️
airflow/utils/operator_resources.py	`86.95% <0%> (+4.34%)`	⬆️
...etes_request_factory/kubernetes_request_factory.py	`72.82% <0%> (+5.43%)`	⬆️
airflow/executors/__init__.py	`63.46% <0%> (+5.76%)`	⬆️
airflow/utils/module_loading.py	`100% <0%> (+10.52%)`	⬆️
airflow/utils/decorators.py	`91.66% <0%> (+14.58%)`	⬆️
airflow/settings.py	`80.28% <0%> (+14.78%)`	⬆️
... and 323 more

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 a7e369f...d48a96b. Read the comment docs.

codecov-io · 2019-01-16T10:04:25Z

Codecov Report

Merging #4475 into master will increase coverage by 0.59%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #4475      +/-   ##
==========================================
+ Coverage   73.54%   74.14%   +0.59%     
==========================================
  Files         421      421              
  Lines       27724    27736      +12     
==========================================
+ Hits        20389    20564     +175     
+ Misses       7335     7172     -163

Impacted Files	Coverage Δ
airflow/contrib/hooks/imap_hook.py	`95.41% <75%> (-2.53%)`	⬇️
...etes_request_factory/kubernetes_request_factory.py	`72.82% <0%> (+4.34%)`	⬆️
airflow/configuration.py	`92.08% <0%> (+5.03%)`	⬆️
.../kubernetes_request_factory/pod_request_factory.py	`100% <0%> (+39.28%)`	⬆️
...rflow/contrib/operators/kubernetes_pod_operator.py	`100% <0%> (+40.54%)`	⬆️
airflow/contrib/kubernetes/kube_client.py	`75% <0%> (+41.66%)`	⬆️
airflow/contrib/kubernetes/pod_generator.py	`86.48% <0%> (+43.24%)`	⬆️
airflow/contrib/kubernetes/volume.py	`100% <0%> (+50%)`	⬆️
airflow/contrib/kubernetes/pod_launcher.py	`90.08% <0%> (+52.89%)`	⬆️
... and 1 more

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 92727f7...764d3a0. Read the comment docs.

ashb · 2019-01-16T12:12:49Z

My first though is that the Hook should probably just return [] (no attacments) and this sort of error handling/exception should be thrown from the Operator.

The S3 case is different - getting the attachments of a message is closer to listing keys (which doesn't throw any exceptions, it returns an empty list) than getting a specific file.

feluelle · 2019-01-16T14:35:06Z

The S3 case is different - getting the attachments of a message is closer to listing keys (which doesn't throw any exceptions, it returns an empty list) than getting a specific file.

Is it really? But the download_mail_attachments actually also raises no exception atm because to create files from the attachments it loops through the attachments that have been found. So also this task will succeed even it hasn't downloaded anything.

ashb · 2019-01-18T16:28:53Z

$ mkdir tmp
$ ls tmp/
$ echo $?
0

feluelle · 2019-01-22T08:50:21Z

Hey @ashb & @kaxil,

please take a look once more. I changed it so you are able to specify what should happen when there is no attachment with the given name.

airflow/contrib/hooks/imap_hook.py

- fixes escaping chars and symlink tests not testing correctly - adds note for handling symlinks [AIRFLOW-3602] Changes ImapHook handling of retrieving no attachments NOTE: You can now specify what should happen when retrieving or downloading a non-existing mail attachment - update docs and remove unnecessary default value descriptions - reformatting [AIRFLOW-3602] Add silent mode

ashb · 2019-01-23T13:46:08Z

Waiting for green tests then will merge

kaxil · 2019-01-25T21:22:31Z

Thanks @feluelle

…#4475)

…apache#4475)

feluelle force-pushed the feature/AIRFLOW-3602-imap-hook-raise-exception-on-no-attachments branch from 6db3ac7 to d48a96b Compare January 16, 2019 09:19

feluelle mentioned this pull request Jan 16, 2019

[AIRFLOW-3552] Add ImapAttachmetToS3Operator #4476

Merged

6 tasks

feluelle force-pushed the feature/AIRFLOW-3602-imap-hook-raise-exception-on-no-attachments branch from 2b4e682 to 93bbc1c Compare January 22, 2019 08:43

feluelle changed the title ~~[AIRFLOW-3602] Changes ImapHook to raise an exception on no attachments~~ [AIRFLOW-3602] Changes ImapHook handling of retrieving no attachments Jan 22, 2019

ashb reviewed Jan 22, 2019

View reviewed changes

airflow/contrib/hooks/imap_hook.py Outdated Show resolved Hide resolved

feluelle force-pushed the feature/AIRFLOW-3602-imap-hook-raise-exception-on-no-attachments branch from 93bbc1c to ef43c4f Compare January 23, 2019 13:21

feluelle force-pushed the feature/AIRFLOW-3602-imap-hook-raise-exception-on-no-attachments branch from ef43c4f to 764d3a0 Compare January 23, 2019 13:34

ashb approved these changes Jan 23, 2019

View reviewed changes

ashb changed the title ~~[AIRFLOW-3602] Changes ImapHook handling of retrieving no attachments~~ [AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments Jan 23, 2019

kaxil merged this pull request into apache:master Jan 25, 2019

kaxil pushed a commit that referenced this pull request Jan 25, 2019

[AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments (…

aed71a7

…#4475)

ashb pushed a commit to ashb/airflow that referenced this pull request Feb 2, 2019

[AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments (…

b07ed86

…apache#4475)

feluelle deleted the feature/AIRFLOW-3602-imap-hook-raise-exception-on-no-attachments branch April 9, 2019 12:10

ashb mentioned this pull request Jun 18, 2019

AIRFLOW-4809 | s3_delete_objects_operator should not fail on empty list of keys #5428

Closed

6 tasks

wmorris75 pushed a commit to modmed-external/incubator-airflow that referenced this pull request Jul 29, 2019

[AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments (…

cd6b232

…apache#4475)

[AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments #4475

[AIRFLOW-3602] Improve ImapHook handling of retrieving no attachments #4475

Uh oh!

Conversation

feluelle commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

feluelle commented Jan 12, 2019

Uh oh!

kaxil commented Jan 12, 2019

Uh oh!

feluelle commented Jan 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feluelle commented Jan 14, 2019

Uh oh!

feluelle commented Jan 16, 2019

Uh oh!

codecov-io commented Jan 16, 2019

Codecov Report

Uh oh!

codecov-io commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ashb commented Jan 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

feluelle commented Jan 16, 2019

Uh oh!

ashb commented Jan 18, 2019

Uh oh!

feluelle commented Jan 22, 2019

Uh oh!

Uh oh!

ashb commented Jan 23, 2019

Uh oh!

kaxil commented Jan 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feluelle commented Jan 10, 2019 •

edited

Loading

feluelle commented Jan 12, 2019 •

edited

Loading

codecov-io commented Jan 16, 2019 •

edited

Loading

ashb commented Jan 16, 2019 •

edited

Loading