feat(compose): add --configs-path and --ignore-volume-source overrides - #555
feat(compose): add --configs-path and --ignore-volume-source overrides#555smrutisenapati wants to merge 4 commits into
Conversation
|
🤖 Pull Request Artifacts (#32297167387) 🎉 |
There was a problem hiding this comment.
Pull request overview
Adds a --configs-path override to the rio compose workflow so users can redirect the host-side bind mount that normally sources from /opt/rapyuta/configs, without changing container-side mount paths.
Changes:
- Added
--configs-pathflag torio compose generateandrio compose up, passing the override through to compose generation. - Reworked default volume mount handling to support a configurable host-side configs directory (
get_default_volume_mounts,CONFIGS_DIR). - Updated compose population logic to rewrite host-side
volumes[].subPathentries that are under/opt/rapyuta/configs.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
riocli/compose/up.py |
Adds --configs-path option and forwards it into compose generation. |
riocli/compose/generate.py |
Adds --configs-path option and threads it through generate_compose_file() into populate(). |
riocli/compose/populate.py |
Implements host-path substitution for configs mounts and applies it to default/custom mounts and fixperms volume handling. |
riocli/compose/defaults.py |
Introduces CONFIGS_DIR and a helper to generate default mounts with an optional host-side override. |
Comments suppressed due to low confidence (1)
riocli/compose/populate.py:83
- With the new --configs-path override, host-side volume strings may contain additional ':' segments (notably Windows drive letters like "C:/..."). The current volume-target detection uses vol.split(":")[1], which mis-parses such mounts and can prevent init-fixperms dependencies from being applied. Parse from the right (rsplit) so the target path is correctly extracted regardless of ':' in the source.
fixup_vols = get_volumes_requiring_fixup(processed_deployments, configs_path)
if fixup_vols:
fix_cmds = [_build_fixup_cmd(entry) for entry in fixup_vols]
fixperms_vols = [
f"{entry['host']}:{entry['container']}:rw" for entry in fixup_vols
fea87bd to
722d6ff
Compare
smrutisenapati
changed the title
smrutisenapati
changed the title
ankitrgadiya
left a comment
ankitrgadiya
left a comment
There was a problem hiding this comment.
Verification
Ran in a clean worktree at f5b7199:
uv run pytest tests/unit/— 246 passeduv run ruff check ./ruff format --check .— cleanrio compose generate --help— the long--ignore-volume-sourcestring and the backslash-continued docstring examples render fine- End-to-end
rio compose generateagainst a hand-written Package + Deployment with--configs-pathand--ignore-volume-source: default mount rewritten, customsubPaths rewritten, the ignored bind dropped,/var/spool/print/csvleft alone. Behaves as the description says. - Probed
_get_volume_targetdirectly against 8 volume-string shapes — it returns the container path correctly for every shapebuild_volume_mountscan emit.
Blocking
- With
--configs-path,init-fixpermsnow runschown -R/chmodas root against the developer's local directory (populate.py:88). Verified by generating the compose file and reading the emittedinit-fixpermsservice. Either skip the fixups whenconfigs_pathis set, or say so in the flag help — right now nothing warns the user. rio compose downdid not get the two new flags, so the third caller ofgenerate_compose_file()can write a compose file with un-overridden/opt/rapyuta/configspaths (up.py:157).
Question
Is Windows support planned for rio compose? It decides whether _get_volume_target is load-bearing or dead defensive code — see the comment on populate.py:163.
Please confirm before merge
- A
subPathwith no local counterpart._build_fixup_cmd's own docstring documents that Docker pre-creates a directory at a missing bind source. With--configs-paththat now happens inside the user's tree. Please runrio compose up --configs-path <dir>where one declaredsubPathis absent locally, and confirm what ends up on disk.
| configs_path=configs_path.as_posix() if configs_path else None, | ||
| ignore_volume_source=ignore_volume_source, |
There was a problem hiding this comment.
issue (blocking): rio compose down is the third caller of generate_compose_file() and did not get either flag.
The description gives "it calls the shared generate_compose_file() to regenerate the compose doc on every invocation" as the reason up needed the same treatment. down.py:111 shares that property — it regenerates and then write_compose_yamls the result whenever the compose file is missing or empty — but it still calls generate_compose_file() with only ctx/values/secrets/files.
Concrete effect: rio compose down in a directory with no docker-compose.yaml writes one whose binds all point at /opt/rapyuta/configs, regardless of what the services were actually started with. Anything run against that file afterwards by hand — docker compose -f docker-compose.yaml ps/up/logs — uses the device paths rather than the local ones.
To be straight about the severity: I could not make down itself misbehave, because docker compose down identifies containers by project and service name rather than by volume, and up regenerates unconditionally. So this is the flag surface and the file it leaves on disk, not the teardown. Adding the two options to down and forwarding them keeps the three entry points consistent.
There was a problem hiding this comment.
Resolved. Both flags are on down now and reach generate_compose_file() at down.py:142-143.
The help text divergence is a good call — saying "only takes effect when the compose file has to be (re)generated" on down rather than copying up's wording sets the right expectation, since the regeneration path is conditional there.
One gap left, non-blocking: nothing tests this pass-through. Commented separately at down.py:142.
| fixup_vols = get_volumes_requiring_fixup( | ||
| processed_deployments, configs_path, ignore_volume_source | ||
| ) |
There was a problem hiding this comment.
issue (blocking): With --configs-path, init-fixperms runs chown -R and chmod as root against the developer's local directory.
Threading configs_path into get_volumes_requiring_fixup rewrites entry["host"], so the init-fixperms service (user: "0:0") binds the local path instead of the device's /opt/rapyuta/configs. Generated from a manifest declaring subPath: /opt/rapyuta/configs/wms/settings.yaml, uid: 1000, gid: 1000, perm: 644 with --configs-path ./local-configs:
init-fixperms:
user: 0:0
command:
- sh
- -c
- if [ -f /app/settings.yaml ]; then chown 1000:1000 /app/settings.yaml && chmod
644 /app/settings.yaml; else mkdir -p /app/settings.yaml && chown -R 1000:1000
/app/settings.yaml && chmod 644 /app/settings.yaml; fi
volumes:
- <abs>/local-configs/wms/settings.yaml:/app/settings.yaml:rwBind mounts share the inode, so rio compose up --configs-path ./local-configs rewrites ownership and mode of the developer's own files. Point the flag at a git checkout of config templates and the checkout comes back owned by 1000:1000 (or 0:0, for a manifest declaring uid: 0) and needs sudo to edit again. Worse when a declared subPath has no local counterpart: per _build_fixup_cmd's docstring, Docker pre-creates a directory at the missing bind source, and the else branch then chown -Rs it — so root-owned junk directories appear inside the local config tree.
On a real device that side effect is the point. On a developer's laptop it is not, and neither the flag help nor the PR description mentions it. Two options that both work: skip the fixups entirely when configs_path is set (the local files are the developer's to own), or keep them and say so in the --configs-path help so the choice is informed.
There was a problem hiding this comment.
Resolved. get_volumes_requiring_fixup now skips volumes whose subPath is under CONFIGS_DIR when configs_path is set (populate.py:271-272), so no fixup entry is produced and init-fixperms never binds the local tree. test_service_with_configs_path_redirected_volume_has_no_depends_on pins both halves — no init-fixperms service, and no depends_on edge on the consuming service.
Documenting the skip in the --configs-path help was the right move, since it makes the trade visible: redirected volumes get no permission fixup at all, so a subPath that doesn't exist locally leaves the root-owned directory Docker creates during bind setup uncorrected. I flagged that in the summary as the one thing left to confirm with a live daemon — I think it's the better failure mode than chowning a developer's files, just want it to be a deliberate choice.
| """ | ||
| # Start with default volume mounts | ||
| service_volumes = DEFAULT_VOLUME_MOUNTS.copy() | ||
| service_volumes = get_default_volume_mounts(configs_path) |
There was a problem hiding this comment.
suggestion: --ignore-volume-source silently cannot drop any of the five default mounts.
get_default_volume_mounts() never sees ignore_volume_source, so the patterns only ever apply to deployment-declared volumes. test_default_top_level_mount_unaffected_by_ignore pins that for /opt/rapyuta/configs and the reasoning there is sound — but the same is true of /var/log/riouser, /var/log/rapyuta/deployments, /var/lib/docker/containers and /dev, and those are exactly the paths the flag's stated purpose ("paths that have no local equivalent") describes on a dev machine. --ignore-volume-source '/var/lib/docker/containers' is accepted and does nothing.
The help at generate.py:94 says "matched against a volume's full host-side path -- drops the bind entirely" with no scoping caveat, so there is nothing to tell the user which volumes are eligible. Either filter the defaults through _is_ignored_volume_source too, or add "applies to volumes declared by a deployment; the default mounts are not affected" to the help.
There was a problem hiding this comment.
Addressed. build_volume_mounts now runs the non-configs defaults through _is_ignored_volume_source (populate.py:483-490), so /var/log/riouser, /var/log/rapyuta/deployments, /var/lib/docker/containers and /dev are droppable. Confirmed by probe: --ignore-volume-source '/var/log/*' drops /var/log/riouser.
Leaving the whole-tree CONFIGS_DIR bind to --configs-path instead, and saying so in the help, resolves the ambiguity I was actually complaining about. Good.
One follow-up on how it's implemented — commented at populate.py:484.
| help="gitignore-style pattern matched against a volume's full host-side path -- drops " | ||
| "the bind entirely instead of mounting it. Repeatable; evaluated in order, last match " | ||
| "wins; prefix with '!' to re-include a path an earlier pattern excluded (e.g. " | ||
| "--ignore-volume-source '/opt/rapyuta/configs/station/*' --ignore-volume-source " | ||
| "'!/opt/rapyuta/configs/station/sim-nginx.conf.template'). Independent of --configs-path " | ||
| "-- applies whether or not that flag is also given.", |
There was a problem hiding this comment.
suggestion (non-blocking): Say that patterns match the manifest's subPath, not the rewritten path.
_substitute_configs_path checks the ignore patterns before applying configs_path, so a pattern must be written against the path as the manifest declares it (/opt/rapyuta/configs/auth/*) even when --configs-path ./local-configs is in play. "a volume's full host-side path" reads the other way — with --configs-path given, the host side of the generated bind is ./local-configs/auth/..., and a pattern written against that never matches.
The examples happen to show the right form, but only implicitly. One clause — "matched against the volume's subPath as declared in the manifest, before any --configs-path rewrite" — makes it explicit. Same in up.py:87.
There was a problem hiding this comment.
Resolved. "as declared in the manifest's subPath (before any --configs-path rewrite)" is exactly the disambiguation I was after, and it's consistent across generate.py:101, up.py:90, and down.py.
| def _get_volume_target(vol: str) -> str | None: | ||
| """Extracts the container-side mount path from a compose volume string. | ||
|
|
||
| Parses from the right so host paths containing extra ':' (e.g. Windows | ||
| drive letters like "C:/...") don't shift the field positions. The trailing | ||
| segment is treated as a mode ("rw", "ro", "rslave", ...) rather than the | ||
| container path whenever it doesn't look like an absolute path. | ||
| """ | ||
| parts = vol.split(":") | ||
| if len(parts) < 2: | ||
| return None | ||
| if len(parts) >= 3 and not parts[-1].startswith("/"): | ||
| return parts[-2] | ||
| return parts[-1] |
There was a problem hiding this comment.
question: Is Windows support planned for rio compose? If not, what does this function protect against?
The docstring gives Windows drive letters as the motivation, and that is the only case where it differs from the vol.split(":")[1] it replaces. I checked every volume-string shape build_volume_mounts can emit on a POSIX host and the two parses agree on all of them:
| volume string | _get_volume_target |
vol.split(":")[1] |
|---|---|---|
/host:/container |
/container |
/container |
/host:/container:rw |
/container |
/container |
myvol:/data:ro |
/data |
/data |
/data |
None |
None |
C:/local/x:/container:rw |
/container |
/local/x |
Only the last row diverges. The nearest POSIX equivalent is a host path containing a literal colon — legal on Linux, and newly reachable now that --configs-path lets the user name any directory — but Compose's short volume syntax cannot express it either way, so it is not really a case this rescues.
So: if Windows is on the roadmap, this is groundwork and worth keeping (please say so in the docstring, and the .as_posix() calls at generate.py:189 / up.py:157 become part of the same story). If it is not, then the init-fixperms wiring bug being fixed here is unreachable, and the change is hardening with no failure behind it — still fine to keep, but the docstring should not imply otherwise.
There was a problem hiding this comment.
Answered, and the docstring is better for it. Dropping the Windows framing for "a host path containing a literal :" is the honest motivation, and --configs-path genuinely makes it reachable:
--configs-path /home/dev/my:configs
→ /home/dev/my:configs:/opt/rapyuta/configs:rslave
_get_volume_target → /opt/rapyuta/configs ✓
vol.split(":")[1] → configs ✗
So the fix guards a real case on the platform the project does target. No further action.
| isinstance(vol, str) | ||
| and len(vol.split(":")) >= 2 | ||
| and vol.split(":")[1] in affected_paths | ||
| isinstance(vol, str) and _get_volume_target(vol) in affected_paths |
There was a problem hiding this comment.
todo (non-blocking): Nothing tests the init-fixperms depends_on wiring.
No test under tests/unit/compose/ references _get_volume_target, and none asserts that a service declaring a uid/gid/perm volume ends up with depends_on: {init-fixperms: service_completed_successfully}. So this whole block is unverified by the suite — break the match here and all 246 tests still pass, while containers start before the permission fixup has run.
The 248 new lines in test_populate.py all exercise the helpers directly; one test that drives populate() end to end and asserts the depends_on edge would cover the part with real consequences.
There was a problem hiding this comment.
Resolved, and verified as real coverage. I re-ran the same mutation that motivated this comment — broke the _get_volume_target(vol) in affected_paths match in the wiring loop — and TestPopulateFixpermsDependsOnWiring::test_service_declaring_fixup_volume_depends_on_init_fixperms fails with KeyError: 'init-fixperms'. At the old head that break kept all 246 tests green. Thanks for driving populate() end to end rather than asserting on the helper.
Adds --configs-path to rio compose generate/up/down, letting users bind-mount a local directory in place of /opt/rapyuta/configs on the host side of generated volume mounts. Adds --ignore-volume-source (repeatable, gitignore-style) to drop specific binds entirely for paths with no local equivalent, independent of --configs-path. Volumes redirected under --configs-path are skipped by the init-fixperms permission fixup, since they now point at the developer's own local files rather than a device path. Squashed from the PR's individual commits for a single clean rebase onto devel; see rapyuta-io-cli#555 for the full review history. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
f5b7199 to
9042f1e
Compare
ankitrgadiya
left a comment
ankitrgadiya
left a comment
There was a problem hiding this comment.
Re-review at 9042f1e
The branch was force-pushed: the four commits I reviewed are squashed into one and rebased from 337390561e3b onto 18946e8354f2. devel itself reworked compose in that window (populate.py +116, test_populate.py +356 — the named_volumes/CLOUD_RUNTIME/spinner work), so I diffed each revision against its own merge base rather than f5b7199..HEAD, and re-read the integration points with that new code.
All six findings from the last round are addressed. Status is threaded onto each original comment. Two are worth calling out as genuine fixes rather than restatements:
- The
depends_onwiring is now real coverage. Breaking the_get_volume_targetmatch in the wiring loop kept all 246 tests green at the old head; at this head it failstest_service_declaring_fixup_volume_depends_on_init_fixpermswithKeyError: 'init-fixperms'. - The colon-in-host-path motivation for
_get_volume_targetnow holds up. With--configs-path /home/dev/my:configs, the generated string is/home/dev/my:configs:/opt/rapyuta/configs:rslave; the new parse returns/opt/rapyuta/configs, the oldvol.split(":")[1]returnedconfigs. The flag made a previously-unreachable case reachable, which is a better answer than the Windows framing.
Verification at this head
uv run pytest tests/unit/— 318 passed in 3.20suv run ruff check ./ruff format --check .— clean, 287 files formatted- All three
generate_compose_file()callers now pass both kwargs (generate.py:197,up.py:162,down.py:142) - Probed
_is_ignored_volume_sourceover 7 pattern shapes: the sibling-prefix guard is correct (/opt/rapyuta/configs/stationdoes not match/opt/rapyuta/configs/stationary/x), negation re-includes, and default mounts are now reachable by glob.fnmatch's*crosses/unlike strict gitignore, but the directory-prefix rule already drops the subtree, so the outcome matches intent — not raising it.
Two new comments, neither blocking. Both are test-coverage gaps I confirmed by mutation, not by reading: removing the down.py pass-through, and reordering the defaults list, each leave all 318 tests green.
Still yours to confirm before merge: docker compose up --configs-path <dir> where a declared subPath does not exist locally. Finding 2's fix means redirected volumes are now skipped by init-fixperms entirely, so nothing corrects the root-owned directory Docker auto-creates at a missing subPath — the gap _build_fixup_cmd's docstring documents. The --configs-path help warns about the skip, which I think is the right trade, but please confirm the failure mode is tolerable on a dev machine. Needs a Docker daemon, which I don't have here.
| configs_path=configs_path.as_posix() if configs_path else None, | ||
| ignore_volume_source=ignore_volume_source, |
There was a problem hiding this comment.
todo (non-blocking): The fix for the blocking finding is the only new wiring here with no test.
I deleted both kwargs from this call — reproducing exactly the bug the last round reported — and ran the full suite: 318 passed. So a future refactor can silently undo this and CI stays green.
test_up.py's TestUpCommandIgnoreVolumeSource::test_ignore_volume_source_accepted_without_configs_path is a ready-made template: patch riocli.compose.down.generate_compose_file, invoke down in a tmp_path with no docker-compose.yaml so the regeneration branch runs, and assert on call_args.kwargs. tests/unit/compose/test_down.py already exists to put it in.
| service_volumes = service_volumes[:1] + [ | ||
| vol | ||
| for vol in service_volumes[1:] | ||
| if not _is_ignored_volume_source( | ||
| vol.split(":", 1)[0], ignore_volume_source | ||
| ) | ||
| ] |
There was a problem hiding this comment.
suggestion (non-blocking): [:1] ties the flag's semantics to the ordering of the get_default_volume_mounts() literal, and no test pins that ordering.
The slice is correct today only because the CONFIGS_DIR entry happens to be first in defaults.py:21-27. I moved /var/log/riouser ahead of it and ran the suite: 318 passed — while --configs-path /local --ignore-volume-source /local then silently drops the entire configs bind:
mounts: ['/var/log/riouser:...', '/var/log/rapyuta/deployments:...',
'/var/lib/docker/containers:...', '/dev:...']
configs bind present: False
test_default_top_level_mount_unaffected_by_ignore doesn't catch it, because its patterns are written against /opt/rapyuta/configs/... while the filter matches the rewritten host side (/local) — so the assertion passes whichever entry the slice happened to protect.
Selecting the exempt mount by what it is rather than where it sits would be order-independent and self-documenting, reusing the helper already in this file:
| service_volumes = service_volumes[:1] + [ | |
| vol | |
| for vol in service_volumes[1:] | |
| if not _is_ignored_volume_source( | |
| vol.split(":", 1)[0], ignore_volume_source | |
| ) | |
| ] | |
| if ignore_volume_source: | |
| service_volumes = [ | |
| vol | |
| for vol in service_volumes | |
| if _get_volume_target(vol) == CONFIGS_DIR | |
| or not _is_ignored_volume_source( | |
| vol.split(":", 1)[0], ignore_volume_source | |
| ) | |
| ] |
build_volume_mounts protected the /opt/rapyuta/configs default bind from --ignore-volume-source by keeping service_volumes[:1], relying on it being first in get_default_volume_mounts()'s list. Reordering that list would silently let --ignore-volume-source drop the whole configs bind. Select it by _get_volume_target(vol) == CONFIGS_DIR instead, so it's identified by what it is rather than where it sits. Also adds down.py's missing --configs-path/--ignore-volume-source pass-through test, mirroring TestUpCommandIgnoreVolumeSource. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
init-fixperms skips volumes redirected under --configs-path (so it doesn't chown/chmod a developer's own local files), but that means nothing corrects Docker's behavior when a bind-mount source doesn't exist: it auto-creates an empty directory there, even where the manifest means a file, and the container fails with a confusing "Is a directory" error. Warn at generate time instead, listing exactly which locally-missing paths would hit this, so a forgotten/misplaced local file surfaces clearly up front rather than as a container-side failure. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…s missing" This reverts commit bc61bcb.
Summary
--configs-pathtorio compose generateandrio compose up, letting users bind-mount a local directory in place of/opt/rapyuta/configson the host side of generated volume mounts (both the default volume mount and any deploymentvolumes[].subPathunder that prefix).--ignore-volume-source(repeatable, gitignore-style, matched against a volume's full host-side path) to drop specific binds entirely instead of mounting them, for paths that have no local equivalent. Patterns evaluate in order with last-match-wins, and a leading!re-includes a path an earlier, broader pattern excluded (e.g. drop everything under a directory except one file). Not scoped to/opt/rapyuta/configsand independent of--configs-path-- it applies to any absolute host path a deployment declares as a volume source, whether or not--configs-pathis also given./opt/rapyuta/configsare left unchanged by both flags, so app behavior inside the container is unaffected -- only the host source of the bind mount changes.upneeded the same changes since it calls the sharedgenerate_compose_file()to regenerate the compose doc on every invocation.init-fixpermsdependency-wiring check to parse volume strings from the right (_get_volume_target) instead ofvol.split(":")[1], so host paths containing extra:(e.g. Windows drive letters) no longer shift the container-path field position -- found during review of the--configs-pathoverride.Test plan
uv run pytest tests/unit/-- 246 passeduv run ruff check ./uv run ruff format --check .-- clean--configs-path/--ignore-volume-sourcefor a dummy Package/Deployment manifest, confirmed viadocker inspectthat the default mount, custom volume subPaths, ignore/re-include patterns (with and without--configs-path), and full-path matching all behave as documentedrio compose up --configs-path ./local-configsfollowed bydocker exec ... cat /opt/rapyuta/configs/server/hello.txtreturned content from the local override file, confirming the mount worked end-to-end--configs-path, ignore-drop, glob-drop, negate-reinclude, and standalone use of--ignore-volume-sourcewithout--configs-path), run against the AppImage build of this branch (commitf5b7199, rebased onto currentdevel) -- all pass, no regressions intest_compose.py/test_compose_negative.py(1 pre-existing, unrelated Click-message-format failure reproduces identically against plain devel)devel🤖 Generated with Claude Code