fix: make the local-docker deploy path usable (one --target + free-port selection)#4
Open
JoshuaBearup wants to merge 1 commit into
Open
fix: make the local-docker deploy path usable (one --target + free-port selection)#4JoshuaBearup wants to merge 1 commit into
JoshuaBearup wants to merge 1 commit into
Conversation
Two fixes to the local-docker target, which deploy.mjs supports but the CLI couldn't reach reliably: 1. `polyrange one` hardcoded `--target=fly`, so the single-class command could never deploy to local-docker even though `generator/deploy.mjs` accepts `--target=local-docker`. Thread `--target` (default fly) and `--region` through to the deploy subprocess. 2. local-docker picked its host port with `8000 + random(1000)` and no collision check, so concurrent deploys could choose the same port and the second `docker run -p` would fail. Ask the OS for a free ephemeral port on 127.0.0.1 instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
1 participant
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.
What
Two small fixes that make the existing
local-dockertarget actually usable from the CLI.1.
polyrange onecouldn't target local-dockergenerator/deploy.mjsaccepts--target=fly|local-docker, butcmdOnehardcoded--target=fly:So
node polyrange.mjs one --class=… --target=local-dockersilently still deployed to Fly. Fixed by threading--target(defaultfly) and--regionthrough to the subprocess.2. local-docker port had no collision check
The host port was chosen as
8000 + Math.floor(Math.random() * 1000)with no check that it's free. Under concurrent deploys (e.g. an eval fan-out) two cells can draw the same port and the seconddocker run -pfails with a port-in-use error. Replaced with an OS-assigned free ephemeral port on127.0.0.1vianet.createServer().listen(0).Testing
node -cclean on both files. VerifiedfindFreePort()returns distinct usable ports on repeated calls, and thatcmdOnenow forwardsflags.targetinstead of a literal. I did not run a full container deploy end-to-end here (no API key for the generation step in my environment), but both changes are in the deploy plumbing, independent of generation.Note
I left the related "precheck requires flyctl even for a local-docker run" out of this PR —
onedoesn't run precheck, andeval/deployare Fly-only today, so making precheck target-aware really belongs with a separate "local-docker eval" change rather than a bug fix. Happy to follow up on that if local-docker eval is on the roadmap.