From 139f5e394246778af020e53a85162157f2cb047c Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 04:30:36 +0200 Subject: [PATCH] fix(lagoon): keep Status plugin-only; migrate:status adds the framework sets itself (WR-12) 57a849d made lagoon.Status read migrationSets, which changed its result for consumers: the framework sets came first and a plugin with no migrations lost its row. Status is back to its pre-57a849d behaviour. migrate:status (table and --json) now reads the unexported setStatus, which lists the framework sets and the plugins from migrationSets, so it still agrees with migrate --pretend. Both share historyRows. - TestStatusPluginRows pins Status: plugin rows only, an empty row for a plugin with no migrations or no history table, none without HasMigrations - TestMigrateStatusJSON also checks the table lists the framework sets - TestTwoPluginMigrationSetsIsolated and TestDocsMigrate are back to the plugin-only rows --- docs/database/migrations.md | 2 + modules/lagoon/README.md | 2 +- modules/lagoon/commands.go | 2 +- modules/lagoon/example_test.go | 11 +---- modules/lagoon/migrations.go | 47 +++++++++++++++----- modules/lagoon/migrations_test.go | 71 +++++++++++-------------------- modules/lagoon/pending_test.go | 46 ++++++++++++++++++++ scripts/check-phase19.8.sh | 2 + 8 files changed, 116 insertions(+), 67 deletions(-) diff --git a/docs/database/migrations.md b/docs/database/migrations.md index 68703c1b..f15dbaaf 100644 --- a/docs/database/migrations.md +++ b/docs/database/migrations.md @@ -104,6 +104,8 @@ if err := lagoon.RollbackLast(db, plugins, "acme.blog"); err != nil { `lagoon.Migrate`, `lagoon.Status` and `lagoon.RollbackLast` take the activated plugins; a plugin that does not implement `pact.HasMigrations` is skipped. +`lagoon.Status` returns one row per plugin, a plugin with no migrations included, and leaves out the framework's own sets that `migrate:status` prints. + ## Porting version.yaml WinterCMS runs a plugin's update scripts by version number and seeds data from the same files. When you port a plugin: diff --git a/modules/lagoon/README.md b/modules/lagoon/README.md index 33072959..d37855e7 100644 --- a/modules/lagoon/README.md +++ b/modules/lagoon/README.md @@ -135,7 +135,7 @@ func (p *Plugin) Migrations() []*gormigrate.Migration { | `lagoon.DSN` | Reads `database.dsn` from config. | | `lagoon.Migrate` | Runs framework and plugin migrations in order. | | `lagoon.RollbackLast` | Rolls back the last migration of one plugin. | -| `lagoon.Status` | Lists the applied migration IDs of every set `lagoon.Migrate` applies, framework sets first (under their history IDs, such as `summercms.attach`), then each plugin with migrations, as `lagoon.StatusRow` values. | +| `lagoon.Status` | Lists applied migration IDs per plugin as `lagoon.StatusRow` values: one row for each plugin that implements `pact.HasMigrations`, in plugin order; a plugin with no migrations, or without a history table yet, gets a row without IDs. The framework's own sets are not listed; `migrate:status` adds them. | | `lagoon.DatabaseLabel` | Describes a DSN for a person as `host:port/name` (parsed with `pgconn.ParseConfig`), never the user or password; `not configured` when empty, `configured` when it does not parse. Used by the startup summaries of `serve` and `queue:work`. | | `lagoon.PendingMigrations` | Lists the migrations `lagoon.Migrate` would apply, as `lagoon.MigrationRef` values (`Plugin`, `ID`): the framework sets first, then each plugin in order, each in declaration order. A missing history table means the whole set is pending. Read-only; `serve` reports the count in its startup summary. | | `lagoon.HistoryTableName` | Returns the gormigrate history table for a plugin ID. | diff --git a/modules/lagoon/commands.go b/modules/lagoon/commands.go index 4f65b363..acb472c6 100644 --- a/modules/lagoon/commands.go +++ b/modules/lagoon/commands.go @@ -279,7 +279,7 @@ sets with its table and whether it is applied, for scripts.`, }, Run: func(ctx context.Context, in bonfire.Input, out bonfire.Output) error { return withDB(ctx, app, func(gdb *gorm.DB) error { - rows, err := Status(gdb, plugins) + rows, err := setStatus(gdb, plugins) if err != nil { return err } diff --git a/modules/lagoon/example_test.go b/modules/lagoon/example_test.go index 483eb6fa..2e0573b8 100644 --- a/modules/lagoon/example_test.go +++ b/modules/lagoon/example_test.go @@ -359,16 +359,9 @@ func TestDocsMigrate(t *testing.T) { if err != nil { t.Fatal(err) } - // The framework's own sets come first (WR-12), then the plugin. - framework := []string{"summercms.attach", "summercms.cabana", lagoon.DeferredHistoryID, lagoon.QueueHistoryID} want := "acme.blog summer_migrations_acme_blog [20260101000100_create_posts 20260101000200_create_comments_and_categories]" - if len(lines) != len(framework)+1 || lines[len(lines)-1] != want { - t.Fatalf("status = %q, want the framework sets and then %q", lines, want) - } - for i, id := range framework { - if !strings.HasPrefix(lines[i], id+" ") { - t.Errorf("status line %d = %q, want the framework set %s", i, lines[i], id) - } + if len(lines) != 1 || lines[0] != want { + t.Fatalf("status = %q, want %q", lines, want) } if db.Migrator().HasTable("acme_blog_comments") { t.Fatal("rollback left acme_blog_comments") diff --git a/modules/lagoon/migrations.go b/modules/lagoon/migrations.go index a351efe9..d100bd5b 100644 --- a/modules/lagoon/migrations.go +++ b/modules/lagoon/migrations.go @@ -130,20 +130,34 @@ func lastMigrationPlugin(plugins []party.Plugin) string { return "" } -// StatusRow is one migration set's recorded IDs: Plugin is the plugin ID or -// the framework set's history ID (for example summercms.attach), Table its -// history table. +// StatusRow is one plugin's recorded migration ids. type StatusRow struct { Plugin string Table string IDs []string } -// Status reads the history table of every set Migrate applies, in the -// same order: the framework's own sets first (uploads, backend admin, -// deferred bindings, job queue), then each plugin with at least one -// migration. A missing history table gives a row without IDs. +// Status reads each plugin history table: one row per plugin that +// implements pact.HasMigrations, in plugin order, a plugin with no +// migrations included. The framework's own sets are not listed. func Status(gdb *gorm.DB, plugins []party.Plugin) ([]StatusRow, error) { + if gdb == nil { + return nil, fmt.Errorf("lagoon: gorm db is nil") + } + var ids []string + for _, p := range plugins { + if _, ok := p.(pact.HasMigrations); ok { + ids = append(ids, p.ID()) + } + } + return historyRows(gdb, ids) +} + +// setStatus reads the history table of every set Migrate applies, in its +// order (migrationSets): the framework's own sets first, then each plugin +// with at least one migration. migrate:status prints it, so that it agrees +// with migrate --pretend. +func setStatus(gdb *gorm.DB, plugins []party.Plugin) ([]StatusRow, error) { if gdb == nil { return nil, fmt.Errorf("lagoon: gorm db is nil") } @@ -151,19 +165,30 @@ func Status(gdb *gorm.DB, plugins []party.Plugin) ([]StatusRow, error) { if err != nil { return nil, err } - var rows []StatusRow + ids := make([]string, 0, len(sets)) for _, s := range sets { - table, err := HistoryTableName(s.id) + ids = append(ids, s.id) + } + return historyRows(gdb, ids) +} + +// historyRows reads the applied IDs of each history ID's table, sorted. A +// missing history table gives a row without IDs (nil), an empty one a row +// with an empty slice. +func historyRows(gdb *gorm.DB, ids []string) ([]StatusRow, error) { + var rows []StatusRow + for _, id := range ids { + table, err := HistoryTableName(id) if err != nil { return nil, err } - row := StatusRow{Plugin: s.id, Table: table} + row := StatusRow{Plugin: id, Table: table} if !gdb.Migrator().HasTable(table) { rows = append(rows, row) continue } if err := gdb.Table(table).Order("id").Pluck("id", &row.IDs).Error; err != nil { - return nil, fmt.Errorf("lagoon: status %s: %w", s.id, err) + return nil, fmt.Errorf("lagoon: status %s: %w", id, err) } if row.IDs == nil { row.IDs = []string{} diff --git a/modules/lagoon/migrations_test.go b/modules/lagoon/migrations_test.go index 3bdb4d49..e04b6da6 100644 --- a/modules/lagoon/migrations_test.go +++ b/modules/lagoon/migrations_test.go @@ -186,43 +186,18 @@ func TestTwoPluginMigrationSetsIsolated(t *testing.T) { t.Fatalf("idempotent migrate: %v", err) } - // Status lists the sets Migrate applies (WR-12): the framework sets - // first, in migrationSets order, then the plugins. - statusOf := func(t *testing.T) (alpha, beta StatusRow) { - t.Helper() - status, err := Status(gdb, plugins) - if err != nil { - t.Fatal(err) - } - framework := []string{"summercms.attach", "summercms.cabana", DeferredHistoryID, QueueHistoryID} - if len(status) != len(framework)+2 { - t.Fatalf("status rows = %+v", status) - } - for i, id := range framework { - if status[i].Plugin != id { - t.Fatalf("status row %d = %+v, want the framework set %s", i, status[i], id) - } - } - found := map[string]StatusRow{} - for _, row := range status[len(framework):] { - found[row.Plugin] = row - } - var ok bool - if alpha, ok = found["demo.alpha"]; !ok { - t.Fatalf("no demo.alpha row: %+v", status) - } - if beta, ok = found["demo.beta"]; !ok { - t.Fatalf("no demo.beta row: %+v", status) - } - return alpha, beta + status, err := Status(gdb, plugins) + if err != nil { + t.Fatal(err) } - - alphaRow, betaRow := statusOf(t) - if alphaRow.Table != "summer_migrations_demo_alpha" || strings.Join(alphaRow.IDs, ",") != "202609170001_create_alpha" { - t.Fatalf("alpha status = %+v", alphaRow) + if len(status) != 2 { + t.Fatalf("status rows = %+v", status) } - if betaRow.Table != "summer_migrations_demo_beta" || strings.Join(betaRow.IDs, ",") != "202609170001_create_beta,202609170002_seed_beta" { - t.Fatalf("beta status = %+v", betaRow) + if status[0].Plugin != "demo.alpha" || status[0].Table != "summer_migrations_demo_alpha" || strings.Join(status[0].IDs, ",") != "202609170001_create_alpha" { + t.Fatalf("alpha status = %+v", status[0]) + } + if status[1].Plugin != "demo.beta" || status[1].Table != "summer_migrations_demo_beta" || strings.Join(status[1].IDs, ",") != "202609170001_create_beta,202609170002_seed_beta" { + t.Fatalf("beta status = %+v", status[1]) } if !gdb.Migrator().HasTable("lagoon_alpha") || !gdb.Migrator().HasTable("lagoon_beta") { @@ -250,12 +225,15 @@ func TestTwoPluginMigrationSetsIsolated(t *testing.T) { t.Fatal("schema rollback must not run on the first beta RollbackLast") } - alphaRow, betaRow = statusOf(t) - if strings.Join(alphaRow.IDs, ",") != "202609170001_create_alpha" { - t.Fatalf("alpha history changed: %+v", alphaRow) + status, err = Status(gdb, plugins) + if err != nil { + t.Fatal(err) } - if strings.Join(betaRow.IDs, ",") != "202609170001_create_beta" { - t.Fatalf("beta history = %+v", betaRow) + if strings.Join(status[0].IDs, ",") != "202609170001_create_alpha" { + t.Fatalf("alpha history changed: %+v", status[0]) + } + if strings.Join(status[1].IDs, ",") != "202609170001_create_beta" { + t.Fatalf("beta history = %+v", status[1]) } if err := RollbackLast(gdb, plugins, "demo.beta"); err != nil { @@ -268,12 +246,15 @@ func TestTwoPluginMigrationSetsIsolated(t *testing.T) { t.Fatal("alpha table must survive beta rollback") } - alphaRow, betaRow = statusOf(t) - if len(alphaRow.IDs) != 1 { - t.Fatalf("alpha history after beta down = %+v", alphaRow) + status, err = Status(gdb, plugins) + if err != nil { + t.Fatal(err) } - if len(betaRow.IDs) != 0 { - t.Fatalf("beta history should be empty, got %+v", betaRow) + if len(status[0].IDs) != 1 { + t.Fatalf("alpha history after beta down = %+v", status[0]) + } + if len(status[1].IDs) != 0 { + t.Fatalf("beta history should be empty, got %+v", status[1]) } } diff --git a/modules/lagoon/pending_test.go b/modules/lagoon/pending_test.go index e825c400..8f770937 100644 --- a/modules/lagoon/pending_test.go +++ b/modules/lagoon/pending_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "reflect" "slices" "strings" "testing" @@ -334,6 +335,51 @@ func TestMigrateStatusJSON(t *testing.T) { t.Errorf("%v: pending entries lack \"applied\": false:\n%s", args, out) } } + + // The table lists the same sets: the framework's own first, in the + // order migrate applies them, then the plugins. + out, errOut, code, _ := runLagoon(t, "development", dsn, plugins, "migrate:status") + if code != bonfire.ExitOK { + t.Fatalf("migrate:status: exit %d\n%s", code, errOut) + } + var sets []string + for _, line := range strings.Split(strings.TrimSpace(out), "\n")[1:] { + sets = append(sets, strings.Fields(line)[0]) + } + wantSets := []string{"summercms.attach", "summercms.cabana", DeferredHistoryID, QueueHistoryID, "demo.alpha", "demo.beta"} + if !slices.Equal(sets, wantSets) { + t.Errorf("migrate:status sets = %q, want %q:\n%s", sets, wantSets, out) + } +} + +// TestStatusPluginRows: Status lists the plugins only, never the +// framework's own sets (migrate:status adds those itself). Every plugin +// that implements pact.HasMigrations gets a row in plugin order, one with +// no migrations and one whose history table does not exist yet included; +// a plugin without HasMigrations gets none. +func TestStatusPluginRows(t *testing.T) { + db, _ := dedicatedDB(t, "lagoon_status_plugin_rows") + gdb, err := Use(t.Context(), db) + if err != nil { + t.Fatal(err) + } + alpha, beta := pendingPlugins() + empty := migPlugin{id: "demo.empty"} + if err := Migrate(gdb, []party.Plugin{alpha}); err != nil { + t.Fatal(err) + } + rows, err := Status(gdb, []party.Plugin{alpha, empty, stubPlugin{id: "demo.plain"}, beta}) + if err != nil { + t.Fatal(err) + } + want := []StatusRow{ + {Plugin: "demo.alpha", Table: "summer_migrations_demo_alpha", IDs: []string{"202610210001_create_pending_alpha"}}, + {Plugin: "demo.empty", Table: "summer_migrations_demo_empty"}, + {Plugin: "demo.beta", Table: "summer_migrations_demo_beta"}, + } + if !reflect.DeepEqual(rows, want) { + t.Fatalf("Status = %+v, want %+v", rows, want) + } } // TestMigrateAppliedLines (WR-07): migrate's "N applied" lines count what diff --git a/scripts/check-phase19.8.sh b/scripts/check-phase19.8.sh index f3f879c5..58f83bae 100755 --- a/scripts/check-phase19.8.sh +++ b/scripts/check-phase19.8.sh @@ -201,6 +201,8 @@ DOCKER_LAGOON_TESTS=( TestMigrateAppliedLines # quick 261022-4z8 (re-review fixes): WR-13. TestMigrateRereadFailureWarns + # quick 261022-4z8: WR-12 rework, lagoon.Status stays plugin-only. + TestStatusPluginRows ) DOCKER_CABANA_TESTS=(TestTokenCreateJSON TestTokenListJSON TestTokenCreateQuietKeepsToken)