feat: make axum host configurable via manifest and env vars#231
Open
ChristianPavilonis wants to merge 3 commits intomainfrom
Open
feat: make axum host configurable via manifest and env vars#231ChristianPavilonis wants to merge 3 commits intomainfrom
ChristianPavilonis wants to merge 3 commits intomainfrom
Conversation
aram356
requested changes
Mar 30, 2026
Contributor
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Well-structured feature that adds configurable host/port binding for the Axum adapter with a clean precedence model (env vars → manifest → defaults). CI gates all pass. One behavioral inconsistency around port 0 validation needs fixing.
😃 Praise
- Clean configuration precedence design with good separation of concerns between CLI, adapter, and manifest layers
- Excellent test isolation: using
resolve_addr_from_partswith explicit parameters avoids env var races in parallel test runners - Thorough test coverage for the happy paths in all three layers (manifest, CLI, adapter)
Findings
Blocking
- 🔧 Port 0 inconsistency:
cli.rsrejects port 0 fromaxum.tomlbutresolve_addr_from_partsandresolve_dev_addracceptEDGEZERO_PORT=0and manifest port 0, causing random OS port binding (dev_server.rs:188)
Non-blocking
- 🤔
resolve_addrpublic API surface: exported publicly but only used internally byrun_app— considerpub(crate)or documenting the external use case (lib.rs:22) - 🤔
hostfield as String, not IpAddr: accepts any string at parse time, IP validation deferred to adapter runtime (manifest.rs:285) - 🤔 Duplicated resolution logic:
resolve_dev_addrin edgezero-cli duplicatesresolve_addr_from_partslogic with different logging (cli/dev_server.rs:88) - 🤔 Inconsistent error strictness: invalid host in
axum.toml→ hard error; invalid host inedgezero.toml→ warning + fallback. Reasonable per context but worth documenting. - ⛏ Incidental
#[derive(Debug)]onAxumProject— harmless (cli.rs:143)
📌 Out of Scope
- Missing tests for invalid env var fallback paths (e.g.
EDGEZERO_HOST="not-an-ip",EDGEZERO_PORT="abc") — good to add but not blocking - Two config sources (
axum.tomlvsedgezero.toml) for the same adapter is pre-existing complexity worth documenting
CI Status
- fmt: ✅ PASS
- clippy: ✅ PASS
- tests: ✅ PASS (322 passed)
Add host/port configuration to the axum adapter so the bind address can be set to 0.0.0.0 or any other IP instead of the hardcoded 127.0.0.1:8787 default. Configuration precedence (highest wins): 1. EDGEZERO_HOST / EDGEZERO_PORT environment variables 2. Manifest value (edgezero.toml or axum.toml) 3. Default: 127.0.0.1:8787 Invalid values produce warnings instead of silently falling back to defaults. The CLI subprocess now receives EDGEZERO_HOST/EDGEZERO_PORT so resolved values propagate to the running server.
…dr visibility - Extract resolve_bind_addr into edgezero_core::addr so both the axum adapter and the CLI dev server share a single code path for resolving bind addresses from env vars and config. - Reject port 0 (random OS port) with a warning and fallback to 8787, matching the existing cli.rs validation. - Narrow resolve_addr to pub(crate) since it has no external consumers.
5f95ce3 to
9561502
Compare
aram356
requested changes
Apr 1, 2026
Contributor
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Well-structured feature that adds configurable host/port binding with clean precedence logic (env > manifest > defaults). The centralised addr.rs module and backward-compatible manifest changes are solid. Main concern is a validation inconsistency between the axum.toml path and the shared resolve_bind_addr path.
😃 Praise
- Extracting
resolve_bind_addras a pure function with comprehensive tests (including IPv6, port 0, partial overrides) is excellent — avoids env var race conditions in parallel tests - Backward-compatible manifest extension using
Option+#[serde(default)]is the right pattern - Port 0 rejection in
addr.rsprevents confusing random-port behavior
Findings
Blocking
- ❓ Two separate validation paths:
read_axum_project(axum.toml) hard-errors on invalid IP, whileresolve_bind_addrwarns and falls back. Same user mistake → different behavior depending on config source. See inline comment oncli.rs.
Non-blocking
- 🤔
pub mod addrAPI surface:resolve_bind_addris now part ofedgezero_core's public API. Confirm this is intentional vspub(crate). See inline onlib.rs. - 🤔 Silent warning drop in CLI fallback:
log::warn!inresolve_bind_addrmay fire before the logger is initialised in the CLI fallback path, silently swallowing invalid-value warnings. See inline onaddr.rs. - 🏕 Magic
8787in cli.rs: The default port8787is now defined asDEFAULT_PORTinedgezero_core::addrbut remains hardcoded inread_axum_project(None => 8787). Consider using the constant or a localconstto keep values in sync. - 🌱 Late host validation: Manifest accepts arbitrary strings for
host, validated only at bind time. Defensible for WASM compat but worth a doc comment. See inline onmanifest.rs.
CI Status
- fmt: ✅ PASS
- clippy: ✅ PASS
- tests: ✅ PASS (322 tests)
- resolve_bind_addr now returns Result<SocketAddr, String> instead of silently falling back on invalid values (addresses silent warning drop) - read_axum_project delegates host/port validation to resolve_bind_addr, eliminating the inconsistency between axum.toml and edgezero.toml paths - DEFAULT_HOST and DEFAULT_PORT are now pub const for cross-crate reuse - CLI fallback path surfaces errors via eprintln instead of dropped log::warn - Added doc comment on manifest host field explaining late IP validation
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
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.
Summary
hostandportfields toManifestAdapterDefinitionso adapters can declare bind addresses inedgezero.tomlunder[adapters.<name>.adapter]adapter.hostfromaxum.tomlwith IP validation, defaulting to127.0.0.1run_app()now reads host/port from manifest config withEDGEZERO_HOST/EDGEZERO_PORTenv var overridesrun_cargosubprocess both respect the same env varsConfiguration Precedence
EDGEZERO_HOST/EDGEZERO_PORTenvironment variables (highest)edgezero.tomloraxum.toml)127.0.0.1:8787Usage
# Or via environment EDGEZERO_HOST=0.0.0.0 EDGEZERO_PORT=3000 cargo run -p my-adapterFiles Changed
crates/edgezero-core/src/manifest.rshost+porttoManifestAdapterDefinition+ testscrates/edgezero-adapter-axum/src/cli.rshostfromaxum.toml, propagate via env vars to subprocesscrates/edgezero-adapter-axum/src/dev_server.rsresolve_addr()with manifest + env var precedence + warningscrates/edgezero-adapter-axum/src/lib.rsresolve_addrcrates/edgezero-cli/src/dev_server.rs