Remove sqlalchemy dependency from postgres connection check #340

logicbomb · 2023-04-23T15:48:34Z

I decided to include both the log check & the pg_isready check, as it seems they both need to be successful in order to enure the database is running.

I don't think the driver parameter belongs in Postgres.__init__ method, as it's only used when building the connection string, but I didn't want to introduce any breaking changes.

Also, I am pretty new to python and was having trouble adding my fork of the project as a package to an external test project. It would be nice if there were some guidance on that in the README. I tried following the instructions in both the poetry and pip documentation, but couldn't install from github as there is neither setup.py nor pyproject.toml in the root directory.

Create a new generic database subclass - DependencyFreeDbContainer Remove test that was testing sqlalchemy support for driver types Add tests for supported versions of Postgres Modify the `get_connection_url` convenience method to support a driverless url

Note that Postgres container no longer depends on SqlAlchemy Remove reference to unsupported version of postgres Show an example of using the `driver` parameter

tillahoffmann

Thanks for the contribution. Do you think we can achieve the same outcome by overriding _connect for the postgres container and moving the content of _verify_status into _connect?

tillahoffmann · 2023-05-04T21:08:50Z

README.rst

@@ -43,14 +43,30 @@ Getting Started
    >>> from testcontainers.postgres import PostgresContainer
    >>> import sqlalchemy

-    >>> with PostgresContainer("postgres:9.5") as postgres:


Let's stick with 9.5 here. Pinning the version in the docs is to ensure that things don't accidentally break because one of the dependencies changed.

According to https://www.postgresql.org/support/versioning/, 9.5 is not supported anymore, the oldest supported version is 11. My suggestion would be to go with 15 and then have another 5 years until the docs have to be changed... :-)

Makes sense, let's bump to 15.

... so now 16 :-)

tillahoffmann · 2023-05-04T21:09:52Z

README.rst

-    >>> with PostgresContainer("postgres:9.5") as postgres:
-    ...     engine = sqlalchemy.create_engine(postgres.get_connection_url())
+    >>> with PostgresContainer("postgres:latest") as postgres:
+    ...     psql_url = postgres.get_connection_url() # postgresql+psycopg2://test:test@localhost:61472/test


Let's remove the comment because the url will vary depending on the environment.

nicos68 · 2023-08-29T17:04:48Z

Hi,

any idea when this could be merged ? I’m using peewee as my orm and would love to get rid of that too.

jankatins · 2023-10-19T16:04:09Z

This could also remove the hard dependency on psycopg2 which is slowly superseded by psycopg (v3). At least for us it's now the only package which pulls in the psycopg2 binary :-/

jankatins

This actually needs to remove the dependencies on sqlalchemy and psycopg2

jamesbraza · 2023-11-11T01:23:16Z

Hello friends, random person from the internet. If this feature was supported, it would be pretty nice. For those involved, if you could forward it (seems the developer needed some CI workflow manually triggered), it would be much appreciated 🙏

jankatins · 2024-01-23T12:55:16Z

postgres/tests/test_postgres.py

-            for row in result:
-                assert row[0].lower().startswith("postgresql 9.5")
+    # https://www.postgresql.org/support/versioning/
+    supported_versions = ["11", "12", "13", "14", "latest"]


please add 15 and 16 and maybe remove 11 (not supported anymore)

dvuletas · 2024-02-20T15:13:31Z

When this would be expected to be merged?
Thanks!

jankatins · 2024-02-28T13:09:10Z

@logicbomb Will you find time to get this running on top of the current main branch? If not: would you mind if I take this and rebase (or rework, if needed) and open it as a new PR (I will keep your handle as co-author on the commit)?

totallyzen · 2024-03-04T09:31:43Z

If not: would you mind if I take this and rebase (or rework, if needed) and open it as a new PR (I will keep your handle as co-author on the commit)?

Thanks for the activity, mate! I personally wouldn't mind you taking this on (my default attitude is "people are long gone" unless proven otherwise).

However, if I could ask for a tweak 😇 🥺

