Apply schema scripts from a console command, rehearsed in CI (task 45) #19
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/schema-in-ci"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
dotnet run --project src/PlaceMark.Databaseapplies pending scripts, taking its connection string fromConnectionStrings__PlaceMark(ADR-0014's key, double-underscore form, so no password in a process list). Also closes a gap the README admitted — the Compose database can now be created from a clone.CI gains a Postgres service container and one
run:-only step invoking the command twice: the first must apply something under the journal-key prefix, the second must find nothing pending. Both assertions verified to fire.Two acceptance criteria deliberately not delivered
There is no test environment and no production — task 42 is not started — so "applies to test on merge to main" and "manual approval gate" have nothing to point at. No workflow was written against them. ADR-0023 fixes their shape for when #42 lands: test is the same command with the environment's connection string as a secret; production is the same command behind
workflow_dispatch, where the dispatch is the gate. Ticket 45 stays open.What the CI step proves — and does not
Proves: the scripts apply cleanly from empty, in resource-name order, through the entry point a deployment invokes rather than a library call a fixture makes, and that they are journalled so a second application does nothing.
Does not prove: anything about applying a future script to a database already holding a previous schema and real rows. That is the residual risk and it is what a provisioned environment buys.
The journal-key prefix grep is a tripwire for the rename hazard — someone who renames the project and updates the grep still gets green; it catches the accident, not the intent.
Watch on the first run
The service container is the one construct here never run on
act_runner v6.4.0. It is not an action, so theuses:defect should not apply — but if the run dies before any step starts, that block is the suspect. Deletingservices:and the schema step restores the pipeline exactly as it was; the workflow header says so.If
postgresdoes not resolve from the job container you will see ten attempts over ~30s then a connect failure — that is service networking, not the schema.If the second run ever reports something applied, the journal did not record the first. That is a real finding, not flakiness.
Also
Fixes
SchemaUpgraderreportingfailed on ''whenever the failure was not a script's — an unreachable database, a bad credential and a malformed connection string all arrive as a failed result with no error script.ADR index row will conflict with PR #18 — both insert after the
0021row. Keep both, in numeric order.91e52bf8fc5b60ebcd89Verdict: mergeable
Independent review of
91e52bf, in a detached worktree. Nothing blocking. Everything below that is not marked nit or observation was executed rather than read.What I ran
All four CI commands, on the head commit:
dotnet restore,dotnet build --configuration Release(0 warnings,TreatWarningsAsErrorson),dotnet test(32 passing — 27 Infrastructure, 4 Api, 1 WebUI),dotnet format --verify-no-changes(exit 0).The entry point against a
postgres:18.4-trixiecontainer, every path:Applied PlaceMark.Database.Scripts.0001-initial-schema.sql, exit 0No pending scripts: the database is already up to date., exit 0ConnectionStrings__PlaceMark, exit 1Port=1)failed.+Failed to connect to 127.0.0.1:1, exit 128P01: password authentication failed, exit 1Format of the initialization string does not conform…, exit 1failed on 'PlaceMark.Database.Scripts.0001-initial-schema.sql'+42710: collation "case_insensitive" already exists, exit 1The failing-script case was produced by clearing
schemaversionswhile leaving the objects, which is also the state the transaction strategy is supposed to prevent.schemaversionsconfirms the journal key is the full resource name.1. Is the honesty right?
Yes, and I would have objected to the alternative. Task 42 is confirmed unstarted in Vikunja, so criteria 1 and 2 name a test database, a production database and a credential to each, none of which exist; a workflow against them is a file whose first execution is the day it matters, which is exactly what the ADR says. Criterion 3 is delivered and demonstrated, not asserted. Task 45 carries the partial-delivery comment and the
0022→0023correction, and stays open. Correct call.Two observations on ADR-0023, raised now only because its body freezes on merge:
workflow_dispatch, where the dispatch is the gate… needs no further record, only wiring" — rests on a mechanism that has never run on this instance either, which is the objection the record itself uses to reject writing the workflows. A clause saying so, or saying that Forgejo has no GitHub-style environment protection rules and soworkflow_dispatchis the only option rather than the chosen one, would stop a later reader having to work out which it was.SchemaUpgraderTests.Upgrade_Always_JournalsEachScriptByItsFullEmbeddedResourceNamealready pins the exact full resource name, so on a rename the Test step goes red first. The grep is a second, prefix-only tripwire at the deployment entry point — still worth having, just not the only one.2. Does the CI step prove what it claims?
Yes, and it is not vacuous. Run 154's log shows the service container pulled, created and run on
FORGEJO-ACTIONS-TASK-171_WORKFLOW-CI_JOB-build-build-networkat11:57:08, before the job container — soservices:does work on act_runner v6.4.0. The step itself loggedApplied PlaceMark.Database.Scripts.0001-initial-schema.sqlat11:57:42.647andNo pending scripts…at11:57:43.172: half a second apart, no retry, and the grep's subject line was produced by an actual apply against an actually empty database.I then ran the step's shell verbatim against a local container with mutations injected. All four branches fire:
-p:RootNamespace=PlaceMark.SchemaRenamed(confirmed the embedded resource becamePlaceMark.SchemaRenamed.Scripts.0001-initial-schema.sql). The run succeeded and applied a script, and the step still failed:Nothing was applied under the expected journal key 'PlaceMark.Database.Scripts.'., exit 1. That is the real hazard reproduced, not a doctored log.The second run applied something, so the first was not journalled., exit 1.The second run failed…, exit 1.ATTEMPTarithmetic gives the 10 the message claims.3.
ConnectionStrings__PlaceMarkand leak riskConsistent with ADR-0014: it is the double-underscore form of the key that record names, environment variables are its stated mechanism for deployed environments, and taking it from the environment rather than
argvkeeps the password out of the process list.No leak. The value in the workflow is the Compose default, and it is the credential of the container it targets, which lives for one job; no step enables
set -x, and the run 154 log dumps no environment. I specifically tried the failure shapes that tend to echo the string back — unknown keyword and unparsable port, both with a password present — and gotCouldn't set bogus/Couldn't set portwith no value. Under ADR-0023's future shape the string becomes a repository secret and is masked as well.Observation, non-blocking:
Runreads the raw environment variable rather than going throughMicrosoft.Extensions.Configuration, so ADR-0014's layering does not reach this command — a developer whose connection string lives in user secrets, asuser-secrets.example.jsonprescribes, has to supply it a second time. The README's copy-paste hard-codes the Compose defaults two paragraphs above the one that invites you to change them in.env; half a sentence would close that.4.
OutputType=Exeon a project a fixture referencesNo problem found, and I looked for the three that usually bite. The test project's output now also carries
PlaceMark.Database(apphost),.deps.jsonand.runtimeconfig.json— inert, VSTest discovery is unaffected, 27 tests pass. There is noProgramambiguity:PlaceMark.Apidoes not referencePlaceMark.Database(checked every.csproj), soWebApplicationFactory<Program>inPlaceMark.Api.Testsis untouched, and both generatedProgramtypes are internal in any case.CA1515(public types in an executable) is not firing atAnalysisLevel 10.0/Recommended— the build is clean with warnings as errors. The ADR's argument against a separate runner project holds.5. The
SchemaUpgraderfixCorrect diagnosis, and the test genuinely pins it. Reverting the line to
$"...failed on '{result.ErrorScript?.Name}'."and rerunning turnsRun_DatabaseUnreachable_ReportsTheFailureAndFailsred withApplying the PlaceMark schema scripts failed on ''.— the exact string the fix exists to remove. Restored and re-ran green.result.ErrorScriptbeing null for a connection-level failure is confirmed against three distinct causes (unreachable, rejected credential, malformed string), and the script-failure path still names the script.6. ADR-0023 and the index
Conforms to ADR-0001 and
template.md: Status, Date and Source; Context / Decision / Alternatives considered / Consequences;NNNN-short-title.md; imperative title; six alternatives with the substantive rejection argued rather than listed. Index row present, its title matches the record's, in numeric order after 0021. PR #18 does add0022-problem-details-in-every-environment.md(checked its file list), so the number is taken rather than skipped and the stated conflict resolution is right. No0022reference anywhere in the tree. ADR-0021 is correctly left unedited despite now being answered.7. Tests
<Method>_<Scenario>_<Expected>throughout, Shouldly throughout, no?.in front of aShould…,Case.Sensitivewhere the string matters. Two nits, neither worth a push:Run_DatabaseIsEmpty_AppliesEveryScriptAndSucceedsasserts one script name, so the name over-promises the moment a second script lands —AppliesTheSchemaAndSucceedswould age better.output.ToString().ShouldContain("already up to date")omitsCase.Sensitivewhile its neighbours in the same file pass it.Verdict: mergeable
Re-review at
f2a67ee, from a clean archive of the head rather than my earlier worktree, so the rebase is included in what follows. All four points are actioned; nothing new is blocking. Two optional clauses in ADR-0023 are noted under 3, worth deciding before that body freezes and not worth another round.Re-verified
CI run 158 is on this head:
checked out f2a67ee9f83a78e913e5bbe33e9ed337a3d38cce, service container created before the job container as before, 47 tests (1 WebUI, 19 Api, 27 Infrastructure), thenApplied PlaceMark.Database.Scripts.0001-initial-schema.sqlfollowed byNo pending scripts: the database is already up to date., job succeeded. The step is still doing real work on the rebased base.Locally: Release build 0 warnings, 47 tests,
formatexit 0. I re-extracted the step from the rebased YAML and ran it underdashagainst a freshpostgres:18.4-trixie— applied, then no-op, exit 0 — and re-ran the entry point's failure paths (variable unset, dead port, wrong password, malformed string): all exit 1, none echoing the connection string.Rebase itself: the index carries
0022and0023as separate rows in numeric order, and 0015'sAccepted (partly superseded by 0022)comes from #18 in the base rather than from here.PlaceMark.Database's sources are byte-identical to91e52bfapart from the new XML-doc paragraph;CONTRIBUTING.mdandSchemaUpgraderTestsuntouched. I reviewed none of #18's content — it is base now.1. The deferred gate
Accepted, and it goes further than I asked. Naming the fallback — somebody runs the command against production by hand — means the record stays true whichever way
workflow_dispatchturns out, so there is no longer an outcome that falsifies it. The instruction to treat a never-dispatched dispatch workflow with the same suspicion as a never-deployed deployment workflow is the right thing to leave for #42.2. The grep claim
Verified rather than read, by reintroducing the rename two ways.
Through the tests:
-p:RootNamespace=PlaceMark.Renamedon the project reddens the Test step with three failures —Upgrade_Always_JournalsEachScriptByItsFullEmbeddedResourceName,Upgrade_ScriptFailsHalfWay_LeavesNothingBehindAndDoesNotJournalItandRun_DatabaseIsEmpty_AppliesEveryScriptAndSucceeds. So "the Test step above reddens on such a rename first" is true as written.Through something the tests are not running: published the renamed project separately and handed the binary to the step, which is the case the narrowed claim reserves for itself. The run succeeded and applied a script, and the step still failed —
Nothing was applied under the expected journal key 'PlaceMark.Database.Scripts.'., exit 1. Both halves hold. I re-ran the second-run assertion on this head too (schema reset between invocations):The second run applied something, so the first was not journalled., exit 1.3. Reading the variable directly — judged
The conclusion is right, keep it. The two arguments are not equally strong, though, and it is worth knowing which one is carrying it.
The
UserSecretsIdargument is decisive, and it is a fact rather than a preference.AddUserSecretsneeds aUserSecretsIdon the assembly andPlaceMark.Databasehas none, so making user secrets work here means either a second store — and ADR-0014 has already written down what a store whose template drifts does, "a stale template is the predictable failure, and it presents as a null value rather than an error", which would now be two stores drifting — or borrowingPlaceMark.Api's identity, which couples a schema tool to a web project for no gain. That alone settles it.The ambiguity argument is directionally right but stated more strongly than the tree supports. "A layered lookup would let it run with nothing set and silently upgrade whichever database some lower layer named" describes something that cannot happen today:
PlaceMark.Databasehas noappsettings.json, so a builder withAddJsonFile(optional: true)plusAddEnvironmentVariables()would resolve to null and produce the same "set this variable" message the command already produces. The hazard becomes real the first time somebody adds a settings file to the project — which is an argument for the rule rather than against it, since the value of "one source, read directly" is precisely that it survives that commit. Optional: a clause along the lines of "there is no such layer in this project today, and this decision is to keep it that way rather than to fix a present bug" would make the record exact instead of predictive.Optional, and the one I would actually take: the Consequences do not name the cost. ADR-0014's delivery is "a fresh clone is one command from a working local configuration"; this command sits outside that, so the Compose credential now appears in three places —
docker-compose.yml,user-secrets.example.jsonand the README block — and a developer who changes.envhas to change two of them. The README comment you added covers the reader who is looking at it; the ADR is where somebody asks later why the schema tool is not configured like everything else. One line under Consequences closes it.For the record, the third option was considered and rejected correctly: a command-line argument would put the password in the process list. I checked again on this head that no failure path — including the unknown-keyword and unparsable-port forms — echoes the string back.
4. The empty-database test
Verified: added a second
.sqltoScripts/, and the assertion failed with the two-line actual against the one-line expected, naming the extra line. Removed it, suite green.ShouldBe([...])over the split lines is the right shape, and the comment is precise about the mechanism — adding a script means adding a line here, which is what turns a script the wildcard failed to embed into a red test rather than a silent pass.Case.Sensitiveon the up-to-date assertion is consistent with its neighbours now.Nothing further from me.