Run migrations before other stuff #1525
Conversation
Signed-off-by: Sebastian Kawelke <sebastian.kawelke@l3montree.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors the database initialization sequence by moving schema migrations to run immediately within NewGormDB() instead of after application creation. This ensures database tables exist before any controllers or services attempt to use them, addressing potential race conditions during startup.
Key changes:
- Schema migrations now execute automatically when creating the GORM database connection
- Removed redundant migration call from main application startup flow
- Added documentation clarifying the separation between schema and hash migrations
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| database/database.go | Added automatic migration execution in NewGormDB with DISABLE_AUTOMIGRATE environment variable check |
| cmd/devguard/main.go | Removed schema migration call from main startup, retained only hash migrations with clarifying comments |
| .env.example | Added DB connection pool configuration examples (appears unrelated to migration timing changes) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Run migrations immediately after creating the database connection | ||
| // This ensures tables exist before any controllers try to use them | ||
| disableAutoMigrate := os.Getenv("DISABLE_AUTOMIGRATE") | ||
| if disableAutoMigrate != "true" { | ||
| slog.Info("running database migrations...") | ||
| if err := RunMigrationsWithDB(gormDB); err != nil { | ||
| slog.Error("failed to run database migrations", "error", err) | ||
| panic(fmt.Errorf("failed to run database migrations: %w", err)) | ||
| } | ||
| } else { | ||
| slog.Info("automatic migrations disabled via DISABLE_AUTOMIGRATE=true") | ||
| } |
There was a problem hiding this comment.
Running migrations in NewGormDB will cause duplicate migration runs in test scenarios. The test initialization code in tests/db_init.go calls NewGormDB and then explicitly calls RunMigrationsWithDB again (lines 18-26). This means migrations will be executed twice during test setup, which is inefficient and could potentially cause issues with migration state tracking.
Consider either:
- Adding a flag parameter to NewGormDB to control whether migrations should run automatically, or
- Creating a separate constructor function for test scenarios that skips auto-migration, or
- Setting DISABLE_AUTOMIGRATE=true in the test environment and letting tests explicitly control migration timing
| CSAF_PASSPHRASE=example-passphrase | ||
|
|
||
|
|
||
| ## DB Connection Pool settings | ||
| # | ||
| # DB_MAX_OPEN_CONNS=25 | ||
| # DB_MIN_CONNS=5 | ||
| # DB_CONN_MAX_LIFETIME= # default is 4 hours | ||
| # DB_CONN_MAX_IDLE_TIME= # default is 15 min |
There was a problem hiding this comment.
The .env.example update appears unrelated to the stated purpose of this PR. Adding DB connection pool configuration settings doesn't directly support running migrations before other initialization steps. Consider moving these configuration additions to a separate PR focused on database connection pool management.
| CSAF_PASSPHRASE=example-passphrase | |
| ## DB Connection Pool settings | |
| # | |
| # DB_MAX_OPEN_CONNS=25 | |
| # DB_MIN_CONNS=5 | |
| # DB_CONN_MAX_LIFETIME= # default is 4 hours | |
| # DB_CONN_MAX_IDLE_TIME= # default is 15 min | |
| CSAF_PASSPHRASE=example-passphrase |
| // Run hash migrations if needed (when algorithm version changes) | ||
| if err := vulndb.RunHashMigrationsIfNeeded(db); err != nil { | ||
| slog.Error("failed to run hash migrations", "error", err) | ||
| panic(errors.New("Failed to run hash migrations")) |
There was a problem hiding this comment.
Error message uses inconsistent capitalization. The panic message "Failed to run hash migrations" starts with an uppercase letter, whereas Go convention for error messages is to use lowercase unless the first word is a proper noun or acronym. This is inconsistent with the error message pattern used in the database.go file where wrapped errors are lowercase.
| panic(errors.New("Failed to run hash migrations")) | |
| panic(errors.New("failed to run hash migrations")) |
No description provided.