-
Notifications
You must be signed in to change notification settings - Fork 339
feat(generic): Reintroducing the generic SQL module #892
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #892 +/- ##
==========================================
+ Coverage 79.78% 81.45% +1.67%
==========================================
Files 14 13 -1
Lines 1182 1154 -28
Branches 184 184
==========================================
- Hits 943 940 -3
+ Misses 197 173 -24
+ Partials 42 41 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
can we add a new extra? like a db (or more accurately, sql) extra? this is assuming we dont want to aim for complete removal. Even if we do that, i dont like the _connect method on the container - i think connecting is the business of 1) app code 2) sql waiting strategy - so all the db interface has to do is tell someone how to connect - and maybe this can just return the url? This second point is made further complicated by now you can only use this waiting strategy on container that have the interface implemented. i think maybe this waiting strategy can be parametrized - with information on how to build the url. this removes the need for the container to build its own url (finally). just a thought. |
|
New version is ready:
You can now say |
I like the new |
Make one more critical improvement, now
|
@alexanderankin now using |
91d8161
to
a99045a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to migrate Postgres and CrateDB module and it's fairly easy, I left a few nitpicks that you might use as inspiration, otherwise LGTM and I'd merge.
7cd414b
to
a05abdb
Compare
a05abdb
to
bf6a553
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall clearly an improvement on the existing DbContainer
. A few questions/ideas inline, many to clarify my own understanding of stuff.
modules/generic/testcontainers/generic/providers/sql_connection_wait_strategy.py
Show resolved
Hide resolved
modules/generic/testcontainers/generic/providers/sql_connection_wait_strategy.py
Show resolved
Hide resolved
Raises: | ||
NotImplementedError: Must be implemented by subclasses | ||
""" | ||
raise NotImplementedError("Subclasses must implement get_connection_url()") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this have a default implementation to just call _create_connection_url
? The SimpleSqlContainer
implements it in this way & this feels like a reasonable default. I suspect most users of this class would either duplicate the implementation, or call the private method anyway which I think we'd want to discourage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SimpleSqlContainer is just a test, are you sure this is the default for all SQL related implementations?
for example Postgres needs driver
so its not the exact same, but an extended build using the baseline _create_connection_url
which is great. I personally like the current design and believe/hope it allows for max flexibility in the future.
Related to #884
Trying to replace the old generic.py in core to a nicer version under the generic module.
SqlContainer
its not as good (in being generic) asServerContainer
but it should allow us to deprecatecore/testcontainers/core/generic.py
with minimal effort for users, it could lead to a more Generic version likeDBContainer
in the future.Update 1: Refactor to use
SqlConnectWaitStrategy
Update 2: Now
SqlConnectWaitStrategy
is required and the users can provideSqlContainer
with any wait strategy.Update 3: Now utilizes all the latest improvements from
WaitStrategy
Note: I think the added tests + documentation (provided in this PR) are by themselves a great improvement over the current generic.py