fix(cli): resolve deploy ${VAR} secrets from the project root for config/deploy.yml - #3142
Merged
Merged
Conversation
…fig/deploy.yml
ConfigLoader built its lazy SecretResolver with the YAML file's own
directory as projectRoot, so the standard config/deploy.yml layout
looked for config/.kamal/secrets — a file nothing creates — and the
.kamal/secrets leg of ${VAR} interpolation silently never fired
(masked by the process-env fallback).
$projectRootFor() now steps up to the parent of a directory named
config, so interpolation reads the project-root .kamal/secrets that
wheels deploy init scaffolds and that DeploySecretsCli and registry
login already use. A deploy.yml outside a config/ directory keeps
resolving .kamal/secrets alongside itself (existing spec unchanged).
Fixes #3084
Signed-off-by: Peter Amiri <peter@alurium.com>
Contributor
There was a problem hiding this comment.
Wheels Bot — Reviewer
TL;DR: This PR fixes #3084 — wheels deploy's ${VAR} interpolation built its lazy SecretResolver from the deploy.yml's own directory, so the standard config/deploy.yml layout looked for secrets at config/.kamal/secrets (a file nothing creates) while DeploySecretsCli and registry login resolve from the project root. The new $projectRootFor() helper steps up to the parent of a config/ directory and leaves every other layout alone. I verified the logic line by line, the cross-component agreement claim, the TDD evidence, and conventions — no blocking findings. Verdict: approve.
Verified during review
- Root-agreement claim is accurate.
DeploySecretsCli.cfc:83(projectRoot: arguments.opts.projectRoot ?: expandPath("./")) andDeployRegistryCli.cfc:81(new ...SecretResolver()→ sameexpandPath("./")default inSecretResolver.cfc:35) both resolve from the project root; withModule.cfc:2181defaultingconfigPathtoexpandPath("config/deploy.yml"),$projectRootFor()now lands interpolation on that same root. All three consumers agree, as the PR body claims. - Destination overlay follows automatically.
SecretResolver.$secretPath()(SecretResolver.cfc:65-72) derives both.kamal/secretsand.kamal/secrets.<dest>from the singleprojectRoot, so the overlay spec is real coverage, not incidental. - Backward compatibility preserved. The pre-existing spec (
ConfigLoaderSpec.cfc:58-70, deploy.yml with.kamal/alongside, noconfig/dir) still exercises the "alongside" layout, and$projectRootFor()returnsdirunchanged for it. - Cross-engine invariant 8 handled correctly.
left(dir, len(dir) - 1)atConfigLoader.cfc:82is guarded bylen(dir) > 1, soLeft(str, 0)can never fire on Lucee 7. The root-path edge (/config/deploy.yml→/) is pinned by the unit spec. - Helper visibility matches house style.
public+$prefix mirrors the existing$overlayPathFor/$interpolate/$resolveVarsiblings in the same CFC, and percli/CLAUDE.mdpublic is warranted here because the spec calls it directly; MCP tool discovery only readsModule.cfc, so no surface leak. - TDD evidence is genuine. Red run failed on exactly the three new specs; the residual 1 fail / 2 errors are the known docker-in-docker / live-server harness artifacts, identical in both runs.
- Docs already match the new behavior. The deploy guides and
wheels deploy initscaffold the project-root.kamal/secrets; this change aligns code with what users were already told. Changelog fragmentchangelog.d/deploy-secrets-project-root.fixed.mduses the correctfixedtype. - Commit (
fix(cli): resolve deploy ${VAR} secrets from the project root for config/deploy.yml) conforms to commitlint (valid type, scope, subject length, explains the why).
Non-blocking observations
listLast(trimmed, "/\") == "config"atConfigLoader.cfc:83compares case-insensitively (CFML==), so/srv/app/Config/deploy.ymlalso steps up. That's the right call for Windows paths and harmless elsewhere — just noting it's deliberate-looking and fine.- With a relative
--configPath config/deploy.yml,getDirectoryFromPath("config")yields no parent and thelen(parent)guard atConfigLoader.cfc:85falls back to the pre-PR behavior — no regression, since the default path is absolute and relative paths behaved this way before.
Nice, tightly-scoped fix with a pinned unit rule and honest evidence. Approving.
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.
Summary
wheels deploy's${VAR}interpolation never read the.kamal/secretsfile thatwheels deploy initscaffolds.ConfigLoader.load()built its lazySecretResolverwithprojectRoot: getDirectoryFromPath(arguments.path), so the standardconfig/deploy.ymllayout resolved secrets fromconfig/.kamal/secrets— a file nothing creates — whileDeploySecretsCli.print()and registry login both resolve from the project root. The process-env fallback masked the gap.Fix
New
ConfigLoader.$projectRootFor(path)helper: when the deploy.yml sits inside a directory namedconfig, the secrets root is the parent of that directory (the project root); any other layout keeps the YAML's own directory, preserving the existing "secrets alongside deploy.yml" behavior the spec suite relies on. Destination overlays (.kamal/secrets.<dest>) follow the same root, sinceSecretResolverderives both paths from the oneprojectRoot.This covers all ten
loader.load()call sites (DeployMainCli, App/Build/Registry/Lock/Prune/Proxy/Server/Accessory CLIs, SshPoolFactory) without plumbing changes, and explicit--configPathvalues outside aconfig/dir behave exactly as before.Acceptance (from #3084)
.kamal/secretsfeeds${VAR}interpolation for the standardconfig/deploy.ymllayout — specresolves ${VAR} from project-root .kamal/secrets when deploy.yml lives in config/ (#3084)$projectRootForunit spec pins the rule)Evidence (Lucee 7 docker harness,
/wheels/cli/tests)Red-run failures were exactly the three new specs (
Expected [acme/fromProjectRoot] but received [acme/], ditto for the destination overlay, andno function with name [$projectRootFor]). The residual 1 fail / 2 errors are identical in both runs and are container-environment artifacts unrelated to this change:SshClientSpec/SshPoolSpecglobal exceptions (docker: command not foundinside the harness container — no docker-in-docker) andServerCommandsSpec :: reload endpoint responds(needs a live CLI-started server).Core suite sanity (
/wheels/core/tests?db=sqlite): 4445 pass / 12 fail / 0 error — only the toleratedinternal.testClientSpecartifacts.Fixes #3084.
🤖 Generated with Claude Code