Skip to content

Fix Docker data directory permissions#1910

Open
snvtac wants to merge 1 commit into
iii-hq:mainfrom
snvtac:snvtac/1883-docker-compose-data-volume
Open

Fix Docker data directory permissions#1910
snvtac wants to merge 1 commit into
iii-hq:mainfrom
snvtac:snvtac/1883-docker-compose-data-volume

Conversation

@snvtac

@snvtac snvtac commented Jun 25, 2026

Copy link
Copy Markdown

What

  • Pre-create /app/data and /data as UID/GID 65532 in the published distroless engine image.
  • Mirror the Docker template fixture to mount iii_data at /app/data.
  • Add project-init e2e assertions so generated Docker assets keep the writable data volume.
  • Drop the stale 9464 exposure from the engine image and generated Docker fixture.

Why

Fixes #1883. On Linux, iii project init --docker && docker compose up can crash-loop because the configuration worker defaults to ./data/configuration under /app, while the image runs as non-root UID 65532 and has no writable data directory.

Notes

  • I license my contributions to this repository under Apache 2.0, and I have all necessary rights over the code I am contributing.

  • The live generated template is fetched from iii-hq/templates; companion template PR: Fix Docker template data volume templates#31

  • Validation run: rustfmt --check engine/tests/project_init_e2e.rs, YAML parse for the compose fixture, semantic check for iii_data:/app/data, and diff whitespace check.

  • Full cargo test / Docker smoke were not run here: full Git clone repeatedly stalled in this environment, and Docker CLI is unavailable.

@vercel

vercel Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

@snvtac is attempting to deploy a commit to the motia Team on Vercel.

A member of the Team first needs to authorize it.

@iii-hq-ci

iii-hq-ci Bot commented Jun 25, 2026

Copy link
Copy Markdown

License agreement recorded

@snvtac, your agreement has been recorded and you have been added to contributors.md. All future PRs from your account will pass this check automatically.

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The Docker image now pre-creates writable data directories for the non-root runtime. The generated Docker Compose template mounts a named iii_data volume at /app/data, and the Docker-related end-to-end tests assert the updated compose contents.

Changes

Writable Docker data path

Layer / File(s) Summary
Image precreates data directories
engine/Dockerfile, engine/tests/fixtures/templates/docker/Dockerfile
The Dockerfile adds a busybox stage that creates /app/data and /data, copies them into the final nonroot image with ownership preserved, and removes port 9464 from EXPOSE in the runtime Dockerfile and fixture.
Compose mounts data volume and asserts it
engine/tests/fixtures/templates/docker/docker-compose.yml, engine/tests/project_init_e2e.rs
The Docker Compose fixture adds a named iii_data mount at /app/data, declares the top-level volume, and the Docker init/generate tests assert those compose entries through a shared helper.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • guibeira

Poem

🐰 I dug a little data burrow deep,
and made the Docker bunny’s home to keep.
With iii_data snug and ports now trim,
the startup hop is tidy, brisk, and slim.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change: fixing Docker data directory permissions.
Linked Issues check ✅ Passed The Dockerfile, compose fixture, and tests implement the writable /app/data volume and ownership required by #1883.
Out of Scope Changes check ✅ Passed The 9464 port cleanup is called out in the issue, and the rest of the changes stay focused on the Docker data fix.
Description check ✅ Passed The PR description matches the required What/Why/Notes template and includes the key implementation and validation details.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@snvtac snvtac force-pushed the snvtac/1883-docker-compose-data-volume branch from 1330221 to 55f4176 Compare June 25, 2026 17:32

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
engine/tests/project_init_e2e.rs (1)

25-35: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Also lock the Dockerfile 9464 removal in these e2e checks.

This helper covers the new compose-volume contract, but the PR also changes the generated Dockerfile to drop 9464. Neither e2e path asserts that, so the stale expose can come back without failing these tests.

Suggested shape
 fn assert_compose_mounts_engine_data_volume(project: &Path) {
     let compose = std::fs::read_to_string(project.join("docker-compose.yml")).unwrap();
     assert!(
         compose.contains("- iii_data:/app/data"),
         "docker-compose.yml should mount writable engine data, got:\n{compose}"
     );
     assert!(
         compose.contains("volumes:\n  iii_data:"),
         "docker-compose.yml should declare the iii_data named volume, got:\n{compose}"
     );
 }
+
+fn assert_dockerfile_drops_9464(project: &Path) {
+    let dockerfile = std::fs::read_to_string(project.join("Dockerfile")).unwrap();
+    assert!(
+        !dockerfile.contains("9464"),
+        "Dockerfile should not expose 9464, got:\n{dockerfile}"
+    );
+}
     assert_compose_mounts_engine_data_volume(dir.path());
+    assert_dockerfile_drops_9464(dir.path());
🤖 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 `@engine/tests/project_init_e2e.rs` around lines 25 - 35, The e2e coverage in
assert_compose_mounts_engine_data_volume only checks the compose volume contract
and does not guard the generated Dockerfile change. Update the project init e2e
assertions to also verify the Dockerfile no longer exposes 9464, using the
existing project initialization test helpers and the generated Dockerfile
content so the stale port cannot regress unnoticed.
🤖 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 `@engine/Dockerfile`:
- Line 1: The BusyBox base image used in the data-dirs stage is still referenced
by a mutable tag, so update the FROM instruction in the data-dirs stage to use
an immutable sha256 digest instead of busybox:1.36.1. Keep the stage name and
overall Dockerfile structure the same, and replace the tag with the pinned
digest so builds remain reproducible.

---

Nitpick comments:
In `@engine/tests/project_init_e2e.rs`:
- Around line 25-35: The e2e coverage in
assert_compose_mounts_engine_data_volume only checks the compose volume contract
and does not guard the generated Dockerfile change. Update the project init e2e
assertions to also verify the Dockerfile no longer exposes 9464, using the
existing project initialization test helpers and the generated Dockerfile
content so the stale port cannot regress unnoticed.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: accbbc30-3216-4b02-a196-ac997b894b57

📥 Commits

Reviewing files that changed from the base of the PR and between c73bc57 and 1330221.

📒 Files selected for processing (4)
  • engine/Dockerfile
  • engine/tests/fixtures/templates/docker/Dockerfile
  • engine/tests/fixtures/templates/docker/docker-compose.yml
  • engine/tests/project_init_e2e.rs

Comment thread engine/Dockerfile
@@ -1,12 +1,18 @@
FROM busybox:1.36.1 AS data-dirs

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟡 Minor

Pin the new BusyBox stage to an immutable digest.

The busybox:1.36.1 tag is mutable; pin the reference to a sha256 digest to ensure build reproducibility and prevent supply-chain risks where the image content changes without a code change.

🤖 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 `@engine/Dockerfile` at line 1, The BusyBox base image used in the data-dirs
stage is still referenced by a mutable tag, so update the FROM instruction in
the data-dirs stage to use an immutable sha256 digest instead of busybox:1.36.1.
Keep the stage name and overall Dockerfile structure the same, and replace the
tag with the pinned digest so builds remain reproducible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docker compose up crash-loops on Linux: configuration worker can't create ./data (permission denied)

3 participants