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

feat: get container image #1912

Conversation

FedericoAntoniazzi
Copy link

@FedericoAntoniazzi FedericoAntoniazzi commented Nov 11, 2023

What does this PR do?

As per the testcontainers.Container.Name method, allow retrieving the image from a testcontainers.Container interface.

The function has been named ContainerImage since Image clashes with the Container struct field.

Why is it important?

I'm using it on a project to retrieve images from containers running in a docker host and I use testcontainers-go to create containers for a testing environment.

Related issues

No related issues.

How to test this PR

This PR works exactly like Container.Name() method, which hasn't been tested, probably because it mainly uses external dependencies.

@FedericoAntoniazzi FedericoAntoniazzi requested a review from a team as a code owner November 11, 2023 15:11
Copy link

netlify bot commented Nov 11, 2023

Deploy Preview for testcontainers-go ready!

Name Link
🔨 Latest commit aedea1a
🔍 Latest deploy log https://app.netlify.com/sites/testcontainers-go/deploys/654f99bb3b5afb00080896e1
😎 Deploy Preview https://deploy-preview-1912--testcontainers-go.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdelapenya
Copy link
Member

Hi @FedericoAntoniazzi, thanks for taking the time for your contribution. I'd like to understand more about your use case, as the Image of a container can be obtained from the container request that originated it (unless there is a process modifying it). Could you elaborate a little bit more if possible?

@FedericoAntoniazzi
Copy link
Author

Thank you for your response @mdelapenya, let me try to explain my use case:
I'm developing a tool that queries the docker engine to retrieve the image from running containers, and I'm using testcontainers-go as a wrapper for creating N containers for each test case.
Basically, I want to test different image names and formats (e.g. nginx, library/nginx, and docker.io/library/nginx) so I created a function for creating those containers by calling it in each test case. This function returns the list of testcontainers that have been spawned, in order to have information such as the ID or the name (which is modified from the request, I notice a / added by the docker daemon), and retrieving the image directly from the testcontainers-go requires less custom code.

@mdelapenya
Copy link
Member

mdelapenya commented Nov 15, 2023

Hi @FedericoAntoniazzi thanks for the explanation. We are exposing a Docker client in order to do this kind of things:

ctx := context.Background()
client, err := testcontainers.NewDockerClientWithOpts(ctx)
if err != nil {
	// ...
}
defer client.Close()

// c is the Docker container instance
inspect, err := client.ContainerInspect(ctx, c.ID)
if err != nil {
	// ...
}
// Do whatever you need with the ContainerJSON here

I'd advise against adding a new method to the interface given the simplicity of the above solution, thanks to the client. But I'd like to listen to your thoughts before closing this one.

@FedericoAntoniazzi
Copy link
Author

Hi @mdelapenya, I apologize for my late reply.

Basically I've already implemented the proposed solution but I noticed the implementation on the library is way simpler both for implementation code and the interface for developers.

I also acknowledge the feature I implemented is an edge case or even out of scope for the library, so we can close this PR

@mdelapenya
Copy link
Member

Hi @mdelapenya, I apologize for my late reply.

Basically I've already implemented the proposed solution but I noticed the implementation on the library is way simpler both for implementation code and the interface for developers.

I also acknowledge the feature I implemented is an edge case or even out of scope for the library, so we can close this PR

Thanks for your sensitivity, please feel free to send more contributions if you see them feasible 🙏 I'm closing this PR now

Cheers!

@mdelapenya mdelapenya closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants