-
Notifications
You must be signed in to change notification settings - Fork 1
Move rocks building in build phase #68
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
Conversation
Or we can use multistage builds testcontainers/testcontainers-java#4810 |
2f779f2
to
aaf1cb5
Compare
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.
LGTM
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.
Looks good, let's check that the new code works on Windows too (with a VM with shared folders)
@@ -21,8 +21,7 @@ public class TarantoolCartridgeBootstrapFromLuaWithFixedPortsTest { | |||
"Dockerfile", | |||
"cartridge/instances_fixedport.yml", | |||
"cartridge/topology_fixedport.lua") | |||
.withCopyFileToContainer(MountableFile.forClasspathResource("cartridge"), "/app") |
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 leave this since on Windows mapping folders may not work. These lines were added for a purpose. We need to check that the new code will work on all platforms
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.
Probably we need to use copy on Windows platform in the container code too, and then remove it here
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.
Another option is to make copy optional, and pass a corresponding flag to the container setup. That looks like the most universal solution.
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've already tested it here #70 (comment) . But I removed binding here at all
492559a
to
b9dcdbe
Compare
I haven't forgotten about:
Related issues: