Skip to content

Commit ed39fbc

Browse files
committed
test(e2e): run schema-drift migrations in an isolated database
Follow-up fixes from the review of apache#9015: - add e2ehelper.NewIsolatedMigrationDb so the schema-drift guards run the real migration scripts against a dedicated, empty database instead of the shared E2E_DB_URL one - use it in the jira and cross-plugin schema-drift tests - fix the teambition scope-config migration accordingly - document the findings in AGENTS.md
1 parent 90f78d8 commit ed39fbc

5 files changed

Lines changed: 208 additions & 42 deletions

File tree

AGENTS.md

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,14 +180,44 @@ Notes:
180180
- `backend/plugins/schema_e2e/` is **not** a plugin (no `main` package); it is
181181
excluded in `backend/scripts/build-plugins.sh`, otherwise `make build-plugin`
182182
fails with "-buildmode=plugin requires exactly one main package".
183-
- Both tests set a fallback `ENCRYPTION_SECRET` and call `dalgorm.Init(...)`;
183+
- Both tests run the migrations in a **dedicated, empty database**
184+
(`<e2e-db>_<suffix>`) created by `e2ehelper.NewIsolatedMigrationDb` and dropped
185+
again afterwards. The shared `E2E_DB_URL` database must not be used: the other
186+
e2e tests `AutoMigrate` tables without writing to `_devlake_migration_history`,
187+
so re-running the real scripts against it fails with e.g. `Table
188+
'cicd_pipeline_commits' already exists`.
189+
- The helper sets a fallback `ENCRYPTION_SECRET` and calls `dalgorm.Init(...)`;
184190
without it migrations fail with `invalid encKey` / `invalid serializer type
185191
encdec` (`runner.CreateBasicRes` does not register the serializer, only
186192
`CreateAppBasicRes` does).
193+
- An **auto-increment primary key cannot be added to an existing table** via
194+
`AutoMigrate` (MySQL: "there can be only one auto column and it must be
195+
defined as a key"); use explicit dialect-specific DDL (`BIGINT UNSIGNED NOT
196+
NULL AUTO_INCREMENT PRIMARY KEY` / `BIGSERIAL PRIMARY KEY`). Do **not** use
197+
`migrationhelper.TransformTable` for this: it copies rows with a zero PK, so
198+
all of them collapse into a single upserted row.
187199
- Migration scripts must not import `core/models/common` (enforced by
188200
`make migration-script-lint`) — use `core/models/migrationscripts/archived`.
189201

190202

203+
### CI im Fork (GitHub Actions)
204+
Die Workflows in `.github/workflows/` sind (bis auf Verbesserungen in `build.yml`)
205+
identisch mit `upstream/main`. Zwei Fork-spezifische Fallstricke:
206+
- Workflows mit `schedule:`-Trigger werden von GitHub in Forks automatisch
207+
deaktiviert (`disabled_fork`) — betraf `unit-test` (`test.yml`). Mit
208+
`gh workflow enable <id>` reaktivieren; `stale.yml` bleibt bewusst deaktiviert.
209+
- Die Upstream-Workflows triggern nur auf `push` nach `main` bzw.
210+
`pull_request` mit Base `main`/`release-*`. Fork-Feature-Branches (`pr/**`)
211+
und *stacked* PRs (Base ≠ `main`) bekommen dadurch keine CI.
212+
Dafür existiert `.github/workflows/fork-ci.yml` (`branches-ignore: [main]`,
213+
`workflow_dispatch`, `if: github.repository != 'apache/devlake'`), das
214+
golangci-lint, migration-script-lint, unit-test, e2e (MySQL), config-ui,
215+
ASF-Header- und Grafana-Check spiegelt.
216+
**Wichtig:** `fork-ci.yml` gehört nur in den Fork-`main`. Upstream-PR-Branches
217+
von `upstream/main` abzweigen — bei `pull_request`-Events nutzt GitHub die
218+
Workflows des Merge-Refs, die Datei greift also trotzdem, ohne im Upstream-PR
219+
als Diff aufzutauchen.
220+
191221
## Python Plugins
192222
Located in `backend/python/plugins/`. Use Poetry for dependencies. See [backend/python/README.md](backend/python/README.md).
193223
Python is pinned to **3.11**. Note (verified 2026-07-28): Pydantic was migrated to
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
/*
2+
Licensed to the Apache Software Foundation (ASF) under one or more
3+
contributor license agreements. See the NOTICE file distributed with
4+
this work for additional information regarding copyright ownership.
5+
The ASF licenses this file to You under the Apache License, Version 2.0
6+
(the "License"); you may not use this file except in compliance with
7+
the License. You may obtain a copy of the License at
8+
9+
http://www.apache.org/licenses/LICENSE-2.0
10+
11+
Unless required by applicable law or agreed to in writing, software
12+
distributed under the License is distributed on an "AS IS" BASIS,
13+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
See the License for the specific language governing permissions and
15+
limitations under the License.
16+
*/
17+
18+
package e2ehelper
19+
20+
import (
21+
"fmt"
22+
"net/url"
23+
"strings"
24+
"testing"
25+
26+
"github.com/apache/incubator-devlake/core/config"
27+
"github.com/apache/incubator-devlake/core/plugin"
28+
"github.com/apache/incubator-devlake/core/runner"
29+
"github.com/apache/incubator-devlake/impls/dalgorm"
30+
"github.com/apache/incubator-devlake/impls/logruslog"
31+
"gorm.io/gorm"
32+
)
33+
34+
// NewIsolatedMigrationDb creates a dedicated, empty database next to the one
35+
// referenced by E2E_DB_URL (`<e2e-db>_<suffix>`) and returns a connection to it.
36+
//
37+
// Tests that execute the REAL migration scripts must not share the regular e2e
38+
// database: the other plugin e2e tests seed/AutoMigrate tables (domain layer
39+
// included) without recording anything in `_devlake_migration_history`, so a
40+
// subsequent migration run fails with errors such as
41+
// "Table 'cicd_pipeline_commits' already exists" when it tries to create or
42+
// rename a table that is already there.
43+
//
44+
// The database is dropped again when the test finishes. If E2E_DB_URL is not
45+
// set the test is skipped.
46+
func NewIsolatedMigrationDb(t *testing.T, suffix string) *gorm.DB {
47+
cfg := config.GetConfig()
48+
e2eDbUrl := cfg.GetString("E2E_DB_URL")
49+
if e2eDbUrl == "" {
50+
t.Skip("E2E_DB_URL is not set; skipping migration schema check")
51+
}
52+
u, err := url.Parse(e2eDbUrl)
53+
if err != nil {
54+
t.Fatalf("unable to parse E2E_DB_URL: %v", err)
55+
}
56+
isolatedName := fmt.Sprintf("%s_%s", strings.TrimPrefix(u.Path, "/"), suffix)
57+
quotedName := quoteDbName(u.Scheme, isolatedName)
58+
59+
gormConf := &gorm.Config{SkipDefaultTransaction: true}
60+
adminDb, err := runner.MakeDbConnection(e2eDbUrl, gormConf)
61+
if err != nil {
62+
t.Fatalf("unable to connect to E2E_DB_URL: %v", err)
63+
}
64+
if err = adminDb.Exec("DROP DATABASE IF EXISTS " + quotedName).Error; err != nil {
65+
t.Fatalf("unable to drop leftover database %s: %v", isolatedName, err)
66+
}
67+
if err = adminDb.Exec("CREATE DATABASE " + quotedName).Error; err != nil {
68+
t.Fatalf("unable to create database %s: %v", isolatedName, err)
69+
}
70+
closeDb(adminDb)
71+
72+
isolatedUrl := *u
73+
isolatedUrl.Path = "/" + isolatedName
74+
db, err := runner.MakeDbConnection(isolatedUrl.String(), gormConf)
75+
if err != nil {
76+
t.Fatalf("unable to connect to %s: %v", isolatedName, err)
77+
}
78+
79+
// migration scripts and models read DB_URL from the global config, keep it
80+
// consistent with the connection we hand out and restore it afterwards.
81+
previousDbUrl := cfg.GetString("DB_URL")
82+
cfg.Set("DB_URL", isolatedUrl.String())
83+
84+
// Some migration scripts refuse to run without an encryption secret
85+
// (e.g. jira 20220716: "jira v0.11 invalid encKey"). CI does not
86+
// necessarily provide one, so fall back to a deterministic test value.
87+
// dalgorm.Init registers the `encdec` GORM serializer used by connection
88+
// models - without it migrations fail with "invalid serializer type encdec"
89+
// (runner.CreateBasicRes does not register it, only CreateAppBasicRes does).
90+
if cfg.GetString(plugin.EncodeKeyEnvStr) == "" {
91+
cfg.Set(plugin.EncodeKeyEnvStr, "devlake-e2e-test-encryption-secret")
92+
}
93+
dalgorm.Init(cfg.GetString(plugin.EncodeKeyEnvStr))
94+
95+
t.Cleanup(func() {
96+
cfg.Set("DB_URL", previousDbUrl)
97+
closeDb(db)
98+
cleanupDb, cleanupErr := runner.MakeDbConnection(e2eDbUrl, gormConf)
99+
if cleanupErr != nil {
100+
t.Logf("unable to connect for dropping %s: %v", isolatedName, cleanupErr)
101+
return
102+
}
103+
defer closeDb(cleanupDb)
104+
if dropErr := cleanupDb.Exec("DROP DATABASE IF EXISTS " + quotedName).Error; dropErr != nil {
105+
t.Logf("unable to drop database %s: %v", isolatedName, dropErr)
106+
}
107+
})
108+
109+
logruslog.Global.Info("running migrations against isolated database %s", isolatedName)
110+
return db
111+
}
112+
113+
func quoteDbName(scheme string, name string) string {
114+
// database names are derived from E2E_DB_URL + a constant suffix, but quote
115+
// them anyway to stay safe with reserved words.
116+
if strings.EqualFold(scheme, "mysql") {
117+
return "`" + strings.ReplaceAll(name, "`", "") + "`"
118+
}
119+
return `"` + strings.ReplaceAll(name, `"`, "") + `"`
120+
}
121+
122+
func closeDb(db *gorm.DB) {
123+
if sqlDb, err := db.DB(); err == nil {
124+
_ = sqlDb.Close()
125+
}
126+
}

backend/plugins/jira/e2e/migration_schema_test.go

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -21,13 +21,14 @@ import (
2121
"sync"
2222
"testing"
2323

24+
"github.com/apache/incubator-devlake/core/config"
2425
"github.com/apache/incubator-devlake/core/dal"
2526
"github.com/apache/incubator-devlake/core/migration"
2627
coreMigration "github.com/apache/incubator-devlake/core/models/migrationscripts"
27-
"github.com/apache/incubator-devlake/core/plugin"
2828
"github.com/apache/incubator-devlake/core/runner"
2929
"github.com/apache/incubator-devlake/helpers/e2ehelper"
3030
"github.com/apache/incubator-devlake/impls/dalgorm"
31+
"github.com/apache/incubator-devlake/impls/logruslog"
3132
"github.com/apache/incubator-devlake/plugins/jira/impl"
3233
"github.com/stretchr/testify/assert"
3334
"github.com/stretchr/testify/require"
@@ -52,23 +53,20 @@ import (
5253
// the migrated table. Any future migration that forgets to embed
5354
// common.NoPKModel (or otherwise omits a column) will fail this test.
5455
//
56+
// The migrations run against a dedicated, empty database (see
57+
// e2ehelper.NewIsolatedMigrationDb) because the shared e2e database is polluted
58+
// by the other e2e tests, which AutoMigrate tables without recording anything
59+
// in `_devlake_migration_history`.
60+
//
5561
// Requires E2E_DB_URL (runs under `make e2e-test` / `make e2e-test-go-plugins`).
5662
func TestMigrationSchema(t *testing.T) {
5763
var pluginInstance impl.Jira
58-
dataflowTester := e2ehelper.NewDataFlowTester(t, "jira", pluginInstance)
5964

60-
// Some jira migration scripts refuse to run without an encryption secret
61-
// (20220716: "jira v0.11 invalid encKey"); CI does not necessarily provide
62-
// one, so fall back to a deterministic test value. dalgorm.Init registers
63-
// the `encdec` GORM serializer used by connection models — without it the
64-
// migrations fail with "invalid serializer type encdec".
65-
if dataflowTester.Cfg.GetString(plugin.EncodeKeyEnvStr) == "" {
66-
dataflowTester.Cfg.Set(plugin.EncodeKeyEnvStr, "jira-e2e-test-encryption-secret")
67-
}
68-
dalgorm.Init(dataflowTester.Cfg.GetString(plugin.EncodeKeyEnvStr))
65+
db := e2ehelper.NewIsolatedMigrationDb(t, "jira_migration_schema")
66+
dalInstance := dalgorm.NewDalgorm(db)
6967

7068
// Apply the real migration scripts so the schema matches a production install.
71-
basicRes := runner.CreateBasicRes(dataflowTester.Cfg, dataflowTester.Log, dataflowTester.Db)
69+
basicRes := runner.CreateBasicRes(config.GetConfig(), logruslog.Global, db)
7270
migrator, err := migration.NewMigrator(basicRes)
7371
require.NoError(t, err)
7472
migrator.Register(coreMigration.All(), "Framework")
@@ -85,7 +83,7 @@ func TestMigrationSchema(t *testing.T) {
8583
require.NoErrorf(t, err, "unable to parse schema for %T", table)
8684

8785
// Columns that actually exist in the migrated table.
88-
actualColumns, err := dal.GetColumnNames(dataflowTester.Dal, table, keepAll)
86+
actualColumns, err := dal.GetColumnNames(dalInstance, table, keepAll)
8987
if err != nil || len(actualColumns) == 0 {
9088
// No migration creates this table (e.g. API response models
9189
// that are listed in GetTablesInfo but never persisted) —

backend/plugins/schema_e2e/migration_schema_test.go

Lines changed: 8 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import (
3838
coreMigration "github.com/apache/incubator-devlake/core/models/migrationscripts"
3939
"github.com/apache/incubator-devlake/core/plugin"
4040
"github.com/apache/incubator-devlake/core/runner"
41+
"github.com/apache/incubator-devlake/helpers/e2ehelper"
4142
"github.com/apache/incubator-devlake/impls/dalgorm"
4243
"github.com/apache/incubator-devlake/impls/logruslog"
4344
"github.com/stretchr/testify/assert"
@@ -173,28 +174,15 @@ func TestAllGoPluginsListed(t *testing.T) {
173174
// Tables that no migration creates (e.g. models materialized lazily at runtime)
174175
// are skipped, so the check specifically targets *drift* between an existing
175176
// table and its model — which is exactly the failure mode above.
177+
//
178+
// The migrations run against a dedicated, empty database (see
179+
// e2ehelper.NewIsolatedMigrationDb) because the shared e2e database is polluted
180+
// by the other e2e tests, which AutoMigrate tables without recording anything
181+
// in `_devlake_migration_history`.
176182
func TestMigrationSchemaMatchesModels(t *testing.T) {
177-
cfg := config.GetConfig()
178-
e2eDbURL := cfg.GetString("E2E_DB_URL")
179-
if e2eDbURL == "" {
180-
t.Skip("E2E_DB_URL is not set; skipping cross-plugin migration schema check")
181-
}
182-
cfg.Set("DB_URL", e2eDbURL)
183-
// Some migration scripts refuse to run without an encryption secret
184-
// (e.g. jira 20220716: "jira v0.11 invalid encKey"). CI does not
185-
// necessarily provide one, so fall back to a deterministic test value.
186-
if cfg.GetString(plugin.EncodeKeyEnvStr) == "" {
187-
cfg.Set(plugin.EncodeKeyEnvStr, "schema-e2e-test-encryption-secret")
188-
}
189-
190-
db, err := runner.NewGormDb(cfg, logruslog.Global)
191-
require.NoError(t, err)
192-
// register the `encdec` GORM serializer, otherwise migration scripts that
193-
// touch connection models fail with "invalid serializer type encdec"
194-
// (runner.CreateBasicRes does not do this, only CreateAppBasicRes does).
195-
dalgorm.Init(cfg.GetString(plugin.EncodeKeyEnvStr))
183+
db := e2ehelper.NewIsolatedMigrationDb(t, "schema_drift")
196184
dalInstance := dalgorm.NewDalgorm(db)
197-
basicRes := runner.CreateBasicRes(cfg, logruslog.Global, db)
185+
basicRes := runner.CreateBasicRes(config.GetConfig(), logruslog.Global, db)
198186

199187
// Apply the migrations exactly the way the server does on startup.
200188
migrator, migErr := migration.NewMigrator(basicRes)

backend/plugins/teambition/models/migrationscripts/20260727_add_missing_scope_config_columns.go

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,23 +18,20 @@ limitations under the License.
1818
package migrationscripts
1919

2020
import (
21+
"fmt"
22+
2123
"github.com/apache/incubator-devlake/core/context"
2224
"github.com/apache/incubator-devlake/core/errors"
2325
"github.com/apache/incubator-devlake/core/models/migrationscripts/archived"
2426
"github.com/apache/incubator-devlake/helpers/migrationhelper"
2527
)
2628

29+
const teambitionScopeConfigTable20260727 = "_tool_teambition_scope_configs"
30+
2731
// teambitionScopeConfig20260727 mirrors models.TeambitionScopeConfig. The
2832
// migration that created `_tool_teambition_scope_configs` did not include the
2933
// columns of the embedded common.Model (`id`, `created_at`, `updated_at`),
3034
// which the runtime model expects.
31-
//
32-
// Adding the AUTO_INCREMENT primary key `id` to the already existing (and
33-
// primary-key-less) table works: GORM issues a plain ADD COLUMN and MySQL
34-
// backfills consecutive ids for existing rows — verified against MySQL 8.4.
35-
// The `uniqueIndex` on `name` is likewise safe, because newly added columns are
36-
// nullable and both MySQL and PostgreSQL permit duplicate NULLs in a unique
37-
// index.
3835
type teambitionScopeConfig20260727 struct {
3936
archived.Model
4037
Entities []string `gorm:"type:json;serializer:json" json:"entities"`
@@ -48,12 +45,39 @@ type teambitionScopeConfig20260727 struct {
4845
}
4946

5047
func (teambitionScopeConfig20260727) TableName() string {
51-
return "_tool_teambition_scope_configs"
48+
return teambitionScopeConfigTable20260727
5249
}
5350

5451
type addMissingScopeConfigColumns struct{}
5552

53+
// Up adds the columns of the embedded common.Model that the runtime model
54+
// expects.
55+
//
56+
// `id` is an auto-increment primary key, which GORM's AutoMigrate cannot append
57+
// to an existing table: it emits a plain `ADD COLUMN ... AUTO_INCREMENT`, which
58+
// MySQL rejects with "Incorrect table definition; there can be only one auto
59+
// column and it must be defined as a key". The column is therefore added with
60+
// explicit DDL (the table has no primary key so far), letting the database
61+
// backfill ids for existing rows and keep the sequence/counter in sync. The
62+
// remaining columns (`created_at`, `updated_at`) and the indexes are then
63+
// created by AutoMigrate as usual.
5664
func (script *addMissingScopeConfigColumns) Up(basicRes context.BasicRes) errors.Error {
65+
db := basicRes.GetDal()
66+
if !db.HasColumn(teambitionScopeConfigTable20260727, "id") {
67+
ddl := fmt.Sprintf(
68+
"ALTER TABLE %s ADD COLUMN id BIGSERIAL PRIMARY KEY",
69+
teambitionScopeConfigTable20260727,
70+
)
71+
if db.Dialect() == "mysql" {
72+
ddl = fmt.Sprintf(
73+
"ALTER TABLE %s ADD COLUMN id BIGINT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY",
74+
teambitionScopeConfigTable20260727,
75+
)
76+
}
77+
if err := db.Exec(ddl); err != nil {
78+
return err
79+
}
80+
}
5781
return migrationhelper.AutoMigrateTables(basicRes, &teambitionScopeConfig20260727{})
5882
}
5983

0 commit comments

Comments
 (0)