feat(ralphex-dk): add --image and --port CLI flags#301
Conversation
Mirror the existing RALPHEX_IMAGE and RALPHEX_PORT env vars with CLI flags, consistent with how --docker, --network, and --claude-provider expose their env-backed settings. CLI flags take precedence over env vars when both are set. Related to #298
There was a problem hiding this comment.
Pull request overview
This PR adds --image and --port CLI flags to the ralphex-dk Docker wrapper so users can set the Docker image and dashboard port without relying on RALPHEX_IMAGE / RALPHEX_PORT, with CLI taking precedence over environment variables.
Changes:
- Add
--image/--portwrapper flags and resolve their values with CLI > env > default precedence. - Add unit tests covering parsing and precedence behavior for image/port resolution in
main(). - Update README and
llms.txtto document the new flag equivalents for the existing env vars.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/ralphex-dk/ralphex_dk_test.py | Adds tests for --image/--port parsing and for CLI/env/default resolution in main(). |
| scripts/ralphex-dk.sh | Introduces the new argparse flags and uses them when resolving the wrapper’s image/port settings. |
| llms.txt | Documents --image and --port as CLI counterparts to RALPHEX_IMAGE / RALPHEX_PORT. |
| README.md | Updates Docker wrapper env var docs to reference the new CLI flags. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| parser.add_argument("--port", dest="port", metavar="PORT", | ||
| help=f"web dashboard port with --serve (default: {DEFAULT_PORT}) (env: RALPHEX_PORT)") |
There was a problem hiding this comment.
--port here only changes the host-side docker -p mapping, but ralphex inside the container still runs with its own --port default (8080). This can make ralphex print a URL with the wrong port when --port/RALPHEX_PORT is set. Consider passing the chosen port through to ralphex (and mapping host:port to container:port) or, at minimum, clarifying in the flag help that it’s a host bind port remap to container 8080.
| image = parsed.image or os.environ.get("RALPHEX_IMAGE", DEFAULT_IMAGE) | ||
| port = parsed.port or os.environ.get("RALPHEX_PORT", DEFAULT_PORT) | ||
| network = parsed.network or os.environ.get("RALPHEX_DOCKER_NETWORK", "") |
There was a problem hiding this comment.
Using parsed.image or ... / parsed.port or ... makes precedence depend on truthiness, not whether the flag was provided. If a user passes an empty string (e.g. --image ''), the env var would incorrectly win even though the CLI flag was set. Prefer explicit is not None checks for these optional argparse values to guarantee "CLI overrides env" semantics.
| image = parsed.image or os.environ.get("RALPHEX_IMAGE", DEFAULT_IMAGE) | |
| port = parsed.port or os.environ.get("RALPHEX_PORT", DEFAULT_PORT) | |
| network = parsed.network or os.environ.get("RALPHEX_DOCKER_NETWORK", "") | |
| image = parsed.image if parsed.image is not None else os.environ.get("RALPHEX_IMAGE", DEFAULT_IMAGE) | |
| port = parsed.port if parsed.port is not None else os.environ.get("RALPHEX_PORT", DEFAULT_PORT) | |
| network = parsed.network if parsed.network is not None else os.environ.get("RALPHEX_DOCKER_NETWORK", "") |
Mirror the existing
RALPHEX_IMAGEandRALPHEX_PORTenv vars with CLI flags, consistent with how--docker,--network, and--claude-providerexpose their env-backed settings. CLI flags take precedence over env vars when both are set.Related to #298