feat: add --image-mount flag to bake images-to-mount init scripts#121
feat: add --image-mount flag to bake images-to-mount init scripts#121feloy wants to merge 2 commits into
Conversation
Add support for loading images-to-mount YAML files and appending their shell init snippets to /sandbox/.bashrc and /sandbox/.zshrc inside the built image. - src/image_mount.rs (new): parse images-to-mount YAML files (local path or URL), derive the mount name from the filename stem, and replace $MOUNT with /sandbox/mnt/<name> in the init value. - src/containerfile.rs: introduce ContainerfileOptions struct to replace the positional argument list in generate(); add image_mount_inits field that emits printf calls appending each resolved init snippet to .bashrc and .zshrc in the same RUN layer that creates the profile files; add init_for_printf() helper that escapes backslashes, newlines, and single quotes for safe use in a single-quoted shell printf argument; add unit tests covering ordering, multiple mounts, single-quote escaping, and the no-mount-no-zshrc invariant. - src/main.rs: add --image-mount <PATH|URL> CLI flag (clap Append action, repeatable); thread image_mounts through run(); add unit tests for single mount, multiple mounts, and invalid path error. - tests/integration_test.rs: add image_mount module with integration tests (marked #[ignore] for those requiring podman) covering .bashrc and .zshrc content, sandbox ownership, and absence of .zshrc when the flag is not used; add non-ignored binary smoke-test for the invalid-path error path; register cleanup of the new test image tag. - README.md: document the new flag — YAML format, $MOUNT substitution rule, files written, CLI examples, and option table entry. Co-authored-by: Claude <claude@anthropic.com> Signed-off-by: Philippe Martin <phmartin@redhat.com>
📝 WalkthroughWalkthroughThe PR adds ChangesImage-mount build path
Estimated code review effort: 4 (Complex) | ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
mount_name() was using rsplit('/') to extract the filename from all
inputs. On Windows, local temp paths use backslash separators, so
the entire path was returned as the "last component" instead of just
the filename, producing broken mount paths like:
/sandbox/mnt/C:\Users\...\curl.yaml
Fix by delegating to std::path::Path::file_name() for local paths,
which handles OS-native separators correctly. URL inputs (http/https)
continue to use rsplit('/') since URL paths always use forward slashes.
Co-authored-by: Claude <claude@anthropic.com>
Signed-off-by: Philippe Martin <phmartin@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main.rs (1)
152-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
run()accepts 12 positional parameters, several of the same type.Adjacent params like
with_policy: bool, with_agent_settings: bool(and now withimage_mountsinserted between other options) make call sites error-prone to read and easy to reorder incorrectly since the compiler won't catch a swap of twobool/Option<&str>args. The PR already introducesContainerfileOptionsfor the siblingcontainerfile::generatecall — applying the same pattern torun()would be more consistent and safer for future flag additions.This would require updating the ~15 test call sites in this same file, so it's a larger change; flagging for awareness rather than as a blocking issue.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main.rs` around lines 152 - 165, run() has too many positional arguments, including multiple adjacent bool and Option<&str> parameters, which makes call sites easy to mix up. Refactor the run() API to take a single options struct, similar to ContainerfileOptions used for containerfile::generate, and update the internal callers and test call sites in main.rs to pass that struct instead of many positional args. Use the run function and the nearby ContainerfileOptions pattern as the main references when locating the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/containerfile.rs`:
- Around line 256-264: The init_for_printf helper currently escapes backslashes,
newlines, and single quotes, but it still leaves percent signs unescaped, which
can break shell printf formatting for user-provided init text. Update
init_for_printf to also escape % by doubling it before wrapping it in the
single-quoted printf string, and add a regression test covering an init value
containing % so the emitted containerfile output stays literal.
In `@src/image_mount.rs`:
- Around line 54-60: The URL branch in load_yaml_content currently uses the
default ureq::get(...).call(), which leaves socket timeouts unset and can block
indefinitely on slow or stalled URLs. Update the HTTP fetch path to use a
ureq::Agent configured with explicit read and write timeouts, then perform the
request through that agent instead of the default client. Keep the file path
branch unchanged and make sure the timeout-configured client is used only in the
HTTP/HTTPS branch of load_yaml_content.
In `@src/main.rs`:
- Around line 223-237: The image mount setup currently allows multiple
`--image-mount` entries to resolve to the same mount name and silently merge
their init snippets. Add a duplicate-name check in the `src/main.rs` flow that
builds `image_mount_inits` before calling `containerfile::generate`, using the
mount-name derivation from `image_mount::mount_name`/`load_init` to detect
collisions and return an error when two entries map to the same mount path. Keep
the fix localized to the image-mount collection path so
`containerfile::generate` only receives unique mount names.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 152-165: run() has too many positional arguments, including
multiple adjacent bool and Option<&str> parameters, which makes call sites easy
to mix up. Refactor the run() API to take a single options struct, similar to
ContainerfileOptions used for containerfile::generate, and update the internal
callers and test call sites in main.rs to pass that struct instead of many
positional args. Use the run function and the nearby ContainerfileOptions
pattern as the main references when locating the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c1ff4a49-4991-42cc-bb9c-784ce7661bab
📒 Files selected for processing (5)
README.mdsrc/containerfile.rssrc/image_mount.rssrc/main.rstests/integration_test.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
| /// Escapes `init` for use as the format string in a shell `printf '...\n'` call. | ||
| /// | ||
| /// - Backslashes are doubled so printf does not interpret them as escape sequences. | ||
| /// - Real newline characters are replaced with `\n` for printf to output as newlines. | ||
| /// - Single quotes are escaped for the surrounding single-quoted shell string. | ||
| fn init_for_printf(init: &str) -> String { | ||
| init.replace('\\', "\\\\") | ||
| .replace('\n', "\\n") | ||
| .replace('\'', "'\\''") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby tests.
ast-grep outline src/containerfile.rs --view expanded || true
echo "---- file size ----"
wc -l src/containerfile.rs
echo "---- relevant lines ----"
sed -n '240,340p' src/containerfile.rs
echo "---- search for init_for_printf and printf usage ----"
rg -n "init_for_printf|printf" src/containerfile.rsRepository: openkaiden/openshell-image-builder
Length of output: 9806
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact generation logic and tests around init formatting.
sed -n '300,380p' src/containerfile.rs
echo "----"
sed -n '1,220p' src/containerfile.rs | rg -n "image_mount|init|printf|test|percent|%" -nRepository: openkaiden/openshell-image-builder
Length of output: 2884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/containerfile.rs")
text = p.read_text()
for needle in ["init_for_printf", "printf", "image_mount_inits", "export ", "%"]:
print(f"== {needle} ==")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
print(f"{i}: {line}")
print()
PYRepository: openkaiden/openshell-image-builder
Length of output: 3948
Escape % in init_for_printf. printf treats % as a format specifier, so user-provided init text like export DATE_FMT=%Y-%m-%d can be rewritten or fail when emitted here. Double % in the escape helper and add a regression test for a %-containing init.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/containerfile.rs` around lines 256 - 264, The init_for_printf helper
currently escapes backslashes, newlines, and single quotes, but it still leaves
percent signs unescaped, which can break shell printf formatting for
user-provided init text. Update init_for_printf to also escape % by doubling it
before wrapping it in the single-quoted printf string, and add a regression test
covering an init value containing % so the emitted containerfile output stays
literal.
| fn load_yaml_content(path_or_url: &str) -> Result<String, Box<dyn std::error::Error>> { | ||
| if path_or_url.starts_with("http://") || path_or_url.starts_with("https://") { | ||
| Ok(ureq::get(path_or_url).call()?.into_string()?) | ||
| } else { | ||
| Ok(std::fs::read_to_string(path_or_url)?) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
git ls-files src/image_mount.rs Cargo.toml Cargo.lock && \
printf '\n--- src/image_mount.rs ---\n' && cat -n src/image_mount.rs && \
printf '\n--- Cargo.toml ---\n' && cat -n Cargo.toml && \
printf '\n--- Cargo.lock ureq entries ---\n' && rg -n '"ureq"|name = "ureq"|version = ' Cargo.lockRepository: openkaiden/openshell-image-builder
Length of output: 14745
🌐 Web query:
ureq 2.12.1 default timeout_read timeout_write into_string response size cap
💡 Result:
In ureq 2.12.1, the timeout and response size limit settings are as follows: Default Timeouts: - Timeout Read (timeout_read): By default, there is no timeout for socket reads; requests may block indefinitely [1]. - Timeout Write (timeout_write): By default, there is no timeout for socket writes; requests may block indefinitely [1]. - Note: A default timeout_connect of 30 seconds was introduced in the 2.12.x series [2]. You can configure read, write, and other timeouts by building an Agent using the AgentBuilder [3][2]. Response Size Cap for into_string: - The into_string method has a hard-coded response size limit of 10 megabytes (10 * 1,024 * 1,024 bytes) [4]. - If a response body exceeds this 10 MB limit, into_string will return an error [5][6][4]. - This limit is implemented to prevent memory exhaustion when reading large, untrusted responses into a String [5][6]. If you need to handle larger responses, you should use methods like into_reader or configure a BodyWithConfig [7][8][4].
Citations:
- 1: https://docs.rs/ureq/2.1.1/ureq/struct.AgentBuilder.html
- 2: https://github.com/algesten/ureq/blob/master/CHANGELOG.md
- 3: https://docs.rs/crate/ureq/2.12.1
- 4: https://rustdocs.webschool.au/src/ureq/response.rs.html
- 5: https://docs.rs/ureq/2.2.0/ureq/struct.Response.html
- 6: https://docs.rs/ureq/2.1.0/ureq/struct.Response.html
- 7: https://docs.rs/ureq/latest/ureq/struct.Body.html
- 8: https://docs.rs/ureq/latest/ureq/struct.BodyWithConfig.html
Add explicit timeouts to the URL fetch
ureq::get(...).call() here uses the default agent, which leaves socket read/write timeouts unset in ureq 2.12.x. A slow or stalled --image-mount <URL> can block the build indefinitely; build an Agent with read/write timeouts instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/image_mount.rs` around lines 54 - 60, The URL branch in load_yaml_content
currently uses the default ureq::get(...).call(), which leaves socket timeouts
unset and can block indefinitely on slow or stalled URLs. Update the HTTP fetch
path to use a ureq::Agent configured with explicit read and write timeouts, then
perform the request through that agent instead of the default client. Keep the
file path branch unchanged and make sure the timeout-configured client is used
only in the HTTP/HTTPS branch of load_yaml_content.
| let image_mount_inits: Vec<String> = image_mounts | ||
| .iter() | ||
| .map(|path_or_url| image_mount::load_init(path_or_url)) | ||
| .collect::<Result<_, _>>()?; | ||
| let output = containerfile::generate( | ||
| &config, | ||
| agent.as_deref(), | ||
| &features, | ||
| has_agent_settings, | ||
| &skill_names, | ||
| &agent_env_vars, | ||
| with_policy, | ||
| &containerfile::ContainerfileOptions { | ||
| agent: agent.as_deref(), | ||
| features: &features, | ||
| with_agent_settings: has_agent_settings, | ||
| skill_names: &skill_names, | ||
| env_vars: &agent_env_vars, | ||
| with_policy, | ||
| image_mount_inits: &image_mount_inits, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
No collision check for duplicate mount names across --image-mount entries.
mount_name derives the mount path solely from the file stem (src/image_mount.rs line 34-52). If two --image-mount paths/URLs share the same stem (e.g. configs/curl.yaml and other/curl.yaml, or a local file plus a URL both named curl.yaml), both resolve to /sandbox/mnt/curl, and their init snippets get silently appended together with no error — even though they describe two different container images. Nothing in this loop (or downstream in containerfile.rs) rejects or warns about the collision.
🐛 Proposed fix: reject duplicate mount names before generation
let image_mount_inits: Vec<String> = image_mounts
.iter()
.map(|path_or_url| image_mount::load_init(path_or_url))
.collect::<Result<_, _>>()?;
+ let mut seen_names = std::collections::HashSet::new();
+ for path_or_url in image_mounts {
+ if let Some(name) = image_mount::mount_name(path_or_url)
+ && !seen_names.insert(name.clone())
+ {
+ return Err(format!("--image-mount: duplicate mount name '{name}' derived from '{path_or_url}'").into());
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let image_mount_inits: Vec<String> = image_mounts | |
| .iter() | |
| .map(|path_or_url| image_mount::load_init(path_or_url)) | |
| .collect::<Result<_, _>>()?; | |
| let output = containerfile::generate( | |
| &config, | |
| agent.as_deref(), | |
| &features, | |
| has_agent_settings, | |
| &skill_names, | |
| &agent_env_vars, | |
| with_policy, | |
| &containerfile::ContainerfileOptions { | |
| agent: agent.as_deref(), | |
| features: &features, | |
| with_agent_settings: has_agent_settings, | |
| skill_names: &skill_names, | |
| env_vars: &agent_env_vars, | |
| with_policy, | |
| image_mount_inits: &image_mount_inits, | |
| }, | |
| let image_mount_inits: Vec<String> = image_mounts | |
| .iter() | |
| .map(|path_or_url| image_mount::load_init(path_or_url)) | |
| .collect::<Result<_, _>>()?; | |
| let mut seen_names = std::collections::HashSet::new(); | |
| for path_or_url in image_mounts { | |
| if let Some(name) = image_mount::mount_name(path_or_url) | |
| && !seen_names.insert(name.clone()) | |
| { | |
| return Err(format!("--image-mount: duplicate mount name '{name}' derived from '{path_or_url}'").into()); | |
| } | |
| } | |
| let output = containerfile::generate( | |
| &config, | |
| &containerfile::ContainerfileOptions { | |
| agent: agent.as_deref(), | |
| features: &features, | |
| with_agent_settings: has_agent_settings, | |
| skill_names: &skill_names, | |
| env_vars: &agent_env_vars, | |
| with_policy, | |
| image_mount_inits: &image_mount_inits, | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main.rs` around lines 223 - 237, The image mount setup currently allows
multiple `--image-mount` entries to resolve to the same mount name and silently
merge their init snippets. Add a duplicate-name check in the `src/main.rs` flow
that builds `image_mount_inits` before calling `containerfile::generate`, using
the mount-name derivation from `image_mount::mount_name`/`load_init` to detect
collisions and return an error when two entries map to the same mount path. Keep
the fix localized to the image-mount collection path so
`containerfile::generate` only receives unique mount names.
1 participant
Add support for loading images-to-mount YAML files and appending their shell init snippets to /sandbox/.bashrc and /sandbox/.zshrc inside the built image.
src/image_mount.rs (new): parse images-to-mount YAML files (local path or URL), derive the mount name from the filename stem, and replace $MOUNT with /sandbox/mnt/ in the init value.
src/containerfile.rs: introduce ContainerfileOptions struct to replace the positional argument list in generate(); add image_mount_inits field that emits printf calls appending each resolved init snippet to .bashrc and .zshrc in the same RUN layer that creates the profile files; add init_for_printf() helper that escapes backslashes, newlines, and single quotes for safe use in a single-quoted shell printf argument; add unit tests covering ordering, multiple mounts, single-quote escaping, and the no-mount-no-zshrc invariant.
src/main.rs: add --image-mount <PATH|URL> CLI flag (clap Append action, repeatable); thread image_mounts through run(); add unit tests for single mount, multiple mounts, and invalid path error.
tests/integration_test.rs: add image_mount module with integration tests (marked #[ignore] for those requiring podman) covering .bashrc and .zshrc content, sandbox ownership, and absence of .zshrc when the flag is not used; add non-ignored binary smoke-test for the invalid-path error path; register cleanup of the new test image tag.
README.md: document the new flag — YAML format, $MOUNT substitution rule, files written, CLI examples, and option table entry.
How to use: