[ACIX-1225] Create `-v` option on dev env start command #230

Ishirui · 2025-12-17T17:19:37Z

Adds a new option to dev envs: -v, which allows for mounting arbitrary directories or Docker volumes from the host into the dev env on container creation.

Copilot

Pull request overview

This PR adds a new -v/--volume option to the dev environment start command, allowing users to mount additional directories or Docker volumes into development containers. The feature supports bind mounts from host paths (absolute, relative, or tilde-prefixed) and named Docker volumes, with flags like read-only mode and volume options.

Key Changes:

Added extra_mount_specs configuration field to store user-provided volume specifications
Implemented extra_mounts property to parse volume specs into Mount objects
Added validation function __validate_extra_mount_specs to verify mount specifications
Integrated extra mounts into the container creation process in the start() method

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.

File	Description
src/dda/env/dev/types/linux_container.py	Implements the core volume mounting functionality including config field, parsing logic, validation, and integration with container start command
tests/env/dev/types/test_linux_container.py	Adds comprehensive tests for valid mount scenarios (absolute, relative, multiple, with flags) and invalid mount scenarios (non-existent paths, relative destinations)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/dda/env/dev/types/linux_container.py

tests/env/dev/types/test_linux_container.py

src/dda/env/dev/types/linux_container.py

…r dev envs

…logic

This should be interpolated by the shell before it ever gets to Python anyway.

For more complex usecases like using named volumes or weird flags, users should use `--mount`, which will be implemented next.

They would be annoying to fix, and there should not be any Windows-specific behavior here. The positive test cases are still run on Windows.

ofek

Thanks!

src/dda/env/dev/types/linux_container.py

ofek · 2025-12-22T14:52:09Z

src/dda/env/dev/types/linux_container.py

+            for volume in self.extra_mounts:
+                command.extend(("--mount", volume.as_csv()))


Unlike most of the other mounts such as for caching, we don't use these in any context other than constructing arguments. What do you think about removing all of the validation logic and having the options extend the arguments directly? This would also support Windows immediately.

I had sort of the same thought when I noticed that my validation code was becoming way more complex than the actual implementation 😅
My original thought process was that errors coming directly from docker run would be a lot less nice and slightly harder to debug than the ones I've implemented here.

