Fix #1934: Separate proxied and non-proxied accessory boot ordering - #1942
Open
TotyLabs wants to merge 2 commits into
Open
Fix #1934: Separate proxied and non-proxied accessory boot ordering#1942TotyLabs wants to merge 2 commits into
TotyLabs wants to merge 2 commits into
Conversation
…dering On a fresh server with error_pages_path and proxied accessories, kamal setup would fail because proxied accessories were booted before app:boot, which meant kamal-proxy tried to validate the error_pages directory before it was created. The solution splits accessory boot into two phases: - Boot non-proxied accessories before app:boot (original behavior) - Boot proxied accessories after app:boot (new, ensures error pages exist) This preserves all existing semantics while fixing the fresh server scenario. Fixes: basecamp#1934
Contributor
There was a problem hiding this comment.
Pull request overview
Separates accessory startup phases so proxied accessories register only after app error pages are available.
Changes:
- Boots non-proxied accessories before
app:boot. - Boots proxied accessories afterward.
- Adds a mixed-accessory setup test and fixture.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/kamal/cli/main.rb |
Splits accessory startup around app boot. |
test/cli/main_test.rb |
Tests mixed accessory setup behavior. |
test/fixtures/deploy_with_error_pages_and_accessories.yml |
Provides proxied and non-proxied accessory configuration. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Author
|
Hi! Just following up on this PR in case it got buried. The requested review changes have been addressed and it's ready for another look when you have a chance. Thanks! |
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.
On a fresh server with error_pages_path and proxied accessories, kamal setup could fail because proxied accessories booted before app:boot, causing kamal-proxy to validate the error pages directory before it existed.
The solution splits accessory boot into two phases:
Boot non-proxied accessories before app:boot
Boot proxied accessories after app:boot, ensuring error pages exist
This preserves the existing boot order for non-proxied accessories while ensuring error pages exist before proxied accessories are registered.
Fixes #1934