Skip to content

Generic S3 to SQL #23881

Closed
thinhnd2104 wants to merge 55 commits intoapache:mainfrom
thinhnd2104:main
Closed

Generic S3 to SQL #23881
thinhnd2104 wants to merge 55 commits intoapache:mainfrom
thinhnd2104:main

Conversation

@thinhnd2104
Copy link
Contributor

Relate #23666

@vincbeck
Copy link
Contributor

vincbeck commented Jun 1, 2022

General questions and reminders:

  • Did you test you operator in a DAG?
  • Please add unit tests for the operator
  • Please add a sample dag here airflow/providers/amazon/aws/example_dags/example_s3_to_sql.py. You can take a look at an example, airflow/providers/amazon/aws/example_dags/example_sql_to_s3.py should be pretty similar
  • Please add documentation here docs/apache-airflow-providers-amazon/operators/transfer/s3_to_sql.rst. Same as the example dag, feel free to look at other documentation and try to follow the same format

Thank you for adding this operator :)

@thinhnd2104 thinhnd2104 requested a review from mik-laj as a code owner June 1, 2022 16:19
@thinhnd2104
Copy link
Contributor Author

General questions and reminders:

* Did you test you operator in a DAG?

* Please add unit tests for the operator

* Please add a sample dag here `airflow/providers/amazon/aws/example_dags/example_s3_to_sql.py`. You can take a look at an example, `airflow/providers/amazon/aws/example_dags/example_sql_to_s3.py` should be pretty similar

* Please add documentation here `docs/apache-airflow-providers-amazon/operators/transfer/s3_to_sql.rst`. Same as the example dag, feel free to look at other documentation and try to follow the same format

Thank you for adding this operator :)

Thanks for your comments. In my case, I execute this operator on ECS cluster by docker image =)) so I will test this operator and add unit test later.

@thinhnd2104 thinhnd2104 requested a review from josh-fell June 12, 2022 07:00
@thinhnd2104
Copy link
Contributor Author

@vincbeck @josh-fell @Mik-lạ. Please review my operator. Thanks

Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me! Just one minor comment from me

@thinhnd2104
Copy link
Contributor Author

@potiuk please review my operator. thanks

@thinhnd2104
Copy link
Contributor Author

any update? @josh-fell @mik-laj

from airflow.utils.session import create_session


class TestS3ToSqlTransfer(unittest.TestCase):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For net-new tests, it's preferred to use pytest rather than unittest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thinhnd2104 A friendly reminder to have this new test use pytest instead of unittest. No rush of course.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

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

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 6, 2022
@github-actions github-actions bot closed this Sep 11, 2022
@o-nikolas
Copy link
Contributor

@thinhnd2104 This PR has been closed as stale, are you planning to resume work on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants