-
-
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
Added support for copy file to container #226
Conversation
container.go
Outdated
@@ -49,6 +49,8 @@ type Container interface { | |||
NetworkAliases(context.Context) (map[string][]string, error) // get container network aliases for a network | |||
Exec(ctx context.Context, cmd []string) (int, error) | |||
ContainerIP(context.Context) (string, error) // get container ip | |||
CopyToContainer(ctx context.Context, containerPath string, content io.Reader) 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.
Shouldn't it be called "CopyFromContainer" ?
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.
@joe-getcouragenow My use case is a little different than yours. I need to copy a single file to the container. CopyToContainer copies a compacted file to the container (using io.Reader), I then implemented CopyFileToContainer as well to wrap and allow to copy only one file to the container, using CopyToContainer.
Does that make more sense to you?
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.
Yep it does :)
Thanks.
I will try to do the copy from Container --> Host
@gianarb Do you know why the build is failing? I checked in Travis and it says The command "scripts/checks.sh" exited with 1. But when I run this script locally it works |
Found the problem |
@@ -1267,3 +1268,31 @@ func TestContainerNonExistentImage(t *testing.T) { | |||
}) | |||
|
|||
} | |||
|
|||
func TestDockerContainerCopyFileToContainer(t *testing.T) { |
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 added a single test using CopyFileToContainer since it uses already CopyToContainer underneath
docker.go
Outdated
@@ -306,6 +307,31 @@ func (c *DockerContainer) Exec(ctx context.Context, cmd []string) (int, error) { | |||
return exitCode, nil | |||
} | |||
|
|||
func (c *DockerContainer) CopyToContainer(ctx context.Context, containerPath string, content io.Reader) error { | |||
return c.provider.client.CopyToContainer(ctx, c.ID, containerPath, content, types.CopyToContainerOptions{}) |
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.
since I didn't need any special configuration, Im using the default types.CopyToContainerOptions, let me know if you think its better to already expose this
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.
Hi 👋 ! Is there a special need to expose this method, instead of directly calling the client provider method from the CopyFileToContainer
? I feel a bit confused for having two methods in the API.
What do you think about this signature?
func (c *DockerContainer) CopyFileToContainer(ctx context.Context, hostPath string, containerPath string, fileMode int64) error
With this method, the client code must simply provide the source (localPath) and the target (containerPath), and the internals of the method will obtain all the information from those two params: will read the bytes and generate the Reader needed by the provider client. I think the API would be cleaner, but I'd like to hear your voice here.
BTW, I already implemented something like this in the past here: https:/mdelapenya/lpn/blob/master/docker/docker.go#L141-L195
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.
@mdelapenya tks for your comment! The only reason I exposed this method is because it will allow clients to copy multiple files at once, since it exposes the io.Reader to an archive file. Im not really interested in this method to be honest, I'm using the one below that copies a single file.
What do you think? Should I set this first one to internal?
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.
Let's delegate to @gianarb for a decision. From my point of view, I'd go with the simplest use case, which is copying a file. We could build on top of this to copy multiples files later on, if/when needed. Sounds good?
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.
Sounds good to me! I removed the copyToContainer method. Leg me know what you guys think
@gianarb Added tests and a documentation example, let me know if it sounds good to you |
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 added a few suggestions. Please share your thoughts with us :)
docker.go
Outdated
@@ -306,6 +307,31 @@ func (c *DockerContainer) Exec(ctx context.Context, cmd []string) (int, error) { | |||
return exitCode, nil | |||
} | |||
|
|||
func (c *DockerContainer) CopyToContainer(ctx context.Context, containerPath string, content io.Reader) error { | |||
return c.provider.client.CopyToContainer(ctx, c.ID, containerPath, content, types.CopyToContainerOptions{}) |
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.
Hi 👋 ! Is there a special need to expose this method, instead of directly calling the client provider method from the CopyFileToContainer
? I feel a bit confused for having two methods in the API.
What do you think about this signature?
func (c *DockerContainer) CopyFileToContainer(ctx context.Context, hostPath string, containerPath string, fileMode int64) error
With this method, the client code must simply provide the source (localPath) and the target (containerPath), and the internals of the method will obtain all the information from those two params: will read the bytes and generate the Reader needed by the provider client. I think the API would be cleaner, but I'd like to hear your voice here.
BTW, I already implemented something like this in the past here: https:/mdelapenya/lpn/blob/master/docker/docker.go#L141-L195
docker_test.go
Outdated
fileContent, err := ioutil.ReadFile("./testresources/hello.sh") | ||
if err != nil { | ||
t.Fatal(err) | ||
} |
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.
With above explanation, this would be a matter of the internals of the copy method, simplifying client code's life
container.go
Outdated
@@ -49,6 +49,7 @@ type Container interface { | |||
NetworkAliases(context.Context) (map[string][]string, error) // get container network aliases for a network | |||
Exec(ctx context.Context, cmd []string) (int, error) | |||
ContainerIP(context.Context) (string, error) // get container ip | |||
CopyFileToContainer(ctx context.Context, containerPath string, fileName string, fileContent []byte, fileMode int64) 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.
What do you think about not exposing the fileContent
in method header, and obtain it internally? That would simplify clients code.
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.
that sounds great, do you think a single parameter like filePath, meaning the filePath on the client side, and from it we can extract the filename to save in the container?
or maybe have a containerFilePath (instead of containerPath) and from it extract the name to save in the container?
What do you think?
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 like the first one more: containerFilePath and hostFilePath. I'd get the file name from the host file, using it in the container too. Wdyt?
Besides that, what are the Java folks doing? (I'm currently on the phone)
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.
In java the method receives a Transferable (I imagine that this is the content in bytes) and the filepath that we want in the container (path + filename). https:/testcontainers/testcontainers-java/blob/6bdbc8ffe257fc879791eb7fddcf3b5ecccdd52d/core/src/main/java/org/testcontainers/containers/ContainerState.java#L262
Regarding the suggested solution, I think it would be better to use the filename from the container one, this way users will be able to copy the file to the container and at the same time rename it, its more flexible. What do you think?
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 updated the interface, let me know if it sounds good to you
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.
Thanks @franklinlindemberg for this new cool feature! |
@gianarb @mdelapenya my pleasure guys! Do you have an idea will a new release will be created? Currently Im importing my merge commit version (btw the latest commit in master gives me an error about logrus import), but it would be nice to import a specific release |
Missing tests and update in documentation.
Just a draft to validate if it looks good so far