I think your point about Windows paths (great catch btw, I didn't realize splitting on : would break Windows paths... but of course it does 🤦) shifts the balance towards removing it :)

In the end I only removed the processing for -v, as converting this old-style of volume mounts to Mount objects was a bit kludgy.

I left it for the -m option though, as I think it still makes sense to use the custom class and validation there.
LMK if you want me to remove that for -m as well, although I don't see the value in doing that.

src/dda/env/dev/types/linux_container.py

We just pass the arguments straight to `docker run`, with no extra validation, instead. This means we also have to duplicate the test to test separately for `-v` and `-m` options.

…useless validation logic

src/dda/env/dev/types/linux_container.py

Co-authored-by: Ofek Lev <[email protected]>

ofek

Thanks!

* feat(env): Add ability to mount arbitrary volumes into linux container dev envs * feat(test): Add test for creating dev env with extra bind mounts * refactor(env): Use `Mount` class for specifying extra mounts * feat(env): Add validation logic for `-v` parameter * fix(tests): Fix test after using `Mount` class and adding validation logic * feat(tests): Add test for invalid `-v` params * fix(env): Fix split on colons * fix(env): Disallow passing literal `~` in paths This should be interpolated by the shell before it ever gets to Python anyway. * feat(env): Only allow bind mounts with `-v` For more complex usecases like using named volumes or weird flags, users should use `--mount`, which will be implemented next. * feat(env): Add support for `--mount` parameter * fix(tests): Skip `--mount` invalid param tests on Windows They would be annoying to fix, and there should not be any Windows-specific behavior here. The positive test cases are still run on Windows. * refactor(env): Shorten usage examples for brevity * fix(env): Remove Windows-incompatible processing logic for `-v` We just pass the arguments straight to `docker run`, with no extra validation, instead. This means we also have to duplicate the test to test separately for `-v` and `-m` options. * refactor: Pass `--mount` arguments directly to `docker run` + remove useless validation logic * Final help text cleanup Co-authored-by: Ofek Lev <[email protected]> --------- Co-authored-by: Ofek Lev <[email protected]> d6f0e56

Ishirui force-pushed the pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount branch from 06f5a68 to 80a4512 Compare December 18, 2025 17:43

Ishirui marked this pull request as ready for review December 18, 2025 17:46

Ishirui requested a review from a team as a code owner December 18, 2025 17:46

Copilot AI review requested due to automatic review settings December 18, 2025 17:46

Copilot started reviewing on behalf of Ishirui December 18, 2025 17:47 View session

Copilot AI reviewed Dec 18, 2025

View reviewed changes

Ishirui force-pushed the pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount branch from 058aba9 to ea529b7 Compare December 19, 2025 16:05

Ishirui added 11 commits December 19, 2025 17:51

feat(env): Add ability to mount arbitrary volumes into linux containe…

c7cca30

…r dev envs

feat(test): Add test for creating dev env with extra bind mounts

d00e0a1

refactor(env): Use Mount class for specifying extra mounts

9d666b2

feat(env): Add validation logic for -v parameter

fc7f493

fix(tests): Fix test after using Mount class and adding validation …

131a166

…logic

feat(tests): Add test for invalid -v params

e00dbba

fix(env): Fix split on colons

268e390

fix(env): Disallow passing literal ~ in paths

736fe75

This should be interpolated by the shell before it ever gets to Python anyway.

feat(env): Only allow bind mounts with -v

e7a8153

For more complex usecases like using named volumes or weird flags, users should use `--mount`, which will be implemented next.

feat(env): Add support for --mount parameter

3e9a718

fix(tests): Skip --mount invalid param tests on Windows

07e151f

They would be annoying to fix, and there should not be any Windows-specific behavior here. The positive test cases are still run on Windows.

Ishirui force-pushed the pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount branch from f0409c8 to 07e151f Compare December 19, 2025 16:51

ofek reviewed Dec 22, 2025

View reviewed changes

Ishirui added 2 commits December 23, 2025 11:53

refactor(env): Shorten usage examples for brevity

45bbd94

fix(env): Remove Windows-incompatible processing logic for -v

f2dba8b

We just pass the arguments straight to `docker run`, with no extra validation, instead. This means we also have to duplicate the test to test separately for `-v` and `-m` options.

Ishirui requested a review from ofek December 23, 2025 11:29

refactor: Pass --mount arguments directly to docker run + remove …

8f92123

…useless validation logic

ofek reviewed Dec 24, 2025

View reviewed changes

src/dda/env/dev/types/linux_container.py Outdated Show resolved Hide resolved

Final help text cleanup

b741023

Co-authored-by: Ofek Lev <[email protected]>

ofek approved these changes Dec 24, 2025

View reviewed changes

Ishirui merged commit d6f0e56 into main Dec 24, 2025
20 checks passed

Ishirui deleted the pierrelouis.veyrenc/ACIX-1225-arbitrary-bind-mount branch December 24, 2025 17:26

		for volume in self.extra_mounts:
		command.extend(("--mount", volume.as_csv()))

[ACIX-1225] Create -v option on dev env start command #230

[ACIX-1225] Create -v option on dev env start command #230

Uh oh!

Conversation

Ishirui commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ofek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ofek Dec 22, 2025

Choose a reason for hiding this comment

Uh oh!

Ishirui Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Ishirui Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ofek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[ACIX-1225] Create `-v` option on dev env start command #230

[ACIX-1225] Create `-v` option on dev env start command #230

Ishirui commented Dec 17, 2025 •

edited

Loading