Retroactive review followup: Fix API details and style nits in warm pools guide#2430
Merged
Conversation
- Use port 80 with the http handler for the Flycast service; Flycast is HTTP-only and .flycast hostnames can't get public certs - Drop min_machines_running from the example; it's inert with autostop off - Don't claim a specific 422 status for app-name collisions or a 60s max on the wait endpoint (docs and OpenAPI spec say 60s is the default) - Repoint the lease link to an anchor that exists and drop the anchor on the exec link (exec isn't in the API reference yet) - Use the canonical /docs/launch/processes/ URL instead of a redirect - Match blueprint conventions: sentence-case heading, colon separators in Related reading
Contributor
Author
|
Hey @kylemclaren, this is the follow-up from the retroactive review of your warm pools blueprint (#2427). The API usage is good overall (I checked it against the OpenAPI spec at docs.machines.dev). A few things that would be good if you can answer from your testing:
Related: |
kcmartin
changed the title
The blueprint was added to the index page but not to partials/_guides_nav.html.erb, so it never appeared in the side nav. Placed under Architecture Patterns, matching the index page order.
kylemclaren
approved these changes
Jul 15, 2026
kylemclaren
left a comment
kylemclaren
left a comment
Contributor
There was a problem hiding this comment.
Amazing, thanks @kcmartin
The allocation call passed "network": "", which allocates the Flycast address into the organization's default network, not the app's isolated per-app network. Verified against the live Machines API: an empty (or omitted) network lands the IP in the org default 6PN segment, while passing the app's network name places it in the app's own segment alongside its Machines. As written, the Flycast address could not reach a Machine in a custom-network app. Allocate with the same network name used to create the app, and add prose explaining the field does not default to the app's network. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kylemclaren
reviewed
Jul 15, 2026
kylemclaren
left a comment
kylemclaren
left a comment
Contributor
There was a problem hiding this comment.
Two follow-ups from verifying details against the live Machines API. Both are inline suggestions below.
Contributor
|
@kcmartin comments 1 - 3 all addressed, thanks! On exec and ip_assignments, those are documented: |
Co-authored-by: Kyle McLaren <mclaren@fly.io>
Co-authored-by: Kyle McLaren <mclaren@fly.io>
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
2 participants
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.
Follow-up review fixes for the warm pools blueprint (#2427).
Technical:
httphandler for the Flycast service. Flycast is HTTP-only, and.flycasthostnames can't get public certs, so atlshandler on 443 would force clients to skip cert verification. This matches the config in the Connecting to User Machines blueprint.networkfield on the allocation doesn't default to the app's network; left empty, the address lands in the org's default network and can't reach a Machine on the app's isolated per-app network.min_machines_runningfrom the Machine config example; it only has an effect when autostop is enabled, and this config setsautostop: "off"deliberately.422status for app-name collisions (verified against the live API by @kylemclaren), and add422to the Response Codes table in the Machines API reference so the guide cites a documented code.Links:
#create-a-machine-lease(the#machine-leasesanchor doesn't exist), and point the exec link at the generated API reference on docs.machines.dev, since exec isn't in the hand-written reference yet./docs/launch/processes/instead of the/docs/apps/processes/redirect.Style, matching other blueprints: sentence-case "Pointers and footguns" heading, colon separators in Related reading.
Added missing entry for this guide on the side navigation.
Not included here: bringing the hand-written Machines API reference pages up to date with
execandip_assignments(both are documented in the generated reference at docs.machines.dev). That's left for a separate PR.