-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Change the URL building in HttpHookAsync to match the behavior of HttpHook #37696
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 Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
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.
Left a few nitpicks, but overall looks good!
This reverts commit 5fa8aed.
…7101) * Implement | and & operators so that they can be used instead of DatasetAll and DatasetAny * Refactor dataset class inheritance (apache#37590) * Refactor DatasetAll and DatasetAny inheritance They are moved from airflow.models.datasets to airflow.datasets since the intention is to use them with Dataset, not DatasetModel. It is more natural for users to import from the latter module instead. A new (abstract) base class is added for the two classes, plus the OG Dataset class, to inherit from. This allows us to replace a few isinstance checks with simple molymorphism and make the logic a bit simpler. Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Wei Lee <weilee.rx@gmail.com> Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
@alejorodriguez96 could you rebase your changes to the current |
…7101) * Implement | and & operators so that they can be used instead of DatasetAll and DatasetAny * Refactor dataset class inheritance (apache#37590) * Refactor DatasetAll and DatasetAny inheritance They are moved from airflow.models.datasets to airflow.datasets since the intention is to use them with Dataset, not DatasetModel. It is more natural for users to import from the latter module instead. A new (abstract) base class is added for the two classes, plus the OG Dataset class, to inherit from. This allows us to replace a few isinstance checks with simple molymorphism and make the logic a bit simpler. Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Wei Lee <weilee.rx@gmail.com> Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
35ffead
to
24e37fb
Compare
Done! Sorry had some problems rebasing |
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.
Nice work. LGTM.
…pHook (apache#37696) They are moved from airflow.models.datasets to airflow.datasets since the intention is to use them with Dataset, not DatasetModel. It is more natural for users to import from the latter module instead. A new (abstract) base class is added for the two classes, plus the OG Dataset class, to inherit from. This allows us to replace a few isinstance checks with simple molymorphism and make the logic a bit simpler. Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Wei Lee <weilee.rx@gmail.com> Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
…pHook (apache#37696) They are moved from airflow.models.datasets to airflow.datasets since the intention is to use them with Dataset, not DatasetModel. It is more natural for users to import from the latter module instead. A new (abstract) base class is added for the two classes, plus the OG Dataset class, to inherit from. This allows us to replace a few isinstance checks with simple molymorphism and make the logic a bit simpler. Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com> Co-authored-by: Wei Lee <weilee.rx@gmail.com> Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
As mentioned in #37664
HttpHookAsync
asumes thatbase_url
is passed to the hook or can be extracted from connection, whereasHttpHook
handles correctly when nobase_url
is passed andendpoint
is a complete url.This PR aims to set the same behavior to both hooks.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an 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 a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.