-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Created method which copy file from repository to container #378
Created method which copy file from repository to container #378
Conversation
…e point to container path.
@@ -418,6 +418,8 @@ ExecResult execInContainer(Charset outputCharset, String... command) | |||
|
|||
InspectContainerResponse getContainerInfo(); | |||
|
|||
void copyFileToContanier(String localPath, String containerPath); |
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 suggest using MountableFile
instead of String localPath
@Override | ||
public void copyFileToContanier(String localPath, String containerPath){ | ||
|
||
if (!isRunning()) { |
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 isRunning
calls Docker API, I would remove this check and catch an exception from .exec()
@@ -851,6 +851,27 @@ public ExecResult execInContainer(String... command) | |||
} | |||
|
|||
/** |
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.
please move the JavaDoc to Container
interface
Hi @lukidzi, Thanks for your contribution! I added a few review notes from me :) I would also like to see the tests for this change |
@@ -418,6 +418,8 @@ ExecResult execInContainer(Charset outputCharset, String... command) | |||
|
|||
InspectContainerResponse getContainerInfo(); | |||
|
|||
void copyFileToContanier(String localPath, String containerPath); |
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.
typo as well
Hey @glebsts, looks good, but could you please fix what Coday says? (press Thanks! |
|
||
this.dockerClient | ||
.copyArchiveToContainerCmd(this.containerId) | ||
.withHostResource(mountableLocalFile.getDescription()) |
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.
please use getResolvedPath
* Create a container which wait for some time to test if copy to container works. | ||
*/ | ||
@ClassRule | ||
public static GenericContainer alpineCopyToContainer = new GenericContainer("alpine:3.2") |
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.
Actually, it would be nice if you can replace it with try-with-resources
version (see NetworkTest.WithoutRules
). It helps to run individual tests without starting everything
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.
can be removed now
@bsideup wrong mention, I guess?) |
…ed method to getResolvedPath
@@ -392,6 +393,17 @@ ExecResult execInContainer(String... command) | |||
ExecResult execInContainer(Charset outputCharset, String... command) | |||
throws UnsupportedOperationException, IOException, InterruptedException; | |||
|
|||
/** | |||
* | |||
* Method allow to copy file which we have in our repository to docker container |
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.
Small Javadoc change:
This method allows to copy a file
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 also think it's a file inside the classpath and not inside the repository, correct?
@@ -120,7 +122,7 @@ public static void setupContent() throws FileNotFoundException { | |||
.withExposedPorts(80) | |||
.withExtraHost("somehost", "192.168.1.10") | |||
.withCommand("/bin/sh", "-c", "while true; do cat /etc/hosts | nc -l -p 80; done"); | |||
|
|||
// @Test |
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.
Remove commented 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.
the code wasn't commented in this PR.
Although +1 for removing this added space
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.
Whoops, you're right.
@lukidzi I think this would be a good thing to have if you're still interested in contributing. If you're stuck for time please let us know, though, as it doesn't look like thee's too much more to do. |
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.
Hey, sorry for not looking into this PR for some time. I've requested some minor Javadoc changes, and will merge ASAP afterwards.
@@ -392,6 +393,17 @@ ExecResult execInContainer(String... command) | |||
ExecResult execInContainer(Charset outputCharset, String... command) | |||
throws UnsupportedOperationException, IOException, InterruptedException; | |||
|
|||
/** | |||
* | |||
* Method allow to copy file which is inside classpath to docker container |
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.
Copies a file which resides inside the classpath to the container.
* | ||
* Method allow to copy file which is inside classpath to docker container | ||
* | ||
* @param mountableLocalFile path to file which we would like to place in container |
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.
file which is copied into the container
* Method allow to copy file which is inside classpath to docker container | ||
* | ||
* @param mountableLocalFile path to file which we would like to place in container | ||
* @param containerPath path where we want to copy file |
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.
destination path inside the container
@bsideup You're review is still pending, I think the changes are fine now? |
|
||
try ( | ||
GenericContainer alpineCopyToContainer = new GenericContainer("alpine:3.2") | ||
.withCommand("sleep 9999") |
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.
please use top
instead of sleep 9999
final MountableFile mountableFile = MountableFile.forClasspathResource("test_copy_to_container.txt"); | ||
alpineCopyToContainer.copyFileToContainer(mountableFile, "/home/"); | ||
|
||
try (final InputStream response = alpineCopyToContainer |
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 it was requested a few times already, maybe you can add copyFileFromContainer()
as well? :) We should also check the file content here
try (final InputStream response = this.dockerClient | ||
.copyArchiveFromContainerCmd(this.containerId, containerPath) | ||
.exec()) { | ||
try (final TarArchiveInputStream tarInputStream = new TarArchiveInputStream(response)) { |
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.
no need for the nested try
s, just wrap .exec()
result into TarArchiveInputStream
* @throws IOException if there will be error during getNextTarEntry | ||
*/ | ||
private void createFileFromTarArchiveInputStream(final String destinationPath, final TarArchiveInputStream tarArchiveInputStream) throws IOException { | ||
try (final FileOutputStream out = new FileOutputStream(destinationPath)) { |
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 have IOUtils on classpath, please use IOUtils.copy()
@Test | ||
public void copyToContainerTest() throws Exception { | ||
//compare purpose | ||
File outputFile = new File("src/test/resources/copy-from/test_copy_to_container.txt"); |
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.
output file should not be placed in src/test
, use a temporary directory instead
//compare purpose | ||
File outputFile = new File("src/test/resources/copy-from/test_copy_to_container.txt"); | ||
File currentFile = new File("src/test/resources/test_copy_to_container.txt"); | ||
if(outputFile.exists()){ |
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.
temp dir will help to avoid this one as well
@@ -39,15 +42,14 @@ | |||
private static final String RABBITMQ_TEST_MESSAGE = "Hello world"; | |||
private static final int RABBITMQ_PORT = 5672; | |||
private static final int MONGO_PORT = 27017; | |||
|
|||
private static final File CONTENT_FOLDER = new File(System.getProperty("user.home") + "/.tmp-test-container"); |
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.
Why don't you use something like Files.createTempDirectory()
?
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.
Fixed, my bad
approved, thanks! @rnorth do you have something to add? |
Sorry for being slow to respond - just noticed one more thing: because these methods only work when the container is created/running, we should add guards to check and log a suitable error message. I have the branch locally, so will add that. |
Have rebased and squashed on a separate branch, so will merge from there. |
Merged! Thank you for the contribution @lukidzi ! |
I am not sure if this function will be helpful. This is first concept I need to figure out how to check if path exist, how to get message if couldn't copy, and write some tests. I am tested it and I was able to copy file from my resource directory to container.