-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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-1686] Write list_objects, delete_object, and tests for boto3 s3_hook #2716
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2716 +/- ##
==========================================
+ Coverage 73% 73.12% +0.11%
==========================================
Files 156 156
Lines 11889 11899 +10
==========================================
+ Hits 8680 8701 +21
+ Misses 3209 3198 -11
Continue to review full report at Codecov.
|
Please review your commit message. |
ff73ed2
to
6b3db74
Compare
@bolkedebruin Updated commit message, please review when you get a chance! |
Note, this is one of the follow-ups to #2532 |
airflow/hooks/S3_hook.py
Outdated
Delimiter=delimiter) | ||
return [p.Prefix for p in response['CommonPrefixes']] if response.get('CommonPrefixes') else None | ||
|
||
def list_objects_v2(self, bucket_name, prefix='', delimiter=''): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a great method name, and the docs don't make it clear when to use this over otherlist_methods.
Do we need a separate method that just calls the underlying method? There's nothing to stop the callers doing s3_hook.get_conn().list_objects_v2
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original reasoning for this here was for use in conjunction with an operator that I have, which utilize this method directly, in which case having it here and well-tested would be important to ensure sustainability of the operator. Since the operator is out of the scope of the PR, perhaps I should remove the method & test here and reserve it for a later PR if it's usage is more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment was mainly around the function name, specifically the _v2
suffix. That and the docs don't make it clear the difference between this and prefixes, keys.
It is probably a useful addition still.
Adds additional functionality for s3_hook, using boto3 library. List_objects and delete_object methods are useful for the common cleanup task of removing older files in an s3 bucket. Also adds the s3_hook test file, with corresponding tests for the additions.
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
Tests
Commits