Skip to content

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
masterfrom
feat/008-wire-example
Open

008 layer three: status reads sys_app, dependencies are checked, and the example gets installed for real#931
wenjianzhang wants to merge 9 commits into
masterfrom
feat/008-wire-example

Conversation

@wenjianzhang

Copy link
Copy Markdown
Member

Until this branch nothing had ever installed an application. migrate install order answered that no application in the binary registers a manifest - which
was 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-apporder builds 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 considers
every build tag and goes 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. Checked, not assumed: tidy fails there with
Repository not found. A 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. 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.sum restates the main module's transitive
dependency 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":

[order]  1.0.0 failed at order-1793800000000
  applied   order-1793800000000  2026-09-10 21:02:07

The application list is the union of both sources. From sys_app alone it
would drop an application whose migrations ran under plain migrate, which
records 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_app existed 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:

  • One assertion counted with a bare Count(&n) and dropped the error it
    returns. A failing query leaves n at zero, which is what the assertion
    wanted to see, so a broken query would have passed.
  • The end-to-end suite built the binary once per test. It builds once now, and
    the suite goes from 39 seconds to 9.
  • Re-running the counter-proofs after the refactor found one that had stopped
    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 was
    the one check with no local command, reachable only by pushing.

Two findings were skipped with reasons: the per-call sql.Open in the
end-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 a
cycle is a property of the registry and requiresInstalled is a property of
this install against this database - different facts, different layers.

Still open

sys_app has no CreatedAt, so a row stuck at installing carries no time
the attempt started, and nobody can answer "how long has this been stuck".

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.
Copilot AI lite review requested due to automatic review settings September 11, 2026 01:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants