Skip to content
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

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

Closed
logicbomb opened this issue Apr 18, 2023 · 8 comments · Fixed by #445
Closed

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

logicbomb opened this issue Apr 18, 2023 · 8 comments · Fixed by #445
Labels
✅ close on merge Issue that will be closed by an open pull request fix 🔧 maintenance 📦 package: core

Comments

@logicbomb
Copy link

Hi,

It seems that the _connect method is causing the DbContainer project to take a hard dependency on SqlAlchemy. This is troublesome for those of us not using that package. Is it meant to do more than verify the database container is up & running? If not, maybe there's a more neutral way of checking like using pg_isready?

thanks,
Jason

@logicbomb logicbomb changed the title Design question / is a hard dependency on SqlAlchemy reall necessary? Design question / is a hard dependency on SqlAlchemy really necessary? Apr 18, 2023
@tillahoffmann
Copy link
Collaborator

Yes, that's a good point. We should move away from a hard dependency on sqlalchemy as you suggested. Do you have thoughts on how to integrate that change? A PR would be very welcome!

@logicbomb
Copy link
Author

i thought to use popen to execute the pg_isready command, does that sound like a reasonable approach?

@HofmeisterAn
Copy link
Contributor

i thought to use popen to execute the pg_isready command, does that sound like a reasonable approach?

Based on my experience maintaining TC for .NET, waiting for pg_isready to succeed is not always reliable. There were instances where the .NET Npgsql client could not establish a connection. Since I wait for specific log messages, I have not encountered any connection issues.

@logicbomb
Copy link
Author

I put together a WIP here: https://github.com/logicbomb/testcontainers-python/pull/1/files

You can see 2 possible implementations, one based on pg_isready the other checking logs as described by @HofmeisterAn. Using pg_isready is much less code and seems to work, but I'm happy to go with either.

Also, would you all require a change to the way all databases are verified, or would you accept a change only for Postgres?

@tillahoffmann
Copy link
Collaborator

You may want to have a look at the wait_for_logs function here. That should reduce the amount of code required.

@logicbomb
Copy link
Author

ooh, wait_for_logs is nice. i take it you prefer that approach?

@tillahoffmann
Copy link
Collaborator

Yes, using wait_for_logs could be a good option.

@logicbomb
Copy link
Author

#340

alexanderankin pushed a commit that referenced this issue Mar 11, 2024
…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 <[email protected]>
bearrito pushed a commit to bearrito/testcontainers-python that referenced this issue Mar 30, 2024
…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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ close on merge Issue that will be closed by an open pull request fix 🔧 maintenance 📦 package: core
Projects
None yet
5 participants