feat: prepare modules for the new API #2610
Conversation
It will force the image to be passed
mdelapenya
added
the
feature
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
* main: chore: run rootless mode in nighlty builds (testcontainers#2611)
|
|
||
| ```golang | ||
| func RunContainer(ctx context.Context, opts ...testcontainers.ContainerCustomizer) (*CassandraContainer, error) | ||
| func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustomizer) (*CassandraContainer, error) |
There was a problem hiding this comment.
Silly thought / wild thought / thinking out loud; as this is changing the interface;
docker run is a combination of steps;
- create container
- if image is missing, pull the image (depending on pull settings)
- once image is pulled;
- create container again
- start container
- attach to the container (if in foreground)
Specifically, the "image is missing" (and what to do in that case) is something that could be relevant for testing;
- Do you want the image to be pulled?
- Perhaps you want the image to be loaded (offline tar of images)
- Perhaps even need the image to be built
Wondering if the img string should be / could be a functional argument that allows controlling this (in future);
PullImageIfMissing("foo:latest")LoadImageIfMissing("foo:latest", "/path/to/tar")- ...
We probably don't need all of those implemented, but it would allow for those to be expanded on 🤔
There was a problem hiding this comment.
Before this change, we have (deprecated in this PR) a testcontainers.WithImage functional opt, that could leverage what you propose. We already have support for loading images from tar files, and the code will automatically pull the image if missing. So I'd see the second use case as a nice addition.
There was a problem hiding this comment.
I am not sure if we have to be that considerate for this change at hand (where we want to be pragmatic and move forward to allow support for Official Modules in tc-go soon), but it is for sure something to consider more for the v1 release of tc-go.
tc-java essentially provides support for such an abstraction, in practice mostly realized through a Future<String> parameter. However, the resulting API needs to be on a higher abstraction level, so that it encapsulates pulling, building or loading images.
There was a problem hiding this comment.
we want to be pragmatic and move forward
Indeed. My comment for the niceness is for follow-ups, not for this PR.
There was a problem hiding this comment.
I'm merging this, taking your ideas as follow-ups. Thanks all for the discussion here
* main: feat: prepare modules for the new API (#2610)
|
will the package also be updated soon? related to #2625 |
What does this PR do?
This PR updates all the modules, the module generator and the current docs to use the new API:
RunContainerin favor ofRunRunfunction needs the Docker image as second argument, which forces users to know which image is used in their tests.Why is it important?
Prepare for a more consistent API that makes evident which image is used.
Follow-ups
Modules catalog examples will need to be udpated in consequence.