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.

Sign up for GitHub

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

Jump to bottom

Add schema override in DbApiHook #16521

Merged
merged 1 commit into from
Jun 23, 2021
Merged

Add schema override in DbApiHook #16521

merged 1 commit into from
Jun 23, 2021

Conversation

LukeHong
Copy link
Contributor

@LukeHong LukeHong commented Jun 18, 2021 •
edited by eladkal
Loading

closes: #16520

Overwrite get_uri() from DbApiHook to use schema argument in PostgresHook.

^ 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.

@boring-cyborg
Copy link

boring-cyborg bot commented Jun 18, 2021

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)
Here are some useful points:

  • Pay attention to the quality of your code (flake8, pylint and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

jedcunningham
jedcunningham requested changes Jun 18, 2021
View reviewed changes
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Can you add some tests?

airflow/providers/postgres/hooks/postgres.py Outdated Show resolved Hide resolved
@jedcunningham
Copy link
Member

jedcunningham commented Jun 18, 2021 •
edited
Loading

Closes: #16520
(Ugh, thought this would properly link the issue/PR, but no luck)

@eladkal eladkal requested a review from xinbinhuang June 20, 2021 06:51
@LukeHong LukeHong changed the title Implement get_uri() in PostgresHook WIP: Implement get_uri() in PostgresHook Jun 21, 2021
@LukeHong LukeHong changed the title WIP: Implement get_uri() in PostgresHook Add schema override in DbApiHook Jun 21, 2021
uranusjr
uranusjr approved these changes Jun 21, 2021
View reviewed changes
Copy link
Member

@uranusjr uranusjr left a comment

Choose a reason for hiding this comment

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

Much cleaner IMO!

xinbinhuang
xinbinhuang reviewed Jun 21, 2021
View reviewed changes
airflow/hooks/dbapi.py Outdated Show resolved Hide resolved
@xinbinhuang
Copy link
Contributor

@LukeHong can you try to rebase to master? Pylint is failing but likely not due to your changes.

@LukeHong LukeHong force-pushed the main branch 2 times, most recently from e11986d to b0c8900 Compare June 23, 2021 04:10
@LukeHong
Copy link
Contributor Author

Update: fix tests

@xinbinhuang xinbinhuang merged commit 3ee916e into apache:main Jun 23, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Jun 23, 2021

Awesome work, congrats on your first merged pull request!

@xinbinhuang
Copy link
Contributor

@LukeHong Thanks for the contribution! :)

@eladkal eladkal modified the milestone: Airflow 2.2 Jun 24, 2021
Jorricks pushed a commit to Jorricks/airflow that referenced this pull request Jun 24, 2021
@LukeHong @Jorricks
Add schema as DbApiHook instance attribute (apache#16521)
09d9171
@potiuk potiuk mentioned this pull request Jul 15, 2021
Closed
26 tasks
@potiuk potiuk mentioned this pull request Jul 27, 2021
Closed
66 tasks
@potiuk potiuk mentioned this pull request Aug 4, 2021
Merged
potiuk added a commit to potiuk/airflow that referenced this pull request Aug 5, 2021
@potiuk
Make schema in DBApiHook private
49da83c 
There was a change in apache#16521 that introduced schema field in
DBApiHook, but unfortunately using it in provider Hooks deriving
from DBApiHook is backwards incompatible for Airflow 2.1 and below.

This caused Postgres 2.1.0 release backwards incompatibility and
failures for Airflow 2.1.0.

Since the change is small and most of DBApi-derived hooks already
set the schema field on their own, the best approach is to
make the schema field private for the DBApiHook and make a change
in Postgres Hook to store the schema in the same way as all other
operators.

Fixes: apache#17422
potiuk added a commit to potiuk/airflow that referenced this pull request Aug 7, 2021
@potiuk
Make schema in DBApiHook private
b9801ec 
There was a change in apache#16521 that introduced schema field in
DBApiHook, but unfortunately using it in provider Hooks deriving
from DBApiHook is backwards incompatible for Airflow 2.1 and below.

This caused Postgres 2.1.0 release backwards incompatibility and
failures for Airflow 2.1.0.

Since the change is small and most of DBApi-derived hooks already
set the schema field on their own, the best approach is to
make the schema field private for the DBApiHook and make a change
in Postgres Hook to store the schema in the same way as all other
operators.

Fixes: apache#17422
potiuk added a commit that referenced this pull request Aug 7, 2021
@potiuk
Make schema in DBApiHook private (#17423)
04b6559 
There was a change in #16521 that introduced schema field in
DBApiHook, but unfortunately using it in provider Hooks deriving
from DBApiHook is backwards incompatible for Airflow 2.1 and below.

This caused Postgres 2.1.0 release backwards incompatibility and
failures for Airflow 2.1.0.

Since the change is small and most of DBApi-derived hooks already
set the schema field on their own, the best approach is to
make the schema field private for the DBApiHook and make a change
in Postgres Hook to store the schema in the same way as all other
operators.

Fixes: #17422
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jedcunningham jedcunningham jedcunningham requested changes

@xinbinhuang xinbinhuang xinbinhuang approved these changes

@uranusjr uranusjr uranusjr approved these changes

@potiuk potiuk potiuk approved these changes

Assignees
No one assigned
Labels
area:providers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DbApiHook.get_uri() doesn't follow PostgresHook schema argument
6 participants
@LukeHong @jedcunningham @xinbinhuang @potiuk @uranusjr @eladkal