Default web ports from image EXPOSE - #1099
Conversation
Miren ignored EXPOSE when resolving web service ports, so images that listen somewhere other than 3000 waited for the alternative-port fallback unless users repeated the port in app.toml. Carry deterministic OCI exposed-port keys through BuildResult and fold a single TCP declaration into the resolved service config. Explicit ports still win, while multiple ports, UDP-only declarations, and custom service images stay conservative when the right choice is ambiguous or unavailable.
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
This is a draft PR, so I'm judging whether it's ready to graduate to human review. It is — the implementation is solid and I'd be comfortable with it being merged after a final human sign-off.
What the change does: it threads image EXPOSE metadata through the build pipeline — from BuildResult.ExposedPorts (populated in both the stackbuild and Dockerfile paths) — and uses it to auto-populate svc.Port on the synthesized web service when exactly one TCP port is declared and nothing in app.toml already sets a port.
What I like about the implementation: The guard in defaultServicePortFromImage is conservative and correct: it skips the service if it isn't named "web", if it carries a custom Image (right call — EXPOSE metadata belongs to the app image, not a sidecar), if a scalar Port is already set, or if a Ports array is present. All four conditions are exercised by the test cases. singleExposedTCPPort is correctly strict: it rejects multiple ports (ambiguous), UDP, and out-of-range values, and it treats a bare port number (no / suffix) as TCP, matching Docker's own semantics.
The refactor in buildkit.go is clean: extracting applyImageConfig and exposedPortNames from the inline anonymous struct lets both the stackbuild path and the Dockerfile path share the same parsing, and the test TestApplyImageConfig now exercises the parsing directly. The sort in exposedPortNames makes port order deterministic, which matters because the test asserts a specific slice order (["4000/tcp", "8080/tcp"]).
One small thing worth noting: the TestBuildVersionConfigDefaultsWebPortFromImage table test uses buildVersionConfig (the full pipeline) rather than testing defaultServicePortFromImage in isolation. That's fine here — it means the test validates the real call site — but it also means each case goes through appconfig.ResolveDefaults and the full service-building path. Nothing is broken by this; I just want to flag it in case a future maintainer wonders why an isolated unit test isn't available. The coverage of the interesting cases (UDP, multiple ports, existing port wins, custom image, ports array) is thorough regardless.
No correctness issues I can find. The res != nil guard around defaultServicePortFromImage is consistent with the existing guard for WorkingDir. The draft is ready for human review.
🍪 full review note · comment /biscuit review to run biscuit again.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour. 📝 WalkthroughWalkthroughBuildKit now extracts and sorts image-exposed ports into Merge Risk: ⚪ Minimal · up to The change derives an unspecified web port from a single TCP port advertised by the image while preserving explicit configuration; no actionable merge-blocking risk remains beyond normal checks and review. Comment |
evanphx
left a comment
evanphx
left a comment
There was a problem hiding this comment.
Oh, very nice and friendly!
2 participants
Images already advertise their listening port through
EXPOSE, but Miren ignored it and always resolved an unspecified web port to 3000. Common upstream images therefore needed duplicate port config or waited for the alternative-port fallback.Builds now carry OCI exposed-port metadata into the resolved service config. A single TCP declaration fills an otherwise unspecified web port, while explicit port config still wins and ambiguous declarations stay conservative. Persisting the result as the ordinary scalar port keeps routing, health checks, and
$PORTon the same value.A Docker-backed smoke deploy with
EXPOSE 4321came up directly on 4321 and served through the default HTTPS route.Closes MIR-1445