-
Notifications
You must be signed in to change notification settings - Fork 1
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
Fix problem if topology isn't applied correctly #70
Conversation
82f9df0
to
56da7f3
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.
Overall LGTM, I have two small questions
@@ -0,0 +1,16 @@ | |||
cartridge = require('cartridge') |
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.
q: what is incorrect in this topology?
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.
There's no server on 3303 port. I added comment in topology
@@ -21,8 +26,8 @@ public class TarantoolCartridgeBootstrapFromLuaWithFixedPortsTest { | |||
"Dockerfile", | |||
"cartridge/instances_fixedport.yml", | |||
"cartridge/topology_fixedport.lua") | |||
.withCopyFileToContainer(MountableFile.forClasspathResource("cartridge"), "/app") | |||
.withCopyFileToContainer(MountableFile.forClasspathResource("cartridge/instances_fixedport.yml"),"/app/instances.yml") | |||
.withDirectoryBinding("cartridge") |
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.
IIRC Vova changed this line because we had some problems with windows (?) or something. Did you look at the history of this change? Is it safe to use binding here? (I know, it's the default preferred variant, but there was some problem)
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.
It's hard to see why it had been changed #46. I asked @dkasimovskiy to try to run tests(he has windows + wsl) but he didn't get what I want and merged PR 🤷♂️
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.
@akudiyar we will test how it's working with windows. You can check other PRs in testcontainers
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.
On the latest WSL2 it works fine.
I haven't forgotten about:
Related issues: