Check destination before eager loading sub-configs#1912
Open
ThomasSevestre wants to merge 4 commits into
Open
Conversation
When require_destination: true and no destination is given, a sub-config validation error (e.g. "Builder arch not set") could mask the real cause, because the eager-loaded sub-configs validated themselves before the destination guard ran. Move ensure_destination_if_required to run right after the top-level validate!, so the missing-destination error surfaces first regardless of which destination-only sub-config happens to be incomplete.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a confusing error path when require_destination: true is set but no destination is provided, by ensuring the missing-destination check runs before eager-loaded sub-configs are constructed/validated.
Changes:
- Move
ensure_destination_if_requiredto run immediately after top-level config validation inKamal::Configuration#initialize. - Add a regression test ensuring the destination guard raises
ArgumentError, "You must specify a destination"before sub-config validation can mask it.
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 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/kamal/configuration.rb | Runs the destination-required guard earlier to prevent sub-config validation from masking the real error. |
| test/configuration_test.rb | Adds regression coverage asserting destination validation precedes sub-config validation failures. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.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
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.
Check destination before eager loading sub-configs
Problem
When a project sets
require_destination: trueand the user runs a command without-d <destination>, Kamal raises a confusing, unrelated error instead of telling the user the destination is missing.This happens whenever a required sub-config key lives only in the per-destination file. Because Kamal eager-loads and validates every sub-config during
Configuration#initializebefore checking the destination, the first incomplete sub-config raises and masks the real cause. Depending on your layout the masking error varies (builder arch, servers, registry, …).Example — misleading config
deploy.yml(base, nobuilder.arch— it lives in the destination files):deploy.production.yml:Misleading message — running any command without
-d:The user has no idea this actually means "you forgot
-d production".After this change:
Root cause
In
Kamal::Configuration#initialize, the eager-loaded sub-configs (Servers,Registry,Builder, …) validate themselves during construction, and one of them raises beforeensure_destination_if_requiredruns at the end of the method.ensure_destination_if_requiredonly depends on@destinationandraw_config.require_destination, both available right after the top-levelvalidate!— so the guard can safely run much earlier.Fix
Move
ensure_destination_if_requiredto run immediately after the top-levelvalidate!, before the sub-configs are eager-loaded. The missing-destination error now surfaces first, regardless of which destination-only sub-config happens to be incomplete. The generic top-levelvalidate!stays where it is.Tests
test/configuration_test.rb): base config missing a destination-only key (builder.arch) +require_destination: true+ no destination → raisesArgumentError, "You must specify a destination". Confirmed it fails onmainwith the oldKamal::ConfigurationError: builder: Builder arch not setand passes with the fix.855 runs, 3116 assertions, 0 failures, 0 errors, 0 skips.