Chore: Remove deprecated pyscopg v2 (pyscopg2) #532
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
aaronsteers
changed the title
aaronsteers
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve updates to the PostgreSQL connection handling within the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
airbyte/_processors/sql/postgres.py (1)
32-32: Consider adding migration notesSince we're moving from psycopg2 to psycopg3, should we add a note in the docstring about this significant change? This could help users understand the migration path. wdyt? 🤔
Here's a suggestion for the docstring:
def get_sql_alchemy_url(self) -> SecretString: - """Return the SQLAlchemy URL to use.""" + """Return the SQLAlchemy URL to use. + + Note: As of version X.Y.Z, we use psycopg3 instead of psycopg2. + For migration details, see: <link-to-docs> + """tests/conftest.py (1)
140-142: LGTM! Quick suggestion about URL readabilityThe URL change from postgresql to postgresql+psycopg looks good and aligns with removing psycopg2! 🎉
What do you think about extracting the URL components into variables for better readability? Something like this:
- pg_url = ( - f"postgresql+psycopg://postgres:postgres@{host}:{PYTEST_POSTGRES_PORT}/postgres" - ) + scheme = "postgresql+psycopg" + credentials = "postgres:postgres" + database = "postgres" + pg_url = f"{scheme}://{credentials}@{host}:{PYTEST_POSTGRES_PORT}/{database}"This would make it easier to modify individual components in the future. WDYT?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
airbyte/_processors/sql/postgres.py(1 hunks)pyproject.toml(0 hunks)tests/conftest.py(1 hunks)
💤 Files with no reviewable changes (1)
- pyproject.toml
🔇 Additional comments (1)
airbyte/_processors/sql/postgres.py (1)
32-32: Verify SQLAlchemy compatibility with psycopg3
The URL scheme change looks good! Just want to double-check SQLAlchemy compatibility with the new driver. Mind if we verify this? 🤔
✅ Verification successful
SQLAlchemy and psycopg3 compatibility looks great! 👍
The verification shows everything is properly configured:
- SQLAlchemy version constraint (
>=1.4.51,!=2.0.36,<3.0) is compatible with psycopg3 - psycopg3 is correctly specified with required extras:
psycopg = {extras = ["binary", "pool"], version = "^3.2.2"} - The URL scheme change is consistent across the codebase (both in main code and tests)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SQLAlchemy and psycopg version constraints in project dependencies
# Check for SQLAlchemy version specification
rg -l "sqlalchemy" pyproject.toml requirements*.txt setup*.py
# Check for psycopg version specification
rg -l "psycopg[^2]" pyproject.toml requirements*.txt setup*.py
Length of output: 380
Script:
#!/bin/bash
# Let's check the actual content of pyproject.toml for version constraints
cat pyproject.toml
# Also search for any SQLAlchemy usage patterns in the codebase
ast-grep --pattern 'create_engine($_)'
# And check for any existing SQLAlchemy connection strings
rg "postgresql\+" -A 2 -B 2
Length of output: 11833
|
/poetry-lock
|
Resolving dependencies... changes
|
/poetry-lock
|
Summary by CodeRabbit
New Features
psycopgdriver for enhanced database interactions.Bug Fixes
Tests