008 layer three: status reads sys_app, dependencies are checked, and the example gets installed for real - #931
Open
wenjianzhang wants to merge 9 commits into
Open
008 layer three: status reads sys_app, dependencies are checked, and the example gets installed for real#931wenjianzhang wants to merge 9 commits into
wenjianzhang wants to merge 9 commits into
Conversation
app-order registered its migrations and its menus and nothing else, so `migrate install order` answered that no application in the binary registers a manifest. Which was true, and made the installer untestable against the one application this repository ships. The manifest goes in the migration package rather than one of its own because that is the package a host has to import for the application to exist at all - its migrations register from there too. A second package would be a second thing to remember to import, and forgetting it would leave an application whose migrations run and which no installer can name. Its Version is not the migration version and the two move independently: adding a migration file without renaming the application is normal, and so is a release that changes no schema. The migration versions decide what runs; this decides what the installed row says. go-admin-core moves to v2.8.0, which is where contract/app lives.
Everything under `migrate install` was covered with an injected engine and a hand-built schema, which is where the shapes belong. What none of it could catch is the wiring: whether an application's init() reaches both registries, whether the installer finds a manifest through app.Snapshot, whether the seeder writes what the uninstaller goes looking for, and whether the command exits non-zero when a migration fails - which a deployment reads to decide whether to start the new version. This builds a go-admin binary with the example application linked in, migrates a real database with it, and drives the whole sequence: framework migrations only, install, install again, put a row in the application's own table, uninstall, reinstall. It lives in its own module. A tagged import in the main module would still be resolved by `go mod tidy`, which considers every build tag and would go looking for github.com/go-admin-team/example-app-order on the network - a repository that does not exist, because the example is a directory inside this one. That was checked rather than assumed: tidy fails there with "Repository not found". A build tag of `ignore` is skipped by tidy but cannot be turned on either, because the standard library uses it for files that are not meant to build at all. A separate module with replace directives is invisible to the main module's tidy, its build, its tests and checksilent, and needs no go.work. Three degradations turn it red: the example application not registering a manifest, the uninstall not clearing sys_migration - where the reinstall then seeds nothing and the assertion reads "menus = 0, want 4" - and the seeder not recording its grants, where the uninstall then leaves every policy behind.
The separate module is invisible to `go test ./...` in the main module, which is the point, and also means nothing would ever run it. This step does.
The migration rows answer "did this run". They cannot answer "is this
application installed", and the difference is not academic: an install that
stopped partway leaves every migration reading applied and a row saying the
install never finished. Until now nothing printed that row.
[order] 1.0.0 failed at order-1793800000000
applied order-1793800000000 2026-09-10 21:02:07
The application list is the union of the two sources rather than either one.
Reading it from sys_app alone would drop an application whose migrations ran
under plain `migrate`, which records no row; reading it from the migration
rows alone drops one whose code has been taken out of the binary, which is
when somebody most wants to see it named - that one now gets a group of its
own, empty, saying why.
A database from before sys_app existed prints exactly what it printed before.
`migrate status` has to keep working on a database that has not been migrated
at all, which is when it is most wanted.
Four degradations turn the new assertions red: dropping the sys_app-only
applications from the listing, printing no summary, not narrowing sys_app by
--app, and reporting an unfinished install as an installed one.
An application's manifest can name others it needs. Until now the list was stored and never read. It is checked, not satisfied. Installing the dependencies too would make "install this application" mean "and everything it happens to name, and everything those name" - a blast radius the operator did not ask for and cannot see beforehand. What they get is the list and the order to do it in. A dependency whose own install failed, or never finished, is not a dependency that is there. The message says which, because the two send you to different places: one to install it, the other to look at why it did not take. The check runs before anything is written, so a refusal cannot cost the operator the row that told them what they had. Separately, a cycle anywhere in the registered manifests is refused, whether or not the application being installed is in it. A cycle between two others is still an authoring mistake, and the day somebody installs into it - with an error naming two applications they did not ask for - is the worse time to find out. The error is the cycle rather than the walk that reached it, and the walk's order is sorted, so the same set of manifests always reports the same one. Requires naming an application that is not registered is not a cycle; it is the database's answer to give, at the time it matters. Six degradations turn the new assertions red: accepting any dependency, accepting a row regardless of its status, returning no cycle, not trimming the reported path to the cycle itself, and running the check after the row has already been written - the last of which was rebuilt after the first attempt at it deleted the check rather than moving it, and so went red on the wrong assertion.
Cleanup from a review pass over this branch. No behaviour changes except the two noted below. runInstall took app.Snapshot() twice, once inside manifestFor and once for the cycle check. Snapshot is a deep copy of the registry, and worse than the copying, the two calls could in principle disagree - the set the cycle check validated was not provably the set the manifest came from. One snapshot, passed to both. appSummary converted a display code back to a stored one with NormalizeAppCode, which is not that inverse: it leaves "core" as "core", so the framework needed a branch of its own to stay out of the listing. AppFilter is the documented inverse and maps it to the empty string, which is not a code any row is filed under - so the branch goes, and the function now matches filterAppsByApp twenty lines below it, which was already using AppFilter. That branch only half-covered what it guarded: a sys_app row carrying an empty or reserved app_code was still merged into the framework's group by groupByApp, with only its summary suppressed. loadApps now drops such rows, which is the one place that settles it for every reader of the map. requiresInstalled built two parallel slices with a tuple assignment repeated in three branches; it now picks a reason and appends once. Its last arm was a catch-all on "not installed", so a status constant added later would have been described as "did not finish" - a sentence that would be wrong for whatever reason the constant was added. Unrecognised values now say so. It also takes the normalised code the caller already has rather than computing it a third time. refuseOnDependencyCycle sorted each manifest's Requires before walking them. Requires is a slice and already has a fixed order, so the sort bought no determinism - that comes from the sorted outer loop, which walks a map - and only made a reported cycle harder to line up against the manifest that caused it. The filtering pass that went with it is covered by the registration check underneath. The cycle path is trimmed with slices.Index, which also removes a fallback return that the grey/path invariant made unreachable.
…hecks Two tests in this package each wrote out what an installed sys_app row looks like, field for field, and a third inlined the same Create with a different status. One appRow(t, db, code, status) now covers all three, so a new NOT NULL column on SysApp is one edit rather than three. One assertion counted with a bare db.Model(...).Count(&n) and dropped the error that call returns. A failing query leaves n at zero, which is exactly what that assertion wanted to see - so the test would have passed on a broken query. The package already had a count helper that fails on the error, and this now uses it. Also a cycle reached from outside itself. The existing case walks straight into its own cycle from the first code, so the path trimming had nothing to do and replacing it with the untrimmed path left the test green - the trimming was never covered. With a requiring b, b requiring c and c requiring b, the untrimmed report names a as part of a cycle it is not in, and the test goes red.
Three tests each called newEnv, and newEnv built the binary, so the same binary was linked three times - about 17 seconds of the run, measured. A binary is read-only and there is nothing to isolate between tests; each test still gets its own directory and its own database. The package now builds it on the first test that needs one and removes it in TestMain. The suite goes from 39 seconds to 9. The three assertion blocks listed the same six queries, two or three times each, differing only in the counts expected - so renaming a table meant finding three places. They now share one list, with a flag for the uninstall's "all of them at zero". The reinstall check gets stronger on the way past: it was three of the six and is now all six. The per-call sql.Open in count and exec stays. It looks like waste and is not: the binary under test writes the same file, and a connection held open across a run of it is a second writer for nothing. The shared part is factored out; the opening is still per call, and the comment now says why.
It was an inlined `go test` in the workflow, and the only thing in the build that runs it. Every other gate there goes through make - make test, make build, make checksilent - and `make test` is `go test ./...` in this module, which cannot reach test/e2e-apporder because that is a module of its own. So the one check that exercises installing an application was the one check a developer had no command for, and the only place it could turn red was after pushing.
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.
Until this branch nothing had ever installed an application.
migrate install orderanswered that no application in the binary registers a manifest - whichwas true, and left every path past the installer's first check covered only by
unit tests with an injected engine and a hand-built schema.
The example application says what it is
app-order registered its migrations and its menus and no manifest. It has one
now, in the migration package, because that is the package a host must import
for the application to exist at all - a second package would be a second thing
to remember to import, and forgetting it leaves an application whose
migrations run and which no installer can name.
Its version is not the migration version, and the two move independently:
adding a migration file without renaming the application is normal, and so is
a release that changes no schema.
And now it actually gets installed
test/e2e-apporderbuilds a go-admin binary with the example linked in,migrates a real database with it, and drives the sequence: framework
migrations only, install, install again, write a row in the application's own
table, uninstall, reinstall.
It is a module of its own, and that was not the first plan. A build-tagged
import in the main module is still resolved by
go mod tidy, which considersevery build tag and goes looking for
github.com/go-admin-team/example-app-orderon the network - a repository that does not exist, because the example is a
directory inside this one. Checked, not assumed: tidy fails there with
Repository not found. A tag ofignoreis skipped by tidy but cannot beturned on either, because the standard library uses it for files that are not
meant to build. A separate module with replace directives is invisible to the
main module's tidy, build, tests and checksilent, and needs no
go.work.Its cost is worth stating: its
go.sumrestates the main module's transitivedependency graph, and nothing keeps the two in step. It cannot silently
downgrade anything - MVS takes the maximum of both - but a dependency bump
here will leave it un-tidied until somebody remembers.
migrate status says what sys_app knows
The migration rows answer "did this run". They cannot answer "is this
application installed":
The application list is the union of both sources. From
sys_appalone itwould drop an application whose migrations ran under plain
migrate, whichrecords no row; from the migration rows alone it drops one whose code has been
taken out of the binary, which is when somebody most wants to see it named. A
database from before
sys_appexisted prints exactly what it printed before.Dependencies are checked, not satisfied
Installing them too would make "install this application" mean "and everything
it happens to name, and everything those name". What the operator gets is the
list and the order to do it in. A dependency whose own install failed, or
never finished, is named as such - the two send you to different places.
A cycle anywhere in the registered manifests is refused on any install,
whether or not the application being installed is part of it, and the error is
the cycle rather than the walk that reached it.
Verification
29 degradations, each expected to turn a named assertion red, and each did.
Two were rebuilt and re-run after the first attempt at them went red on the
wrong assertion - one failed in a test's setup, the other deleted the check it
was supposed to be moving.
The end-to-end path also confirmed the exit code this branch's parent changed:
a failed migration exits 1, a successful one exits 0. A deployment reads that
to decide whether to start the new version.
A review pass over the branch
Four review passes (reuse, simplification, efficiency, altitude) ran over the
diff; eleven findings were applied and are in the last four commits. The ones
worth naming here:
Count(&n)and dropped the error itreturns. A failing query leaves
nat zero, which is what the assertionwanted to see, so a broken query would have passed.
the suite goes from 39 seconds to 9.
going red. Not a regression: the cycle-path trimming had never been covered,
because the only case walked straight into its own cycle from the first
code, where the trim is a no-op. A cycle reached from outside itself now
covers it.
make test-e2e. The one check that exercises installing an application wasthe one check with no local command, reachable only by pushing.
Two findings were skipped with reasons: the per-call
sql.Openin theend-to-end helpers stays, because the binary under test writes the same file
and a held-open connection is a second writer for nothing; and the cycle check
stays at the command layer rather than moving inside
install(), because acycle is a property of the registry and
requiresInstalledis a property ofthis install against this database - different facts, different layers.
Still open
sys_apphas noCreatedAt, so a row stuck atinstallingcarries no timethe attempt started, and nobody can answer "how long has this been stuck".