Skip to content

RT-156 Update psycopg 2.9 -> 3.1, add simple test #190

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

taras-radchenko-reef
Copy link
Contributor

@taras-radchenko-reef taras-radchenko-reef commented Jun 28, 2024

  • Update psycopg from 2.9 to 3.1 (there are some minor breaking differences listed here)
  • Add small unit test for testing DB connection

Copy link
Contributor

@mjurbanski-reef mjurbanski-reef left a comment

Choose a reason for hiding this comment

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

since this is a django cookiecutter template, please do a db tests using pytest-django

Copy link
Contributor

@mjurbanski-reef mjurbanski-reef left a comment

Choose a reason for hiding this comment

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

btw, please silence

couldn't import psycopg 'c' implementation: No module named 'psycopg_c'

message

@@ -12,6 +12,7 @@ Currently, `cookiecutter-rt-django` has no explicit versioning amd we purely rel

## [Unreleased]

* **BREAKING** Update psycopg from 2.9 to 3.1 (https://www.psycopg.org/psycopg3/docs/basic/from_pg2.html)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it breaking if we use it strictly through django ORM as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that Django adjusts the behavior (at least settings) to the installed version, so I guess is safe to use with either. I haven't found any problem reports coming from this upgrade from a quick online search. I'll remove this

Copy link
Contributor

Choose a reason for hiding this comment

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

it was fine here, but I guess it will be also fine without that line since its verryyy rare we use posrgresql driver features directly

pytestmark = pytest.mark.django_db


class DummyValue(models.Model):
Copy link
Contributor

Choose a reason for hiding this comment

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

django has some built-in models, no reason to add one just for this tests

... I'm actually perplexed how this passed the CI without a migration for this dummy model - I was underimpression it would not.

Copy link
Contributor

Choose a reason for hiding this comment

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

it seems this only works because there are no migrations in this project

If someone uses this template as is, as soon as they create migrations they will have to debug this broken tests which is not a good experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack, now using User model instead

features.md Outdated
@@ -9,7 +9,7 @@
- [Gunicorn](https://gunicorn.org) for running WSGI instances on prod
- [Uvicorn](https://www.uvicorn.org) for ASGI instances on prod
- [Nginx](https://www.nginx.com) as high-performance reverse proxy with automatic SSL certificate renewal
- [Postgres](https://www.postgresql.org) for database
- [Postgres](https://www.postgresql.org) with [psycopg](https://www.psycopg.org) for database
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- [Postgres](https://www.postgresql.org) with [psycopg](https://www.psycopg.org) for database
- [Postgres](https://www.postgresql.org) with [psycopg3](https://www.psycopg.org) for database

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the link to point to psycopg3 page as well

This logger is only used for the following debug message:
  couldn't import psycopg 'c' implementation: No module named 'psycopg_c'

Disable it in Django settings because it is logged before pytest
configures its loggers.
@taras-radchenko-reef
Copy link
Contributor Author

btw, please silence

couldn't import psycopg 'c' implementation: No module named 'psycopg_c'

message

This is logged when psycopg is imported, and uses logging config from Django settings (this happens before pytest configures log capturing). I have disabled this logger, it's only used for this message.

@mjurbanski-reef mjurbanski-reef merged commit aed226d into reef-technologies:master Jul 2, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants