Skip to content
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

Move rocks building in build phase #68

Merged
merged 3 commits into from Jun 2, 2023
Merged

Move rocks building in build phase #68

merged 3 commits into from Jun 2, 2023

Conversation

ArtDu
Copy link
Contributor

@ArtDu ArtDu commented Apr 25, 2023

I haven't forgotten about:

  • Tests
  • Changelog
  • Documentation
  • Commit messages comply with the guideline
  • Cleanup the code for review. See checklist

Related issues:

@ArtDu
Copy link
Contributor Author

ArtDu commented Apr 26, 2023

Or we can use multistage builds testcontainers/testcontainers-java#4810

@ArtDu ArtDu force-pushed the rocks_in_image branch 2 times, most recently from 2f779f2 to aaf1cb5 Compare May 5, 2023 14:28
@ArtDu ArtDu marked this pull request as ready for review May 5, 2023 14:28
iDneprov
iDneprov previously approved these changes May 5, 2023
Copy link
Contributor

@iDneprov iDneprov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ArtDu ArtDu requested a review from akudiyar May 5, 2023 15:40
@ArtDu ArtDu self-assigned this May 5, 2023
Copy link
Collaborator

@akudiyar akudiyar left a 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")
Copy link
Collaborator

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

Copy link
Collaborator

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

Copy link
Collaborator

@akudiyar akudiyar May 29, 2023

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.

Copy link
Contributor Author

@ArtDu ArtDu Jun 1, 2023

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

@ArtDu ArtDu requested a review from akudiyar June 1, 2023 14:41
@dkasimovskiy dkasimovskiy merged commit d6f21a6 into master Jun 2, 2023
1 check passed
@dkasimovskiy dkasimovskiy deleted the rocks_in_image branch June 2, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants