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-5803] Update S3Hook import paths [AIP-21] #6465

Merged
merged 5 commits into from
Nov 9, 2019

Conversation

mingrammer
Copy link
Contributor

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-5803
    • 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.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

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

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.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

@mik-laj
Copy link
Member

mik-laj commented Oct 29, 2019

Why did you add the AWS prefix? This is not in line with GCP operator naming guidelines. We have no general rules, so we can use the GCP if they make sense.

Maybe you would like to help with creating a document describing the general principles of naming operators. I will gladly add my comments. A spreadsheet with planned changes would also be helpful. I have these for GCP changes.

CC: @BasPH @potiuk

https://docs.google.com/document/d/1_rTdJSLCt0eyrAylmmgYc3yZr-_h51fVlnvMmWqhCkY/edit?ts=5bb72dfd#

@mik-laj mik-laj added provider:amazon-aws AWS/Amazon - related issues AIP-21 labels Oct 29, 2019
@mingrammer
Copy link
Contributor Author

mingrammer commented Oct 29, 2019

As I mentioned here #6455 (comment), I just followed the current class naming conventions for other AWS operators/hooks.

So I'm a bit confused about which conventions I should follow.

@mik-laj
Copy link
Member

mik-laj commented Oct 29, 2019

I am not sure of the correct solution. Can you write about this problem on mailing list? I don't think there has been an official discussion yet about ordering class names for AWS. The only document that describes it is AIP-21

Case #6 Other isolated cases
There are other random cases of inconsistencies in the naming of classes. It is necessary to review the list of all classes and prepare a plan of change. Support from major cloud service providers will be useful.

My team has support from Google, so we have a plan that was presented in the integration guide.

@mingrammer
Copy link
Contributor Author

mingrammer commented Oct 30, 2019

@mingrammer mingrammer changed the title [AIRFLOW-5803] Rename S3Hook to AWSS3Hook and update import paths [AIRFLOW-5803] Rename S3Hook to AWSS3Hook and update import paths [AIP-21] Oct 30, 2019
@mingrammer
Copy link
Contributor Author

mingrammer commented Oct 31, 2019

According to some threads on the above mail (all the conventions were proposed by @BasPH), I think we should keep the name S3Hook as is.

So I'll fix the AWSS3Hook back to S3Hook, and will update all its import paths.

@mingrammer mingrammer changed the title [AIRFLOW-5803] Rename S3Hook to AWSS3Hook and update import paths [AIP-21] [AIRFLOW-5803] Update S3Hook import paths [AIP-21] Nov 5, 2019
@mingrammer
Copy link
Contributor Author

Done.

@codecov-io
Copy link

Codecov Report

Merging #6465 into master will decrease coverage by 0.62%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6465      +/-   ##
==========================================
- Coverage   84.02%   83.39%   -0.63%     
==========================================
  Files         632      635       +3     
  Lines       36640    36679      +39     
==========================================
- Hits        30787    30590     -197     
- Misses       5853     6089     +236
Impacted Files Coverage Δ
airflow/contrib/hooks/wasb_hook.py 100% <ø> (ø) ⬆️
airflow/sensors/s3_key_sensor.py 100% <100%> (ø) ⬆️
airflow/contrib/operators/s3_to_gcs_operator.py 86.44% <100%> (ø) ⬆️
airflow/sensors/s3_prefix_sensor.py 100% <100%> (ø) ⬆️
...rflow/contrib/operators/s3_copy_object_operator.py 100% <100%> (ø) ⬆️
airflow/operators/google_api_to_s3_transfer.py 100% <100%> (ø) ⬆️
airflow/operators/gcs_to_s3.py 94.73% <100%> (ø) ⬆️
airflow/operators/redshift_to_s3_operator.py 97.14% <100%> (ø) ⬆️
...ontrib/operators/imap_attachment_to_s3_operator.py 100% <100%> (ø) ⬆️
airflow/operators/s3_to_redshift_operator.py 100% <100%> (ø) ⬆️
... and 65 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 103dc92...e391766. Read the comment docs.

@mingrammer
Copy link
Contributor Author

mingrammer commented Nov 7, 2019

By the final voting result, It seems to have to move S3Hook into /providers/amazon/aws.

Should I update it with an another PR?

https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-21%3A+Changes+in+import+paths#AIP-21:Changesinimportpaths-Votingresults(seevotingbelow)

@potiuk potiuk merged commit 2daf72e into apache:master Nov 9, 2019
@potiuk
Copy link
Member

potiuk commented Nov 9, 2019

Ah. My bad @mingrammer. I was too fast. Can you please make another one to move to amazon.aws ?

@potiuk
Copy link
Member

potiuk commented Nov 9, 2019

I actually had to revert it as it was failing without rebase. Can you please rebase/reopen and add amazon package?

@mingrammer
Copy link
Contributor Author

Sure, I'll do!

@dazza-codes
Copy link
Contributor

Related PR in #6764 for batch operator, so I need to follow all this API-21 stuff in a few places (although I'd prefer to just merge it and have the subsequent API-21 stuff applied in one larger PR, but looks like it's already piecemeal).

@ashb
Copy link
Member

ashb commented Dec 10, 2019

Related PR in #6764 for batch operator, so I need to follow all this API-21 stuff in a few places (although I'd prefer to just merge it and have the subsequent API-21 stuff applied in one larger PR, but looks like it's already piecemeal).

If you want to do one large rename PR I'm all for it - ping me if you want a reviewer on it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants