-
Notifications
You must be signed in to change notification settings - Fork 709
Fix Docker-based Development Workflow #1031
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
base: main
Are you sure you want to change the base?
Conversation
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.
Pull request overview
This PR fixes the Docker-based development environment by addressing shell expansion issues, enabling source-based development workflows with cargo run, and updating documentation to guide developers through the setup process.
Key Changes:
- Fixed Alpine shell incompatibility by replacing brace expansion syntax with explicit comma-separated volume lists
- Added a new
devstage in Dockerfile.source to preserve the Rust toolchain for development containers - Created comprehensive development documentation with setup and troubleshooting instructions
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| docs/DEVELOPMENT.md | New comprehensive guide for Docker-based local development workflow |
| docker-compose.yml | Fixed volume environment variables, added dev service configuration with custom command for cargo run, and corrected volume mount paths |
| Dockerfile.source | Added dev stage inheriting from builder and removed reference to non-existent cli/rustfs-gui directory |
| CONTRIBUTING.md | Added reference link to new development guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| If you modify `Cargo.toml` or `Cargo.lock`, you generally need to rebuild the Docker image to update the cached dependencies layer: | ||
|
|
||
| ```bash | ||
| docker compose --profile dev build rustfs-dev | ||
| ``` |
Copilot
AI
Dec 8, 2025
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.
The statement about needing to rebuild the Docker image when modifying Cargo.toml or Cargo.lock is misleading. Since the source code is mounted at /app and the application runs via cargo run, changes to Cargo.toml will be picked up on container restart without needing to rebuild the image. The image rebuild is only necessary if the dependency pre-fetching layer needs to be updated for better caching, but it's not strictly required for the changes to take effect.
|
@YGoetschel Wait until we verify the changes to |
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.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| VOLUME_LIST="" | ||
| for vol in $VOLUME_LIST_RAW; do | ||
| # Helper to manually expand {N..M} since sh doesn't support it on variables |
Copilot
AI
Dec 9, 2025
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.
The comment says "Helper to manually expand {N..M}" but this is somewhat misleading - this is the main implementation for brace expansion, not just a helper. Consider updating the comment to: "Manually expand {N..M} brace notation since sh doesn't support it" for clarity.
| # Helper to manually expand {N..M} since sh doesn't support it on variables | |
| # Manually expand {N..M} brace notation since sh doesn't support it |
| i=$((i+1)) | ||
| done | ||
| else | ||
| # Fallback if not numbers |
Copilot
AI
Dec 9, 2025
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.
[nitpick] The comment "Fallback if not numbers" could be more descriptive. Consider: "Fallback if START or END are not valid numbers - keep the original pattern unchanged".
| # Fallback if not numbers | |
| # Fallback if START or END are not valid numbers - keep the original pattern unchanged |
| The development image uses `entrypoint.sh` to parse the `RUSTFS_VOLUMES` environment variable (supporting `{N..M}` syntax), create the directories, and pass them to `cargo run`. Ensure your `RUSTFS_VOLUMES` variable is correctly formatted. | ||
|
|
||
| ### Slow Initial Build | ||
| This is expected. The `dev` stage in `Dockerfile.source` compiles all dependencies from scratch. Because the `/usr/local/cargo/registry` is mounted as a volume, these compiled artifacts are preserved between restarts, making future builds fast. |
Copilot
AI
Dec 9, 2025
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.
This statement is technically incorrect. The /usr/local/cargo/registry volume preserves the crates.io index and downloaded crate archives, not compiled artifacts. Compiled artifacts are in the target/ directory, which is preserved on the host via the source mount (.:/app). Consider rewording to: "Because the source code (including target/) is mounted from the host and the cargo registry is cached in a volume, both downloaded dependencies and compiled artifacts are preserved between restarts, making future builds fast."
| This is expected. The `dev` stage in `Dockerfile.source` compiles all dependencies from scratch. Because the `/usr/local/cargo/registry` is mounted as a volume, these compiled artifacts are preserved between restarts, making future builds fast. | |
| This is expected. The `dev` stage in `Dockerfile.source` compiles all dependencies from scratch. Because the source code (including `target/`) is mounted from the host and the cargo registry is cached in a volume, both downloaded dependencies and compiled artifacts are preserved between restarts, making future builds fast. |
|
@houseme I have tested the change. Steps is as below
RustFS instance can run and bucket/ak & sk creation works fine. |
| "sh", "-c", | ||
| "curl -f http://localhost:9000/health && curl -f http://localhost:9001/rustfs/console/health" | ||
| ] | ||
| test: [ "CMD", "sh", "-c", "curl -f http://localhost:9000/health && curl -f http://localhost:9001/rustfs/console/health" ] |
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.
The change seems same with old style. In addition, old style is more readable, I think.
test:
[
"CMD",
"sh", "-c",
"curl -f http://localhost:9000/health && curl -f http://localhost:9001/rustfs/console/health"
]
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.
@YGoetschel FYI
| "sh", "-c", | ||
| "curl -f http://localhost:9000/health && curl -f http://localhost:9001/rustfs/console/health" | ||
| ] | ||
| test: [ "CMD", "sh", "-c", "curl -f http://localhost:9000/health && curl -f http://localhost:9001/rustfs/console/health" ] |
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.
Same with above.
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.
@YGoetschel FYI
Summary
This PR overhauls the Docker-based development environment to enable a proper source-based workflow (cargo run) and fixes several configuration issues preventing the containers from starting correctly.
Changes
1. Fix docker-compose.yml Configuration
Fixed Shell Expansion: Replaced bash-style brace expansion ({0..3}) with explicit lists for RUSTFS_VOLUMES. The Alpine-based
sh (dash) shell does not support brace expansion, which caused volume creation failures.
Enabled Source-Based Development: Updated the rustfs-dev service to:
Fixed Startup Command: Implemented a custom command for rustfs-dev to manually create data directories and pass them as arguments to cargo run. This bypasses the entrypoint.sh limitation when running cargo directly.
Syntax Fixes:
Removed obsolete version field and fixed invalid depends_on references for optional profiles. Corrected relative volume paths to be explicit bind mounts (e.g., ./deploy/...).
2. Update Dockerfile.source
3. Documentation
Added docs/DEVELOPMENT.md: A comprehensive guide on how to set up, run, and debug the docker-based development environment, including instructions for the Console UI.
Updated CONTRIBUTING.md to link to the new development guide.
Verification
Automated Checks
Manual Verification
Startup: Ran docker compose --profile dev up rustfs-dev. Container built and started successfully.
Functionality:
Related Issues