Closed Tranquility2 closed 3 months ago
some thoughts:
Attention: Patch coverage is 90.00000%
with 4 lines
in your changes missing coverage. Please review.
Please upload report for BASE (
main@59cb6fc
). Learn more about missing BASE report.
Files | Patch % | Lines |
---|---|---|
core/testcontainers/core/generic.py | 87.50% | 3 Missing and 1 partial :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Yes, you are right, ServerContainer
is a much better name and it correlates to your second point, I do think its very general and more in par with DBContainer
than a specific module (thats why I placed it under generics). I think the modules should remain specific product related. I'll try and support my claim with the java implementation of "nginx container" (which I'll probably add later on 😅 ) https://github.com/testcontainers/testcontainers-java/blob/6658a2c0a880d01c6d402ea9a4cb5f72eb15083c/modules/nginx/src/main/java/org/testcontainers/containers/NginxContainer.java#L12C13-L12C29
Notice it uses GenericContainer
which is (in a way) the combination of DBContainer
and ServerContainer
.
So to conclude I think we should keep ServerContainer
under generics, and maybe plan in the future to merge DBContainer
and ServerContainer
(?) into GenericContainer
(but to be honest I think the separation is good at this stage)
for context, we are trying to remove DbContainer as well.
will take a look.
I can offer another alternative, completely skip ServerContainer
and just make the modules repeat some code, I can also check it out and propose a draft if you like. (with ServerContainer
I tried to create something useful, maybe I just missed the big picture / context)
rebased with new improvements
As part of the effort described, detailed and presented on #559 This is the seconds PR (out of 4) that should provide all the groundwork to support containers running a server.
This would allow users to use custom images:
Next in line are:
feat(core): Added FastAPI module
feat(core): Added AWS Lambda module
Based on the work done on #585 Expended from issue #83