[WIP][AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21] #6455

BasPH · 2019-10-28T21:21:46Z

Mostly moving around files. Most important change is merging airflow/contrib/operators/dynamodb_to_s3.py and airflow/contrib/operators/hive_to_dynamodb.py together into airflow/providers/aws/operators/dynamodb.py.

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-5777
- 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

…-21]

codecov-io · 2019-10-28T23:15:16Z

Codecov Report

❗ No coverage uploaded for pull request base (master@59b6d8b). Click here to learn what that means.
The diff coverage is 93.16%.

@@           Coverage Diff            @@
##             master   #6455   +/-   ##
========================================
  Coverage          ?   83.7%           
========================================
  Files             ?     633           
  Lines             ?   36646           
  Branches          ?       0           
========================================
  Hits              ?   30673           
  Misses            ?    5973           
  Partials          ?       0

Impacted Files	Coverage Δ
airflow/contrib/hooks/aws_dynamodb_hook.py	`100% <100%> (ø)`
airflow/contrib/operators/hive_to_dynamodb.py	`100% <100%> (ø)`
airflow/contrib/operators/dynamodb_to_s3.py	`100% <100%> (ø)`
airflow/providers/aws/hooks/dynamodb.py	`91.3% <91.3%> (ø)`
airflow/providers/aws/operators/dynamodb.py	`92.68% <92.68%> (ø)`

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 59b6d8b...c3893ee. Read the comment docs.

mik-laj · 2019-10-29T00:57:29Z

why did you transfer transfer operators to another file?

BasPH · 2019-10-29T08:22:39Z

Because it says this in AIP-21: "When there is only one provider as target but source is a database or another non-provider source, the operator is put to the target provider."

BasPH · 2019-10-29T10:53:06Z

Set title to [WIP] because of @potiuk's message on the mailing list. Location of operators TBD.

mingrammer · 2019-10-29T14:35:39Z

AWSAthenaHook is prefixed with AWS, but AwsDynamoDBHook has Aws. Aren't there some conventions for the class names which have a prefix?

mingrammer · 2019-10-29T14:40:09Z

I'm working on https://issues.apache.org/jira/browse/AIRFLOW-5803.

I think it would be better if we have explicit conventions for the naming.

BasPH · 2019-10-29T21:37:18Z

AFAIK there is currently no convention for class naming. For consistency we definitely need consistent naming, and we might as well do it right now because users have to migrate anyways. @potiuk thoughts?

mingrammer · 2019-10-30T02:54:10Z

I did #6465 (comment)

dazza-codes · 2019-12-10T17:09:21Z

airflow/providers/aws/hooks/dynamodb.py

+"""
+This module contains the AWS DynamoDB hook
+"""
+from airflow.contrib.hooks.aws_hook import AwsHook


Should this import from the new package path instead? i.e.

from airflow.providers.aws.hooks import AwsHook

But I'm actually a bit confused about the migrations, because that's not anywhere obvious in https://github.com/apache/airflow/tree/master/airflow/providers/amazon/aws/hooks and it not clear to me whether it should be

from airflow.providers.amazon.aws.hooks import AwsHook

dazza-codes · 2019-12-10T17:10:56Z

airflow/providers/aws/operators/dynamodb.py

+from airflow.hooks.S3_hook import S3Hook
+from airflow.models import BaseOperator
+from airflow.providers.aws.hooks.dynamodb import AwsDynamoDBHook
+from airflow.utils.decorators import apply_defaults


Should this import from the new package path instead? i.e.

from airflow.providers.aws.hooks import S3Hook

But, it's confusing because https://github.com/apache/airflow/blob/master/airflow/providers/amazon/aws/hooks/s3.py#L61 looks like it needs to be imported from

from airflow.providers.amazon.aws.hooks.s3 import S3Hook

But that module itself uses the older contrib package:

from airflow.contrib.hooks.aws_hook import AwsHook

Very confusing.

dazza-codes · 2019-12-10T17:16:01Z

Some of the AIP-21 and naming convention requirements are being loaded onto #6764 as well, so I'll have to watch what happens here also.

stale · 2020-01-24T19:57:08Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

potiuk · 2020-01-27T11:42:46Z

@mik-laj is doing automated migration now for all contrib packages - I guess this one can be closed?

mik-laj · 2020-01-27T13:35:15Z

Yes. I've already done it in my fork.

potiuk · 2020-01-27T14:56:01Z

Closing as the whole migration is automated by @mik-laj

[AIRFLOW-5777] Migrate AWS DynamoDB components to /providers/aws [AIP…

ede29eb

…-21]

BasPH added the provider:amazon-aws AWS/Amazon - related issues label Oct 28, 2019

BasPH added 2 commits October 28, 2019 22:23

isort on airflow/providers/aws/operators/dynamodb.py

0faf55c

Add empty files with deprecation warnings on old paths

c3893ee

BasPH changed the title ~~[AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21]~~ [WIP][AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21] Oct 29, 2019

mik-laj added the AIP-21 label Oct 29, 2019

mingrammer mentioned this pull request Oct 29, 2019

[AIRFLOW-5803] Update S3Hook import paths [AIP-21] #6465

Merged

5 tasks

dazza-codes reviewed Dec 10, 2019

View reviewed changes

github-actions bot added the area:docs label Jan 5, 2020

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 24, 2020

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 27, 2020

potiuk closed this Jan 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP][AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21] #6455

[WIP][AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21] #6455

BasPH commented Oct 28, 2019 •

edited

Loading

codecov-io commented Oct 28, 2019 •

edited

Loading

mik-laj commented Oct 29, 2019

BasPH commented Oct 29, 2019

BasPH commented Oct 29, 2019

mingrammer commented Oct 29, 2019 •

edited

Loading

mingrammer commented Oct 29, 2019 •

edited

Loading

BasPH commented Oct 29, 2019

mingrammer commented Oct 30, 2019 •

edited

Loading

dazza-codes Dec 10, 2019 •

edited

Loading

dazza-codes Dec 10, 2019 •

edited

Loading

dazza-codes commented Dec 10, 2019

stale bot commented Jan 24, 2020

potiuk commented Jan 27, 2020

mik-laj commented Jan 27, 2020

potiuk commented Jan 27, 2020

[WIP][AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21] #6455

[WIP][AIRFLOW-5777] Migrate AWS DynamoDB to /providers/aws [AIP-21] #6455

Conversation

BasPH commented Oct 28, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

codecov-io commented Oct 28, 2019 • edited Loading

Codecov Report

mik-laj commented Oct 29, 2019

BasPH commented Oct 29, 2019

BasPH commented Oct 29, 2019

mingrammer commented Oct 29, 2019 • edited Loading

mingrammer commented Oct 29, 2019 • edited Loading

BasPH commented Oct 29, 2019

mingrammer commented Oct 30, 2019 • edited Loading

dazza-codes Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

dazza-codes Dec 10, 2019 • edited Loading

Choose a reason for hiding this comment

dazza-codes commented Dec 10, 2019

stale bot commented Jan 24, 2020

potiuk commented Jan 27, 2020

mik-laj commented Jan 27, 2020

potiuk commented Jan 27, 2020

BasPH commented Oct 28, 2019 •

edited

Loading

codecov-io commented Oct 28, 2019 •

edited

Loading

mingrammer commented Oct 29, 2019 •

edited

Loading

mingrammer commented Oct 29, 2019 •

edited

Loading

mingrammer commented Oct 30, 2019 •

edited

Loading

dazza-codes Dec 10, 2019 •

edited

Loading

dazza-codes Dec 10, 2019 •

edited

Loading