From 020fe838eaf9c46ff99e1452b9ec128ae40f3dfb Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 22 Oct 2026 02:36:19 +0200 Subject: [PATCH] fix(lagoon): one migration set list for Migrate and PendingMigrations - Migrate iterates migrationSets, so the startup summary, --pretend, the production question, the applied lines and migrate:status --json read the same list; each set keeps its error wording through a label - migrate reports pending-before minus pending-after as its applied lines and result line, also when Migrate fails partway (then it prints what was applied and returns the error) - TestMigrateAppliedLines (Docker) pins both (WR-07) --- modules/lagoon/README.md | 2 +- modules/lagoon/commands.go | 33 +++++++++++++++--- modules/lagoon/migrations.go | 62 ++++++++-------------------------- modules/lagoon/pending_test.go | 56 ++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 52 deletions(-) diff --git a/modules/lagoon/README.md b/modules/lagoon/README.md index 4fbf5aaf..4079f1b1 100644 --- a/modules/lagoon/README.md +++ b/modules/lagoon/README.md @@ -256,7 +256,7 @@ storage: | Command | Flags | Description | |---------|-------|-------------| -| `migrate` | `--pretend`, `--force` | Runs the framework migrations (`system_files`, the backend admin tables, `deferred_bindings` and the job queue), then each plugin's migrations in dependency order; prints `: N applied` per set and `Applied N migrations`, or `Nothing to migrate`. `--pretend` prints each pending migration as `: ` and `N migrations would run`, and applies nothing. In production on an interactive terminal it asks first (`--force` skips the question); off a terminal it never asks. | +| `migrate` | `--pretend`, `--force` | Runs the framework migrations (`system_files`, the backend admin tables, `deferred_bindings` and the job queue), then each plugin's migrations in dependency order; prints `: N applied` per set and `Applied N migrations`, or `Nothing to migrate`. The applied lines count what was actually applied: when a migration fails, `migrate` prints the lines for the sets applied before it, then the error, and no `Applied` line. `--pretend` prints each pending migration as `: ` and `N migrations would run`, and applies nothing. In production on an interactive terminal it asks first (`--force` skips the question); off a terminal it never asks. | | `migrate:rollback` | `--plugin `, `--pretend`, `--force` | Rolls back the last applied migration of the given plugin; without the flag, of the last activated plugin that has migrations. Prints `Rolled back of `; `--pretend` names the migration and changes nothing. Production confirmation as for `migrate`. | | `migrate:status` | `--json` | Prints a table of plugin, history table and applied migration IDs. `--json` prints a JSON array of every declared migration of those plugins: `plugin`, `table`, `id`, `applied`. | | `deferred:purge` | `--days `, `--pretend`, `--force` | Deletes deferred bindings older than `n` days (default `database.deferred_bindings.purge_days`, else 5), with the unattached uploads they hold (blobs after the commit) and the records created under deferral, and prints the counts. Records that were only linked are kept. Model types come from every plugin's `pact.HasModels`; the uploads bucket from the published one or `storage.uploads.bucket_url`. `--pretend` counts the bindings past the cut-off and deletes nothing; production confirmation as for `migrate`. | diff --git a/modules/lagoon/commands.go b/modules/lagoon/commands.go index 13073ae0..936125fe 100644 --- a/modules/lagoon/commands.go +++ b/modules/lagoon/commands.go @@ -103,6 +103,22 @@ func lastApplied(gdb *gorm.DB, plugins []party.Plugin, pluginID string) (string, return "", nil } +// appliedSince returns the migrations of before that after no longer +// lists as pending, in the order of before. +func appliedSince(before, after []MigrationRef) []MigrationRef { + still := make(map[MigrationRef]bool, len(after)) + for _, ref := range after { + still[ref] = true + } + var applied []MigrationRef + for _, ref := range before { + if !still[ref] { + applied = append(applied, ref) + } + } + return applied +} + // statusJSON is one migration of migrate:status --json. type statusJSON struct { Plugin string `json:"plugin"` @@ -154,12 +170,18 @@ asks.`, if err := confirmProduction(app, in, out, fmt.Sprintf("Apply %d migrations to the production database?", len(pending)), "Migration cancelled."); err != nil { return err } - if err := Migrate(gdb, plugins); err != nil { - return err + migrateErr := Migrate(gdb, plugins) + // Report what Migrate actually applied: the pending + // migrations before it ran that are no longer pending, + // also when it failed partway. + after, err := PendingMigrations(gdb, plugins) + if err != nil { + return errors.Join(migrateErr, err) } + applied := appliedSince(pending, after) counts := map[string]int{} var order []string - for _, ref := range pending { + for _, ref := range applied { if counts[ref.Plugin] == 0 { order = append(order, ref.Plugin) } @@ -168,7 +190,10 @@ asks.`, for _, plugin := range order { out.Info(fmt.Sprintf("%s: %d applied", plugin, counts[plugin])) } - out.Success(fmt.Sprintf("Applied %d migrations", len(pending))) + if migrateErr != nil { + return migrateErr + } + out.Success(fmt.Sprintf("Applied %d migrations", len(applied))) return nil }) }, diff --git a/modules/lagoon/migrations.go b/modules/lagoon/migrations.go index 2c7a0cf6..89b0460d 100644 --- a/modules/lagoon/migrations.go +++ b/modules/lagoon/migrations.go @@ -60,58 +60,23 @@ func migrator(gdb *gorm.DB, pluginID string, migrations []*gormigrate.Migration) // Migrate runs the framework-owned system_files, backend-admin, // deferred-binding (DeferredBindingMigrations, history DeferredHistoryID) and // job-queue (QueueMigrations) sets first, then each plugin's HasMigrations set -// in party.Activate order. +// in party.Activate order. It applies the sets migrationSets lists, the list +// PendingMigrations reads, and stops at the first set that fails. func Migrate(gdb *gorm.DB, plugins []party.Plugin) error { if gdb == nil { return fmt.Errorf("lagoon: gorm db is nil") } - m, err := migrator(gdb, "summercms.attach", attach.Migrations) + sets, err := migrationSets(gdb, plugins) if err != nil { return err } - if err := m.Migrate(); err != nil { - return fmt.Errorf("lagoon: migrate system_files: %w", err) - } - admin, err := migrator(gdb, "summercms.cabana", BackendAdminMigrations) - if err != nil { - return err - } - if err := admin.Migrate(); err != nil { - return fmt.Errorf("lagoon: migrate backend admin: %w", err) - } - deferred, err := migrator(gdb, DeferredHistoryID, DeferredBindingMigrations) - if err != nil { - return err - } - if err := deferred.Migrate(); err != nil { - return fmt.Errorf("lagoon: migrate deferred bindings: %w", err) - } - sqlDB, err := gdb.DB() - if err != nil { - return fmt.Errorf("lagoon: migrate queue: %w", err) - } - queue, err := migrator(gdb, QueueHistoryID, QueueMigrations(sqlDB)) - if err != nil { - return err - } - if err := queue.Migrate(); err != nil { - return fmt.Errorf("lagoon: migrate queue: %w", err) - } - for _, p := range plugins { - hm, ok := p.(pact.HasMigrations) - if !ok { - continue - } - migrations := hm.Migrations() - if len(migrations) == 0 { - continue - } - m, err := migrator(gdb, p.ID(), migrations) + for _, s := range sets { + m, err := migrator(gdb, s.id, s.migrations) if err != nil { return err } if err := m.Migrate(); err != nil { - return fmt.Errorf("lagoon: migrate %s: %w", p.ID(), err) + return fmt.Errorf("lagoon: migrate %s: %w", s.label, err) } } return nil @@ -211,7 +176,10 @@ type MigrationRef struct { // migrationSet is one history table's declared migrations. type migrationSet struct { - id string + id string + // label names the set in Migrate's errors: "system_files", "backend + // admin", "deferred bindings", "queue" or the plugin ID. + label string migrations []*gormigrate.Migration } @@ -224,10 +192,10 @@ func migrationSets(gdb *gorm.DB, plugins []party.Plugin) ([]migrationSet, error) return nil, fmt.Errorf("lagoon: migration sets: %w", err) } sets := []migrationSet{ - {"summercms.attach", attach.Migrations}, - {"summercms.cabana", BackendAdminMigrations}, - {DeferredHistoryID, DeferredBindingMigrations}, - {QueueHistoryID, QueueMigrations(sqlDB)}, + {"summercms.attach", "system_files", attach.Migrations}, + {"summercms.cabana", "backend admin", BackendAdminMigrations}, + {DeferredHistoryID, "deferred bindings", DeferredBindingMigrations}, + {QueueHistoryID, "queue", QueueMigrations(sqlDB)}, } for _, p := range plugins { hm, ok := p.(pact.HasMigrations) @@ -235,7 +203,7 @@ func migrationSets(gdb *gorm.DB, plugins []party.Plugin) ([]migrationSet, error) continue } if ms := hm.Migrations(); len(ms) > 0 { - sets = append(sets, migrationSet{p.ID(), ms}) + sets = append(sets, migrationSet{p.ID(), p.ID(), ms}) } } return sets, nil diff --git a/modules/lagoon/pending_test.go b/modules/lagoon/pending_test.go index 065e698c..938e86a9 100644 --- a/modules/lagoon/pending_test.go +++ b/modules/lagoon/pending_test.go @@ -294,3 +294,59 @@ func TestMigrateStatusJSON(t *testing.T) { } } } + +// TestMigrateAppliedLines (WR-07): migrate's "N applied" lines count what +// Migrate applied, also when a later set fails: the sets before the failure +// are reported, the error is returned and no success line is printed. The +// next run reports only what it applied. +func TestMigrateAppliedLines(t *testing.T) { + db, dsn := dedicatedDB(t, "lagoon_applied_lines") + gdb, err := Use(t.Context(), db) + if err != nil { + t.Fatal(err) + } + create := func(name, sql string) *gormigrate.Migration { + return &gormigrate.Migration{ + ID: "202610220001_create_" + name, + Migrate: func(tx *gorm.DB) error { return tx.Exec(sql).Error }, + Rollback: func(tx *gorm.DB) error { return tx.Exec(`DROP TABLE IF EXISTS ` + name).Error }, + } + } + alpha := migPlugin{id: "demo.okay", migrations: []*gormigrate.Migration{create("applied_okay", `CREATE TABLE applied_okay (id BIGSERIAL PRIMARY KEY)`)}} + broken := migPlugin{id: "demo.broken", migrations: []*gormigrate.Migration{create("applied_broken", `CREATE TABLE applied_broken (id BOGUS_TYPE)`)}} + framework, err := PendingMigrations(gdb, nil) + if err != nil { + t.Fatal(err) + } + + out, errOut, code, _ := runLagoon(t, "local", dsn, []party.Plugin{alpha, broken}, "migrate") + if code != bonfire.ExitFailure || !strings.Contains(errOut, "demo.broken") { + t.Fatalf("migrate with a failing set: exit %d\n%s%s", code, out, errOut) + } + if !strings.Contains(out, "demo.okay: 1 applied\n") { + t.Errorf("the set applied before the failure is not reported:\n%s", out) + } + seen := map[string]bool{} + for _, ref := range framework { + if !seen[ref.Plugin] && !strings.Contains(out, ref.Plugin+": ") { + t.Errorf("framework set %s not reported:\n%s", ref.Plugin, out) + } + seen[ref.Plugin] = true + } + if strings.Contains(out, "demo.broken: ") || strings.Contains(out, "Applied ") { + t.Errorf("a failed set or a success line reported:\n%s", out) + } + + fixed := migPlugin{id: "demo.broken", migrations: []*gormigrate.Migration{create("applied_broken", `CREATE TABLE applied_broken (id BIGSERIAL PRIMARY KEY)`)}} + out, errOut, code, _ = runLagoon(t, "local", dsn, []party.Plugin{alpha, fixed}, "migrate") + if code != bonfire.ExitOK { + t.Fatalf("second migrate: exit %d\n%s%s", code, out, errOut) + } + if !strings.Contains(out, "demo.broken: 1 applied\n") || !strings.Contains(out, "Applied 1 migrations") || strings.Contains(out, "demo.okay: ") { + t.Errorf("second migrate result lines:\n%s", out) + } + out, _, _, _ = runLagoon(t, "local", dsn, []party.Plugin{alpha, fixed}, "migrate") + if !strings.Contains(out, "Nothing to migrate") { + t.Errorf("third migrate:\n%s", out) + } +}