fix(scripts): resolve backup/restore config the way the application does - #1042
Merged
Conversation
Both scripts read every path from the process environment alone, while the app fills the same settings from three layers (src/config/load-env.ts): the environment, then ./.env, then <data dir>/.env.generated — the file Dashboard > Infrastructure writes. An install configured through the dashboard was therefore backed up at the DEFAULT paths. That is not reliably loud. backup_sqlite already fails hard on a missing database, but a database left at a default path from BEFORE the operator switched is archived instead and the run exits 0. A backup that captured an abandoned database announces itself only during a restore. backup.sh was already copying .env.generated into the archive without ever reading it, and the runbook pushed the gap onto the operator by telling them to restate the paths by hand. lib-env.sh resolves a key through the three layers in the app's order, so an explicit environment value still wins — the new smoke case asserts that direction too, since silently inverting the precedence would be a worse bug than the one being fixed. Nothing is exported: each key is looked up by name, so a stray entry in an operator's .env cannot reach the script's environment. Only plain KEY=value lines are honoured; a value carrying quotes or a trailing # comment is reported and skipped rather than guessed at, because a silently mis-parsed path is exactly the failure this exists to prevent. The Postgres details pg_dump uses resolve the same way, so a dashboard-provisioned database no longer needs its credentials restated in the operator's shell. Smoke case (f) reproduces the dangerous shape specifically: a stale database at the default path while .env.generated names the live one. It failed before this change with the stale file archived and a zero exit.
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
1 participant
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.
The failure
backup.shandrestore.shread every path from the process environment alone. The applicationfills the same settings from three layers (
src/config/load-env.ts):./.env<data dir>/.env.generated— written by Dashboard > InfrastructureSo an install configured through the dashboard was backed up at the default paths.
That is not reliably loud.
backup_sqlitealready fails hard when a database is missing, whichcovers the obvious case. The dangerous shape is the other one: a database left at a default path from
before the operator switched. Then the run archives the abandoned file and exits
0— a backup thatannounces itself as wrong only during a restore.
backup.shwas already copying.env.generatedinto the archive without ever reading it, anddocs/11-operational-runbooks.mdpushed the gap onto the operator, telling them to restateMAIN_DATABASE_NAME/DATABASE_NAMEby hand "when the app overrides them".The change
scripts/lib-env.shresolves a key through the three layers in the application's order. Both scriptsuse it for the database paths, the state directories, and the Postgres connection details
pg_dumpneeds — so a dashboard-provisioned database no longer needs its credentials restated in the
operator's shell.
Deliberate constraints:
one being fixed, so the new smoke case asserts that direction too.
.envcan never reach the script's own environment.
KEY=valuelines are honoured. A value carrying quotes or a trailing#comment isreported and skipped rather than guessed at — a silently mis-parsed path is precisely the failure
this exists to prevent.
.env.generatedis machine-written and always in this form; the warningexists for hand-edited
.envfiles.OPENWA_DATA_DIRandBACKUP_DIRsteer the script rather than the application and are neverwritten to an env file, so they stay environment-only.
Tests
New smoke case (f) reproduces the dangerous shape exactly: a stale database sitting at the default
path while
data/.env.generatednames the live one. It asserts four things — backup takes the livedatabase, restore writes to the path the same layer names, and an explicit environment value still
overrides both.
Watched failing first, on the real defect:
The precedence assertion is separately mutation-proven, since the first failure short-circuits before
it runs — inverting the lookup order in
lib-env.shproduces:Verification
shellcheck scripts/*.sh(0.11.0, the CI version) — clean, including the newlib-env.sh