-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(docker): add initialize-onchain to copy files to docker volume #8
Conversation
WalkthroughThe changes introduce a new service called Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
docker/sui/compose.yaml
(3 hunks)
🔇 Additional comments (3)
docker/sui/compose.yaml (3)
74-74
: LGTM! Changes address permission issues effectively.
The modifications correctly:
- Use a named volume instead of direct mounting
- Ensure proper initialization sequence through service dependency
Also applies to: 80-81
241-241
: LGTM! Volume declaration is appropriate.
The simple volume declaration is sufficient for this use case and follows the same pattern as other volumes in the file.
22-30
: Verify development workflow impact.
While this solution fixes the permission issues, it introduces a one-way file synchronization that developers should be aware of:
- Changes in the local
onchain
directory will require container restart to be reflected in the volume - Changes made inside the container won't persist in the local directory
Consider documenting these behaviors in the repository's developer guide.
Let's verify the file synchronization behavior:
Also applies to: 74-74, 80-81
✅ Verification successful
Development workflow impact is minimal and well-handled
The one-way file synchronization design is appropriate for this use case:
- The
onchain
directory is only used for package publishing, not active development - Write operations only occur during package deployment via
publish_package.sh
- No development workflows require two-way sync, as verified in the test scripts and documentation
The current implementation correctly handles the intended workflow where source files are copied into the container for deployment purposes only.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any write operations to the onchain directory
# that might need two-way sync
# Look for potential write operations in the codebase
rg -g '!docker' -l 'onchain.*write|onchain.*create|onchain.*save' .
# Check for any shell scripts that might modify onchain files
fd -e sh -x grep -l 'onchain' {}
Length of output: 181
Script:
#!/bin/bash
# Let's check the content of these files to understand the write operations
echo "=== README.md mentions of onchain ==="
rg "onchain.*write|onchain.*create|onchain.*save" README.md
echo -e "\n=== oneclick-test.sh onchain operations ==="
cat e2e_tests/oneclick-test.sh
echo -e "\n=== publish_package.sh onchain operations ==="
cat docker/sui/bin/publish_package.sh
# Let's also check if there are any development instructions or documentation
echo -e "\n=== Development related content in README ==="
rg -A 5 -B 5 "develop|setup|install" README.md
Length of output: 6244
Description
Fix Permission Denied Error for sui move test Execution
Resolved a "permission denied" issue occurring during sui move test by updating the Docker build process. Instead of directly mounting the project folder, files are now copied to a Docker volume, preventing permission changes that impacted local development.
Fixes
#7
Summary by CodeRabbit
New Features
initialize-onchain
, to enhance data management during startup.onchain
for improved service functionality.Improvements
publish-package
service to utilize the newonchain
volume, streamlining the process.