-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fix apache#47555: PostgreSQL URI driver issue #47709
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
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)
|
Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field.
Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field.
9e1a54d to
7b55aed
Compare
…flow into fix-postgres-uri
jason810496
left a comment
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.
Looks good! Only the test needs to be added.
potiuk
left a comment
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.
I don't thnk this is needed/useful. Get_uri - returns "airflow" URI format as described in https://airflow.apache.org/docs/apache-airflow/stable/howto/connection.html#uri-format-example, the #38831 was indeed a way how to get another uri (sqlalchemy URI) - which is pretty different than AIrfow's has different potential parameters and uses postgres as prefix exclusively.
If anything - maybe it's worth (and change this PR into it) to add comment that this is NOT sqlalchemy URI and that sqlalchemy_url should be used instead.
It also makes it very fishy to add postgres-specific code in airflow models anyway.
|
@potiuk My intention was to allow If using I can modify the PR accordingly. |
Yep. That's better approach. It did cause confusion before so you are not the first one. It would be great to have it better decribed in docstring but also maybe even in documentation if we find a good place ? |
I’ll update the |
potiuk
left a comment
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
jason810496
left a comment
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.
Thanks @Dong-yeong0, but the ruff static check need fix.
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
* Fix apache#47555: PostgreSQL URI scheme issue * Fix: allow configuring PostgreSQL connection driver Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field. * Fix apache#47555: PostgreSQL URI scheme issue * Fix: allow configuring PostgreSQL connection driver Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field. * Add the documentation for the 'designator' parameter in the PostgreSQL extras section * Remove the code that constructs the URI based on connection type * Update get_uri method docstring * Clarify that get_uri() does not return a SQLAlchemy URI * Fix rst-backticks * Remove designer description * Fix format code with ruff --------- Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
* Fix apache#47555: PostgreSQL URI scheme issue * Fix: allow configuring PostgreSQL connection driver Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field. * Fix apache#47555: PostgreSQL URI scheme issue * Fix: allow configuring PostgreSQL connection driver Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field. * Add the documentation for the 'designator' parameter in the PostgreSQL extras section * Remove the code that constructs the URI based on connection type * Update get_uri method docstring * Clarify that get_uri() does not return a SQLAlchemy URI * Fix rst-backticks * Remove designer description * Fix format code with ruff --------- Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
* Fix apache#47555: PostgreSQL URI scheme issue * Fix: allow configuring PostgreSQL connection driver Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field. * Fix apache#47555: PostgreSQL URI scheme issue * Fix: allow configuring PostgreSQL connection driver Added support for customizing the PostgreSQL connection driver (`postgres` vs `postgresql`) via the `postgres_driver` option in the extra JSON field. * Add the documentation for the 'designator' parameter in the PostgreSQL extras section * Remove the code that constructs the URI based on connection type * Update get_uri method docstring * Clarify that get_uri() does not return a SQLAlchemy URI * Fix rst-backticks * Remove designer description * Fix format code with ruff --------- Co-authored-by: LIU ZHE YOU <68415893+jason810496@users.noreply.github.com>
This PR fixes an issue(#47555) where Airflow incorrectly generates a PostgreSQL connection URI using
postgres://instead ofpostgresql://, which causes SQLAlchemy to fail.^ 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.rstor{issue_number}.significant.rst, in newsfragments.