-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Organise S3 Classes in Amazon Provider #20167
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
@KennyRich can you please rebase & resolve conflicts? airflow/tests/deprecated_classes.py Lines 1067 to 1068 in f4e04c7
This means that contrib (which is deprecated) is now pointing to deprecated class. This is probably wrong and should direct to the new class directly(?) |
@eladkal Thanks for the feedback, I'll do that and send in a PR |
# Conflicts: # dev/provider_packages/prepare_provider_packages.py # tests/deprecated_classes.py
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.
Please check with Breeze locally that test passes.
Currently if you will run:
pytest -s tests/core/test_core_to_contrib.py
It will fail. some imports are wrong and also you have some duplications (I didn't add comments on all issues... running the test will tell you what needs to be fixed)
Co-authored-by: eladkal <45845474+eladkal@users.noreply.github.com>
Co-authored-by: eladkal <45845474+eladkal@users.noreply.github.com>
Co-authored-by: eladkal <45845474+eladkal@users.noreply.github.com>
Co-authored-by: eladkal <45845474+eladkal@users.noreply.github.com>
@KennyRich you are missing also fixes to test classes: Tests are still importing operators from old paths. |
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.
LGTM
Will merge once CI is green.
@KennyRich can you please rebase and resolve conflicts?
I've resolved the merge conflicts. Thanks for your help so far! |
Awesome work, congrats on your first merged pull request! |
Part of #20139
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.