Add --engine-args flag for custom container engine arguments#2444
Open
rhatdan wants to merge 1 commit intocontainers:mainfrom
Open
Add --engine-args flag for custom container engine arguments#2444rhatdan wants to merge 1 commit intocontainers:mainfrom
rhatdan wants to merge 1 commit intocontainers:mainfrom
Conversation
rhatdan
requested review from
bmahabirbu,
cgruver,
engelmi,
jhjaggars,
maxamillion,
mikebonnet,
olliewalsh and
swarajpande5
as code owners
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
Contributor
Reviewer's GuideAdds a new --engine-args option that can be provided via CLI or ramalama.conf to append arbitrary podman/docker flags to the container engine invocation, with validation to disallow use in nocontainer mode and documentation/tests covering the new behavior. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
Summary of ChangesHello @rhatdan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement by providing advanced users with direct control over container engine behavior. It implements a new Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Consider moving
add_engine_args()later in theEngine.__init__setup (e.g., afteradd_port_option()andadd_tty_option()) so that user-specified engine arguments can reliably override automatically added flags instead of being overridden by them. - Right now
add_engine_args()appends each provided string as a single token; if you expect users to pass combined flags (e.g.,"--mount type=bind,..."), you may want toshlex.spliteach entry so they can include multiple engine flags or flag/value pairs per--engine-argsoccurrence.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving `add_engine_args()` later in the `Engine.__init__` setup (e.g., after `add_port_option()` and `add_tty_option()`) so that user-specified engine arguments can reliably override automatically added flags instead of being overridden by them.
- Right now `add_engine_args()` appends each provided string as a single token; if you expect users to pass combined flags (e.g., `"--mount type=bind,..."`), you may want to `shlex.split` each entry so they can include multiple engine flags or flag/value pairs per `--engine-args` occurrence.
## Individual Comments
### Comment 1
<location> `ramalama/cli.py:1050-1056` </location>
<code_context>
help="""pass `--group-add keep-groups` to podman.
If GPU device on host is accessible to via group access, this option leaks the user groups into the container.""",
+ )
+ parser.add_argument(
+ "--engine-args",
+ dest="engine_args",
+ action='append',
+ type=str,
+ default=config.engine_args,
+ help="""additional arguments to pass to the container engine
+(e.g., '--mount type=bind,src=/path,dst=/path' or '--env VAR=value').
+Only valid when using containers (conflicts with --nocontainer)""",
</code_context>
<issue_to_address>
**issue (bug_risk):** Using `action='append'` with `default=config.engine_args` risks mutating the config list across parses.
`argparse` will append directly into `config.engine_args`, so each parse mutates the same list and later parses see accumulated values (e.g., args “sticking” in a long-lived process). Instead, avoid sharing the config list with argparse: use `default=None` or `default=[]` and then merge with `config.engine_args` explicitly when wiring config and CLI together.
</issue_to_address>
### Comment 2
<location> `ramalama/engine.py:119-124` </location>
<code_context>
if getattr(self.args, "podman_keep_groups", None):
self.exec_args += ["--group-add", "keep-groups"]
+ def add_engine_args(self):
+ """Add custom engine arguments specified by the user."""
+ engine_args = getattr(self.args, "engine_args", None)
+ if engine_args:
+ for arg in engine_args:
+ self.exec_args.append(arg)
+
def add(self, newargs: Sequence[str]):
</code_context>
<issue_to_address>
**issue (bug_risk):** Appending raw strings with spaces to `exec_args` can break engine CLI parsing; consider splitting each arg.
Each `engine_args` entry is added as a single argv element, even when it includes spaces (e.g. `"--mount type=bind,..."`), so the engine receives one argument instead of multiple tokens. To align with normal CLI behavior, consider `shlex.split(arg)` and `self.exec_args.extend(...)` so composite flags are split into separate argv items.
</issue_to_address>
### Comment 3
<location> `test/e2e/test_run.py:223-232` </location>
<code_context>
pytest.param(
["--device", "none", "--pull", "never"], r".*--device.*", None, None, False, None,
id="check --device with unsupported value", marks=skip_if_no_container),
+ pytest.param(
+ ["--engine-args", "--read-only"], r".*--read-only.*", None, None, True, None,
+ id="check --engine-args with single arg", marks=skip_if_no_container),
+ pytest.param(
+ ["--engine-args", "--mount type=bind,src=/data,dst=/data",
+ "--engine-args", "--env CUSTOM_VAR=value"],
+ r".*--mount type=bind,src=/data,dst=/data.*--env CUSTOM_VAR=value.*", None, None, True, None,
+ id="check --engine-args with multiple args", marks=skip_if_no_container),
# fmt: on
],
</code_context>
<issue_to_address>
**suggestion (testing):** Add an E2E test for `--engine-args` used together with `--nocontainer` to cover the user-facing error path
The new E2E tests confirm that engine args reach the dry-run command, but they don’t cover the CLI error behavior when `--engine-args` is used with `--nocontainer` (currently only covered by a unit test in `test_cli.py`). Please add a `pytest.param` combining `--engine-args` and `--nocontainer` that asserts the run fails with the expected error message, so this user-visible behavior is exercised end to end.
Suggested implementation:
```python
pytest.param(
["--engine-args", "--mount type=bind,src=/data,dst=/data",
"--engine-args", "--env CUSTOM_VAR=value"],
r".*--mount type=bind,src=/data,dst=/data.*--env CUSTOM_VAR=value.*", None, None, True, None,
id="check --engine-args with multiple args", marks=skip_if_no_container),
pytest.param(
["--engine-args", "--read-only", "--nocontainer"],
r".*engine-args.*nocontainer.*", None, None, False, None,
id="check --engine-args with --nocontainer errors"),
# fmt: on
],
)
```
The regex `r".*engine-args.*nocontainer.*"` should be updated to match the **actual** user-facing error message emitted by the CLI when `--engine-args` is combined with `--nocontainer`. You can copy the exact message from the existing unit test in `test_cli.py` that covers this behavior and adjust the pattern accordingly (e.g. `r".*The --engine-args option cannot be used together with --nocontainer.*"` or similar).
</issue_to_address>
### Comment 4
<location> `docs/ramalama-run.1.md:48-50` </location>
<code_context>
Possible values are "never", "always" and "auto". (default: auto)
+#### **--engine-args**=*ARG*
+Additional arguments to pass to the container engine (podman or docker).
+This option can be specified multiple times to add multiple arguments.
+
</code_context>
<issue_to_address>
**suggestion (typo):** Consider capitalizing "Podman" and "Docker" for consistency with other docs.
Elsewhere in the docs (e.g., `ramalama.conf.5.md`) these names are capitalized; aligning with that usage would keep things consistent.
```suggestion
#### **--engine-args**=*ARG*
Additional arguments to pass to the container engine (Podman or Docker).
This option can be specified multiple times to add multiple arguments.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
223
to
+232
| pytest.param( | ||
| ["--device", "none", "--pull", "never"], r".*--device.*", None, None, False, None, | ||
| id="check --device with unsupported value", marks=skip_if_no_container), | ||
| pytest.param( | ||
| ["--engine-args", "--read-only"], r".*--read-only.*", None, None, True, None, | ||
| id="check --engine-args with single arg", marks=skip_if_no_container), | ||
| pytest.param( | ||
| ["--engine-args", "--mount type=bind,src=/data,dst=/data", | ||
| "--engine-args", "--env CUSTOM_VAR=value"], | ||
| r".*--mount type=bind,src=/data,dst=/data.*--env CUSTOM_VAR=value.*", None, None, True, None, |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add an E2E test for --engine-args used together with --nocontainer to cover the user-facing error path
The new E2E tests confirm that engine args reach the dry-run command, but they don’t cover the CLI error behavior when --engine-args is used with --nocontainer (currently only covered by a unit test in test_cli.py). Please add a pytest.param combining --engine-args and --nocontainer that asserts the run fails with the expected error message, so this user-visible behavior is exercised end to end.
Suggested implementation:
pytest.param(
["--engine-args", "--mount type=bind,src=/data,dst=/data",
"--engine-args", "--env CUSTOM_VAR=value"],
r".*--mount type=bind,src=/data,dst=/data.*--env CUSTOM_VAR=value.*", None, None, True, None,
id="check --engine-args with multiple args", marks=skip_if_no_container),
pytest.param(
["--engine-args", "--read-only", "--nocontainer"],
r".*engine-args.*nocontainer.*", None, None, False, None,
id="check --engine-args with --nocontainer errors"),
# fmt: on
],
)The regex r".*engine-args.*nocontainer.*" should be updated to match the actual user-facing error message emitted by the CLI when --engine-args is combined with --nocontainer. You can copy the exact message from the existing unit test in test_cli.py that covers this behavior and adjust the pattern accordingly (e.g. r".*The --engine-args option cannot be used together with --nocontainer.*" or similar).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces a useful --engine-args flag to pass custom arguments to the container engine. I have identified a potential issue with how arguments containing spaces are handled when passed to the execution command, and I've suggested using shlex.split to ensure they are correctly tokenized. I've also suggested corresponding updates to the unit tests.
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
6d115e5 to
a12911d
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
a12911d to
b4feee6
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
b4feee6 to
3d61b30
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
3d61b30 to
f9fb6ed
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
added a commit
to rhatdan/ramalama
that referenced
this pull request
Feb 24, 2026
When using action='append' with default=config.engine_args, argparse would append to the config list directly, causing mutation across parses. This fix: - Uses default=None for --engine-args argument - Merges config defaults with CLI args in post_parse_setup() - Creates a copy of config.engine_args to avoid mutation - Ensures CLI args take precedence when provided Addresses review feedback from PR containers#2444 about the list mutation bug. Co-authored-by: Cursor <cursoragent@cursor.com>
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
added a commit
to rhatdan/ramalama
that referenced
this pull request
Feb 24, 2026
When using action='append' with default=config.engine_args, argparse would append to the config list directly, causing mutation across parses. This fix: - Uses default=None for --engine-args argument - Merges config defaults with CLI args in post_parse_setup() - Creates a copy of config.engine_args to avoid mutation - Ensures CLI args take precedence when provided - Moves get_config() import to function top to avoid scope issues Addresses review feedback from PR containers#2444 about the list mutation bug. Co-authored-by: Cursor <cursoragent@cursor.com>
rhatdan
force-pushed
the
add-container-args-flag
branch
from
3af80ce to
406f7ba
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
406f7ba to
2f8ed0b
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
2f8ed0b to
1811466
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
rhatdan
force-pushed
the
add-container-args-flag
branch
from
1811466 to
5af5b1a
Compare
rhatdan
temporarily deployed
to
macos-installer
GitHub Actions
Inactive
Member
Author
|
@bmahabirbu @olliewalsh @engelmi @ieaves PTAL This is ready for review. |
Collaborator
|
There are a few PRs now that are ready to merge but very likely to conflict. I think in the interest of fairness #2473 should land first as it was already in main but had to revert. It's also the most significant user-visible feature. After that merges I'll need to manually rebase and resolve all of the conflicts for #2456. Maybe it would be easier if I then refactor this change on top of that PR? Finally there is #2427. I think that should be lowest priority since it's just more type checking. I think it might be best to split that up into multiple PR to make it easier to merge over time. |
Member
Author
|
I am fine with waiting. |
Collaborator
Collaborator
|
I don't know why you were seeing what you were seeing @olliewalsh but virtually none of the issues you identified in the PR are even present in the diff. Nevertheless, I've made the cosmetic changes you requested as well. |
Implement a new --engine-args flag that allows users to pass additional arguments directly to the container engine (podman/docker). This provides flexibility for advanced users who need to customize container behavior beyond what RamaLama's built-in flags offer. The flag is named --engine-args (not --container-args) to clarify that arguments are passed to the container engine, not the container itself. Changes: - Add --engine-args CLI argument in runtime_options() - Can be specified multiple times to add multiple arguments - Implement add_engine_args() method in Engine class - Arguments are appended directly to exec_args before execution - Add validation to conflict with --nocontainer flag - Add engine_args to BaseConfig for ramalama.conf support - Document flag in ramalama-run and ramalama-serve man pages - Include usage examples, config file examples, and conflict documentation Configuration file support: Users can set default engine args in ramalama.conf: ```toml [ramalama] engine_args = ["--read-only", "--tmpfs /tmp"] ``` Use cases enabled: - Custom mounts: --engine-args '--mount type=bind,src=/data,dst=/data' - Additional env vars: --engine-args '--env CUSTOM_VAR=value' - Security options: --engine-args '--security-opt label=disable' - Read-only containers: --engine-args '--read-only' - Any other podman/docker run flags Tests: - Unit tests verify arguments are added to exec_args correctly - E2E dryrun tests verify arguments appear in engine command - Conflict tests verify --engine-args raises error with --nocontainer - All tests passing (5 new unit tests, 2 e2e tests) The arguments are passed directly to the container engine, allowing users to leverage the full power of podman/docker without RamaLama needing to explicitly support every possible container option. Fixes containers#2440 *This PR was created with the assistance of Cursor AI.* Co-authored-by: Cursor <cursoragent@cursor.com> Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
rhatdan
force-pushed
the
add-container-args-flag
branch
from
5af5b1a to
e21c0aa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implement a new --engine-args flag that allows users to pass additional arguments directly to the container engine (podman/docker). This provides flexibility for advanced users who need to customize container behavior beyond what RamaLama's built-in flags offer.
The flag is named --engine-args (not --container-args) to clarify that arguments are passed to the container engine, not the container itself.
Changes:
Configuration file support:
Users can set default engine args in ramalama.conf:
Use cases enabled:
Tests:
The arguments are passed directly to the container engine, allowing users to leverage the full power of podman/docker without RamaLama needing to explicitly support every possible container option.
Fixes #2440
This PR was created with the assistance of Cursor AI.
Summary by Sourcery
Add support for passing custom arguments to the container engine via CLI and configuration, and ensure they are applied to engine execution only when container mode is enabled.
New Features:
Enhancements:
Documentation:
Tests: