-
-
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
use copyFileToContainer where possible #829
Conversation
@@ -448,6 +460,14 @@ ExecResult execInContainer(Charset outputCharset, String... command) | |||
*/ | |||
void copyFileFromContainer(String containerPath, String destinationPath) throws IOException, InterruptedException; | |||
|
|||
/** | |||
* Copies a file which resides inside the container to user defined directory |
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.
javadoc is misleading.
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.
Nice catch, thanks! Fixed.
@@ -1018,34 +1024,95 @@ public ExecResult execInContainer(String... command) | |||
* {@inheritDoc} | |||
*/ | |||
@Override | |||
public void copyFileToContainer(MountableFile mountableLocalFile, String containerPath) { | |||
@SneakyThrows(IOException.class) | |||
public void copyFileToContainer(MountableFile mountableFile, 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.
both functions do almost exactly the same. Can those two be merged? (Can't you just call the othercopyFileToContainer
function? MountableFile
implements Transferable
0
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.
👍
…tableFile`-based one, fix `CopyFileToContainerTest`
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.
CopyFileToContainerFolderTest
and most of the tests in FileOperationsTest
are failing on Windows 10 (I assume some path stuff, I try to investigate).
Possibly JDBC driver tests are broken as well. Let's give us some time before merging.
Also rename variables in copyFileToContainer() for better readability.
We have this out in a Release Candidate build (1.9.0-rc1) for anyone who is keen to try it! |
Since volume mappings work only with local-ish Docker environment, there is a low hanging fruit to replace configs and small file operations with copyToContainer which uses TarInputStream instead.
It makes it possible to run the tests against remote envs with some file operations.
P.S. it seems that copyToContainer was incorrectly implemented before (containerPath was used as a target directory when file is mounted), this PR deprecates that behaviour (with a warning)