fix(api/dashboard): interpolate Compose vars from the configured deploy env - #407
Merged
Merged
Conversation
A compose file declaring a required variable (`${VAR:?message}`) was
scanned against the repo `.env` alone. Env the user had configured in
Openship never reached the parser, so the scan reported the file as
unparseable even though the deploy would resolve the same variable:
Could not parse the Docker Compose file at "deploy/docker-compose":
Set POSTGRES_PASSWORD in .env
`parseComposeFile` already accepts explicit interpolation values that
override the ones loaded from `envFileContent`. Thread the caller's env
to it: `Source` -> `ResolveOptions` -> `resolveFromReader` ->
`toProjectInfo`. `ResolveOptions` already reaches both the GitHub and
local resolvers, so neither needed changing.
Omitting the env keeps the existing behaviour — a genuinely unset
required variable is still reported rather than silently dropped.
Expose the resolver's new interpolation env on the scan route so the wizard can send what the user already entered. Declared on `PrepareDeployBody` as a string->string record, so the shape is validated at the trust boundary like every other env map on the API. The value is interpolation-only: it is not persisted here, and the response already masks every service env via `maskScanService`, so a supplied secret cannot be echoed back unmasked.
`rescanWithComposePath` re-reads the source because projectType, the service list and each service's env can only come from the compose file. On a file with required variables that re-scan failed, since the env the user had just entered was not part of the request. Carry `config.envVars` through both initialize paths into the prepare body. Empty maps are dropped so a blank value never reaches the API, matching how `scanComposePath` handles an unset pin.
There was a problem hiding this comment.
Pull request overview
This PR fixes Compose wizard scans failing on required ${VAR:?message} substitutions by interpolating Docker Compose using the deployment’s configured environment (in addition to the repo-adjacent .env), so scans succeed when the operator has already provided the needed variables.
Changes:
- Adds an optional
envmap to the prepare request schema/types and threads it through API resolution intoparseComposeFile. - Updates the dashboard prepare call path (
rescanWithComposePath) to include already-configured env vars for interpolation. - Adds an API regression test covering a pinned Compose subpath with a required variable.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| apps/dashboard/src/lib/api/deploy.ts | Extends PrepareProjectSource to allow sending an optional env map for compose interpolation. |
| apps/dashboard/src/context/deployment/useDeploymentConfig.ts | Builds and forwards env during rescan flows so required vars don’t break Compose parsing. |
| apps/api/test/modules/deployments/prepare.service.test.ts | Adds a regression test proving required vars interpolate from configured env with a pinned compose path. |
| apps/api/src/modules/deployments/prepare.service.ts | Plumbs env through resolution and passes it into parseComposeFile options. |
| apps/api/src/modules/deployments/deployment.schema.ts | Adds optional env to PrepareDeployBody at the validation boundary. |
| apps/api/src/modules/deployments/deployment.controller.ts | Forwards optional env into the prepare source while keeping masked output behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
A Compose file that declares a required variable (
${VAR:?message}) fails the wizard scan even when the operator has already entered that variable in the Openship deploy configuration. The scan interpolates against the repo.envalone, so the configured env never reaches the parser. Thread that env through toparseComposeFile, which already accepts it.Motivation
toProjectInfois the single place the wizard parses Compose (prepare.service.ts:780onmain):composeEnvContentis a.envread from beside the Compose file. It is the only interpolation source.SourceandResolveOptionscarry no env at all, so there is no way for a caller to supply one — the values the operator typed into the wizard are simply not in scope at the point the file is parsed.Compose treats
${VAR:?message}as fatal whenVARis unset, so the parse throws and the wizard reports the file as broken. Reproduced against0f59f94fwith the reporter's layout — Compose pinned outside the repo root (thecomposePathfeature from #330), one required variable, supplied through the deploy configuration:mainPOSTGRES_PASSWORD=s3cretError: Could not parse the Docker Compose file at "deploy/docker-compose": Set POSTGRES_PASSWORD in .envservices[db].environment.POSTGRES_PASSWORD === "s3cret"The second row is the behaviour worth keeping: a genuinely unset required variable must still be reported. #339 established that swallowing a parse failure returns a services project with zero services and no reason why, and the existing
reports missing required Compose variablestest pins that. This change only suppresses the error when the operator actually provided the value.The parser side already supports this.
ComposeParseOptions.envis documented as "Explicit interpolation values. Overrides values loaded fromenvFileContent" (compose-parser.ts:69) andbuildInterpolationEnvlayers it over the.envmap (:424). Nothing new was needed there — only the plumbing to reach it.Related issue
Closes #383
Changes
apps/api
modules/deployments/prepare.service.ts—ResolveOptionsgainsenv, mirrored onto bothSourcevariants; forwarded throughresolveFromReaderintotoProjectInfoand on toparseComposeFile.ResolveOptionsalready reaches bothresolveFromGitHubandresolveFromLocal, so neither resolver needed touching.modules/deployments/deployment.schema.ts—PrepareDeployBody.env, aType.Record(Type.String(), Type.String()), matching howproject.schema.tsandservice.schema.tsalready declare env maps. The route is validated, so the shape is checked at the trust boundary rather than in the handler.modules/deployments/deployment.controller.ts— pass the body'senvinto both the github and localSource. Interpolation-only: not persisted here, and the response already masks every service env throughmaskScanService(Service environment secrets returned in plaintext in scan/deployment/service API responses #336), so a supplied value cannot be echoed back unmasked.test/modules/deployments/prepare.service.test.ts— one regression test on the existing tmpdir harness, reproducing the reporter's nesteddeploy/docker-composelayout.apps/dashboard
context/deployment/useDeploymentConfig.ts— newscanEnvhelper foldsconfig.envVarsinto a record;rescanWithComposePathcarries it through both initialize paths. Empty maps are dropped so a blank value never reaches the API, matching how the neighbouringscanComposePathhandles an unset pin.lib/api/deploy.ts—envonPrepareProjectSource.rescanWithComposePathis the path that matters here: it re-reads the source precisely because projectType, the service list and each service's env can only come from the Compose file, so it is the one flow where the operator has already entered env and then triggers a fresh parse.Verification
RED first, on the branch with the fix reverted — the new test reproduces the report verbatim:
GREEN, and the pre-existing missing-variable test still passes beside it:
Full API suite, two separate clean runs:
Dashboard suite and typecheck:
Re-run against committed
HEADafter the three commits: 16/16 on the focused file, old failure gone.Two notes on the checklist rather than a silent tick:
bun formatis not run. All five touched files already failprettier --checkonmain— verified by checking outmainand running it there — so formatting them would reformat lines this PR has no business touching.turbo run lint --filter=@repo/dashboardfails onmainas well as here, withInvalid project directory provided, no such directory: apps/dashboard/lint. Thenext lintscript is incompatible with the installed Next; unrelated to this change. Usedtsc --noEmiton the dashboard instead, which is clean.Questions for a maintainer
@Hydralerne — a few calls I would rather you make than assume:
Scope of the endpoint change. The template asks for prior agreement on new endpoints and schema changes. This adds an optional field to an existing route's body rather than a new endpoint, and I read it as within "bug fix" — but it is a request-shape change, so say the word if you would rather it went through an issue first.
Three sibling callers left unwired, deliberately.
resolveProjectInfohas three other callers with the identical gap, and I did not touch them because each needs a decision about where project env is read from:reconcileComposeDrift(build.service.ts:470) — the interesting one. Its parse failure is caught into aconsole.warn, so on a Compose file with required variables, drift reconciliation silently stops tracking upstream changes and nothing surfaces. It has the project record in hand, so wiring it is a question of which env you consider authoritative there.detectStack(github.controller.ts:834) — a GET, so env would have to arrive as a query parameter or the route would need to change shape.scanLocal(project.controller.ts:923).Happy to do any or all in this PR or a follow-up — tell me which, and where env should come from for the drift path.
No dashboard test.
apps/dashboardhas no jsdom or Testing Library; its render tests userenderToStaticMarkup, which runs no effects, so a hook likerescanWithComposePathcannot be exercised. I did not add a test framework to cover it. If you would rather that half were covered, that is a jsdom addition and a bigger conversation than this fix.The API half is genuinely covered — real resolver, real parser, real temp filesystem.
Checklist
bun run test,bun run --cwd <workspace> lint, andbun formatall pass locally — see the two notes under Verification:bun formatdeliberately not run, and dashboardlintis broken onmainindependently of this change