Skip to content

feat: env-driven server config; image honors injected PORT#30

Open
JasonWildMe wants to merge 1 commit into
mainfrom
feat/env-driven-container-config
Open

feat: env-driven server config; image honors injected PORT#30
JasonWildMe wants to merge 1 commit into
mainfrom
feat/env-driven-container-config

Conversation

@JasonWildMe

Copy link
Copy Markdown

What

Makes the container's runtime config env-driven so the same image runs unmodified across providers:

  • app/main.py: argparse defaults come from DEVICE / HOST / PORT / WORKERS; explicit CLI flags still win. Integer envs are validated (bare ASCII digits, port range 1–65535) and fall back with a warning instead of crashing on provider-injected junk like k8s service-link PORT=tcp://....
  • docker/dockerfile: CMD no longer hardcodes --host/--port (which silently defeated any platform-injected PORT, e.g. Cloud Run's); instead ENV PORT=6050 HOST=0.0.0.0 preserves image behavior exactly. The HEALTHCHECK applies the same validation/fallback as _int_env, so server and probe can never disagree about the bound port.
  • Bare-metal default port stays 8888 — no silent break for python3 -m app.main.
  • README command-line table updated with the env-var column and precedence rules.

Both compose files pass explicit flags and are unaffected.

Why

This is the load-bearing 'portability contract' piece of #26, carved out atomically: without the CMD change, the env-driven PORT never engages in the shipped image; with it, Cloud Run / RunPod / VM all configure the container purely via environment.

Testing

TDD: tests/test_env_config.py — 10 subprocess probes (app.main parses argv at import) covering env defaults, CLI-over-env precedence, malformed/whitespace/out-of-range fallbacks, and the unchanged bare default. Full suite passes (42 existing + 10 new). Healthcheck shell logic verified against 6050 / empty / tcp:// junk / whitespace / 0 / 65536 inputs — matches _int_env on all.

Provenance

Carved out of #26. Adversarially reviewed by Codex 5.6 (gpt-5.6-terra), 3 rounds: it drove the healthcheck/server port-validation mirroring (its round-1 Major), the whitespace-exactness fix, and the port-range validation; each fix is pinned by tests. Note: the image hasn't been rebuilt in CI here — the dockerfile change wants one smoke build before merge.

🤖 Generated with Claude Code

argparse defaults now come from the environment (DEVICE, HOST, PORT,
WORKERS); explicit CLI flags still override, so both compose files are
unaffected. Integer envs tolerate provider-injected junk (e.g. k8s
service-link PORT=tcp://...) instead of crashing at import.

The image CMD no longer hardcodes --host/--port: the dockerfile sets
ENV PORT=6050 HOST=0.0.0.0, so image behavior is unchanged, but a
platform-injected PORT (Cloud Run) now actually reaches the server
instead of being silently overridden by CMD flags. Bare-metal default
port stays 8888.

Written test-first: tests/test_env_config.py (subprocess probes; 4
failing tests drove the change, 4 pin compat invariants).

Carved out of #26; supersedes its app/main.py hunk with the port-
default compatibility preserved.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@JasonWildMe

Copy link
Copy Markdown
Author

Follow-up on the provenance note: the deferred Codex 5.6 round-4 confirmation has now run — converged, no remaining findings (PORT=0/65536 fall back consistently in both app startup and the image healthcheck; valid ports accepted). Smoke docker build of the changed dockerfile is running and will be reported here.

@JasonWildMe

Copy link
Copy Markdown
Author

Smoke build + runtime verification done:

  • docker build -f docker/dockerfile succeeds with the new ENV/HEALTHCHECK/CMD instructions.
  • docker run ml-detector-smoke python3 -c ... (no env overrides): argparse resolves port=6050 host=0.0.0.0 from the image ENV — container behavior identical to the old hardcoded CMD flags.
  • docker run -e PORT=7777 ...: argparse resolves port=7777 — a platform-injected PORT (the Cloud Run scenario) now actually reaches the server, which was the point of this PR.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant