-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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-5769] Move the S3_hook to /providers/aws/hooks #6443
Conversation
Can you keep backwards compatibility and add missing tests? |
Right now, I have no ideas for keeping backward compatibility with Python's case-sensitive nature. |
Ah! that is a good idea. I couldn't think about it. Thank you for help. |
You should also add a note in the UPDATING.MD file |
Ok. I'll do that. Thank you for guiding me 👍 |
@mik-laj I encountered a problem. Some file systems (including macOS's default one) are case-insensitive, so we can't have both So we should use another filename to separate that both. How do you think about that move the And they (aws-*-hooks) look similar to each other. class AWSAthenaHook(AwsHook):
"""
Interact with AWS Athena to run, poll queries and return query results
:param aws_conn_id: aws connection to use.
:type aws_conn_id: str
:param sleep_time: Time to wait between two consecutive call to check query status on athena
:type sleep_time: int
"""
INTERMEDIATE_STATES = ('QUEUED', 'RUNNING',)
FAILURE_STATES = ('FAILED', 'CANCELLED',)
SUCCESS_STATES = ('SUCCEEDED',)
... class S3Hook(AwsHook):
"""
Interact with AWS S3, using the boto3 library.
"""
def get_conn(self):
return self.get_client_type('s3') |
We want to move this hook to a completely different package. Please note that a change of class name should be done in a separate PR. |
Well, I will follow the mentioned conventions. (/providers/hooks/~.py) Yap. I’ll update this PR and send an additional PR for class name. |
89f9013
to
b5f112b
Compare
I'll update the (based on https://github.com/apache/airflow/pull/6446/files#diff-bb7ee25332d5fdca3ead2f545d72b359) |
b7f8903
to
3da5ac2
Compare
3da5ac2
to
b299d83
Compare
b299d83
to
63fbae0
Compare
63fbae0
to
02244ac
Compare
I updated the |
Codecov Report
@@ Coverage Diff @@
## master #6443 +/- ##
=========================================
- Coverage 83.88% 83.7% -0.18%
=========================================
Files 631 632 +1
Lines 36638 36642 +4
=========================================
- Hits 30732 30672 -60
- Misses 5906 5970 +64
Continue to review full report at Codecov.
|
And, Should I change the all import paths with new module path in the other files including test files? (in separate PR) |
@mik-laj Please check it |
Oh. I didn't notice your question. Yes. Other references to this class should be improved. |
Ok. I'll send a PR for that! |
Make sure you have checked all steps below.
Jira
Description
Tests
Commits
Documentation
Of course, this update will lead to the API breaking change. Therefore, all codes using
S3_hook
should change the import module names when this PR is merged.So if you agree with this change, it would be better to include these updates in Airflow 2.0 rather than 1.x because it is kind of specification change.