Make it not be in generic.py. These "db containers" assume a certain URL format, but is it worth it to have an entire class just for this? (I won't hide my distaste over DbContainer and why it is in core in the first place)
It's possible the same thing as for kafka, I would be happy with that approach, see fix(kafka): wait_for_logs in kafka container to reduce lib requirement #377

jankatins · 2024-03-04T10:09:33Z

Make it not be in generic.py. These "db containers" assume a certain URL format, but is it worth it to have an entire class
just for this? (I won't hide my distaste over DbContainer and why it is in core in the first place)

So what's the alternative (in the order of my preference, best to lowest)?

Move it to a different file?
Make all the db container implement ´_create_connection_url()`?
Add a different layer, like singledispatch to turn the container into an URL?

It's possible the same thing as for kafka, I would be happy with that approach, see
#377

This is exactly what this PR does :-) It uses both logs and pg_isready to check instead of opening a connection. It's still missing that the dependency can be removed.

logicbomb · 2024-03-04T17:09:02Z

Hi Jen, Sorry for the late reply, I'm happy for you to take it over - i'm working on too many things atm. jt

…

On Mon, Mar 4, 2024 at 5:09 AM Jan Katins ***@***.***> wrote: Make it not be in generic.py. These "db containers" assume a certain URL format, but is it worth it to have an entire class just for this? (I won't hide my distaste over DbContainer and why it is in core in the first place) So what's the alternative (in the order of my preference, best to lowest)? - Move it to a different file? - Make all the db container implement ´_create_connection_url()`? - Add a different layer, like singledispatch to turn the container into an URL? It's possible the same thing as for kafka, I would be happy with that approach, see #377 <#377> This is exactly what this PR does :-) It uses both logs and pg_isready to check instead of opening a connection. It's still missing that the dependency can be removed. — Reply to this email directly, view it on GitHub <#340 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAASJJIOIKAHHWUN7HJNC4TYWRB6VAVCNFSM6AAAAAAXITOD2KVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNZWGIZDGMJYGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

jankatins · 2024-03-06T20:59:58Z

I opened #445 for the rebased version.

alexanderankin · 2024-03-09T14:27:42Z

closing in favor of #445 for now

…445) Updates the pg testcontainer implementation to not use (and not install) SQLAlchemy nor psycopg2. Closes: #340 Closes: #336 Closes: #320 --------- Co-authored-by: Jason Turim <jason@opscanvas.com>

…estcontainers#445) Updates the pg testcontainer implementation to not use (and not install) SQLAlchemy nor psycopg2. Closes: testcontainers#340 Closes: testcontainers#336 Closes: testcontainers#320 --------- Co-authored-by: Jason Turim <jason@opscanvas.com>

logicbomb mentioned this pull request Apr 23, 2023

Design question / is a hard dependency on SqlAlchemy really necessary? #336

Closed

logicbomb force-pushed the issue_336 branch from 6dcbd60 to 6c122d5 Compare April 23, 2023 15:51

Jason Turim added 2 commits May 1, 2023 08:45

Update README.rst

c6bf6b9

Note that Postgres container no longer depends on SqlAlchemy Remove reference to unsupported version of postgres Show an example of using the `driver` parameter

logicbomb force-pushed the issue_336 branch from 6c122d5 to c6bf6b9 Compare May 1, 2023 12:45

tillahoffmann changed the title ~~Issue 336~~ Remove sqlalchemy dependency from postgres connection check May 4, 2023

tillahoffmann linked an issue May 5, 2023 that may be closed by this pull request

Design question / is a hard dependency on SqlAlchemy really necessary? #336

Closed

tillahoffmann requested changes May 5, 2023

View reviewed changes

Respond to comments

4713d28

tillahoffmann added the 📦 package: postgres label May 9, 2023

tillahoffmann mentioned this pull request May 11, 2023

Make the kafka package dependency optional to be able to use confluent_kafka #351

Closed

github-actions bot added the 👀 requires attention label Aug 29, 2023

jankatins suggested changes Oct 19, 2023

View reviewed changes

jankatins mentioned this pull request Oct 19, 2023

add postgres sqlalchemy asyncio support #359

Closed

jankatins reviewed Jan 23, 2024

View reviewed changes

jankatins mentioned this pull request Mar 6, 2024

fix(postgres): Remove SqlAlchemy dependency from postgres container #445

Merged

alexanderankin closed this Mar 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove sqlalchemy dependency from postgres connection check #340

Remove sqlalchemy dependency from postgres connection check #340

logicbomb commented Apr 23, 2023

tillahoffmann left a comment

tillahoffmann May 4, 2023

jankatins May 11, 2023

tillahoffmann May 11, 2023

jankatins Jan 23, 2024

tillahoffmann May 4, 2023

nicos68 commented Aug 29, 2023

jankatins commented Oct 19, 2023

jankatins left a comment

jamesbraza commented Nov 11, 2023

jankatins Jan 23, 2024

dvuletas commented Feb 20, 2024

jankatins commented Feb 28, 2024

totallyzen commented Mar 4, 2024

jankatins commented Mar 4, 2024

logicbomb commented Mar 4, 2024 via email

jankatins commented Mar 6, 2024

alexanderankin commented Mar 9, 2024

Remove sqlalchemy dependency from postgres connection check #340

Remove sqlalchemy dependency from postgres connection check #340

Conversation

logicbomb commented Apr 23, 2023

tillahoffmann left a comment

Choose a reason for hiding this comment

tillahoffmann May 4, 2023

Choose a reason for hiding this comment

jankatins May 11, 2023

Choose a reason for hiding this comment

tillahoffmann May 11, 2023

Choose a reason for hiding this comment

jankatins Jan 23, 2024

Choose a reason for hiding this comment

tillahoffmann May 4, 2023

Choose a reason for hiding this comment

nicos68 commented Aug 29, 2023

jankatins commented Oct 19, 2023

jankatins left a comment

Choose a reason for hiding this comment

jamesbraza commented Nov 11, 2023

jankatins Jan 23, 2024

Choose a reason for hiding this comment

dvuletas commented Feb 20, 2024

jankatins commented Feb 28, 2024

totallyzen commented Mar 4, 2024

jankatins commented Mar 4, 2024

logicbomb commented Mar 4, 2024 via email

jankatins commented Mar 6, 2024

alexanderankin commented Mar 9, 2024