vbhanuchander-lang commented on PR #9015: URL: https://github.com/apache/devlake/pull/9015#issuecomment-5153831956
Nice piece of work — particularly leaving `20260722_add_sprint_report_table.go` untouched and repairing forward with an additive migration. That's the right call for anything already recorded in `_devlake_migration_history`, and it's the part people most often get wrong. One gap worth considering in the new guard, since it's the most interesting part of this PR. `migration_schema_test.go` runs the real migration chain against a fresh database and compares the resulting columns to the model. That catches drift in the *end state*, which is the bug you started from. But it runs against empty tables, so it cannot exercise the populated-table upgrade path — and that path is exactly where the teambition workaround lives: > `id` is an auto-increment primary key, which GORM's AutoMigrate cannot append to an existing table ... The column is therefore added with explicit DDL On an empty `_tool_teambition_scope_configs`, plain `AutoMigrate` and the explicit DDL produce an identical schema, so the guard stays green either way. If someone later simplifies that DDL back to `AutoMigrate` because it looks redundant, the guard still passes and only real upgrades break — the same failure shape as the bug this PR fixes, one layer up. Would it be worth inserting a row into each table before the tail migrations run, so the guard also asserts the migration *applies* to a non-empty table? Even a single row would have made the `id` case fail loudly rather than silently. Not a blocker — the fix itself looks correct on both engines to me. `Dialect()` returns `Dialector.Name()`, so the `"mysql"` comparison matches, and `BIGSERIAL PRIMARY KEY` is the right Postgres counterpart. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
