ashb commented on code in PR #68314:
URL: https://github.com/apache/airflow/pull/68314#discussion_r3387080279
##########
.pre-commit-config.yaml:
##########
@@ -1176,6 +1176,21 @@ repos:
files: ^airflow-core/src/airflow/migrations/versions/.*\.py$
additional_dependencies: [rich>=13.6.0]
require_serial: true
+ - id: check-postgres-driver-explicit
+ name: Forbid bare postgresql:// without explicit driver (SQL001)
+ entry: ./scripts/ci/prek/check_postgres_driver_explicit.py
+ language: python
+ pass_filenames: true
+ files: >
+ (?x)
+ ^airflow-core/src/.*\.(py|cfg)$|
+ ^airflow-core/docs/howto/set-up-database\.rst$|
+ ^devel-common/src/.*\.cfg$|
+ ^task-sdk-integration-tests/docker-compose\.yaml$
+ exclude: >
+ (?x)
+ ^scripts/ci/prek/check_postgres_driver_explicit\.py$
+ additional_dependencies: [rich>=13.6.0]
Review Comment:
For what amounts to a one off change, adding a pre commit check honestly
feels like overkill. It will stick around and be run every time, but the
warning in configuration.py serves instead.
##########
airflow-core/docs/core-concepts/multi-team.rst:
##########
@@ -470,7 +470,7 @@ is uppercase.
export AIRFLOW__TEAM_B___CELERY__BROKER_URL="redis://team-b-redis:6379/0"
# team_b's Celery result backend
- export
AIRFLOW__TEAM_B___CELERY__RESULT_BACKEND="db+postgresql://team-b-db/celery_results"
+ export
AIRFLOW__TEAM_B___CELERY__RESULT_BACKEND="db+postgresql+psycopg2://team-b-db/celery_results"
Review Comment:
Does this actually work with Celery? How much parsing does Celery do, how
much does it hand off to just SQLA?
##########
airflow-core/src/airflow/configuration.py:
##########
@@ -372,12 +372,12 @@ def _upgrade_postgres_metastore_conn(self):
Upgrade SQL schemas.
As of SQLAlchemy 1.4, schemes `postgres+psycopg2` and `postgres`
- must be replaced with `postgresql`.
+ must be replaced with `postgresql+psycopg2`.
"""
section, key = "database", "sql_alchemy_conn"
old_value = self.get(section, key, _extra_stacklevel=1)
bad_schemes = ["postgres+psycopg2", "postgres"]
Review Comment:
Shouldn't this also include `postgresql`, otherwise it won't do anything --
it won't replace `postgresql://` anyway.
##########
providers/celery/src/airflow/providers/celery/executors/default_celery.py:
##########
@@ -81,7 +81,7 @@ def get_default_celery_config(team_conf) -> dict[str, Any]:
sql_alchemy_conn = team_conf.get("database", "SQL_ALCHEMY_CONN")
# In SQLAlchemy 2.1 the default PostgreSQL driver changed from
psycopg2 to psycopg (v3).
# To maintain existing behavior, we explicitly specify psycopg2 for
driverless PostgreSQL URLs.
- sql_alchemy_conn = sql_alchemy_conn.replace("postgresql://",
"postgresql+psycopg2://", 1)
Review Comment:
What is SQL001 -- I can't see that in Ruff's rules, and it feels odd to make
up a code that looks like it could be ruff but isn't.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
