-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
feat: prepare modules for the new API #2610
feat: prepare modules for the new API #2610
Conversation
It will force the image to be passed
✅ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
RunContainer
in favor ofRun
Run
function 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.