feat: automatic db migrations #816
Conversation
WalkthroughUpdates the Docker entrypoint to run migrations via npm and continue starting the server regardless of migration outcome. Converts migrate.js to CommonJS. Adds a new retryCount migration and makes Run.retryCount optional. Hardens existing migrations to check for column/index existence before adding/removing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Container/Orchestrator
participant E as docker-entrypoint.sh
participant N as npm (migrate script)
participant M as migrate.js (CommonJS)
participant S as Sequelize CLI
participant DB as Database
participant SRV as App Server
U->>E: Start container with CMD
E->>E: Echo "Running database migrations..."
E->>N: npm run migrate
N->>M: invoke runMigrations()
M->>S: execSync sequelize db:migrate
S->>DB: Apply migrations
DB-->>S: Result (success/failure)
S-->>M: exit code
alt success
M-->>N: true
E->>E: Echo "✅ Migrations completed successfully!"
else failure
M-->>N: false
E->>E: Echo "⚠️ Migration failed, but continuing..."
end
E->>SRV: exec "$@" (start server)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/docker-entrypoint.sh(1 hunks)server/src/db/migrate.js(1 hunks)server/src/db/migrations/20250202000000-add-retry-count.js(1 hunks)server/src/db/migrations/20250327111003-add-airtable-columns.js(1 hunks)server/src/db/migrations/20250527105655-add-webhooks.js(1 hunks)server/src/models/Run.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
server/src/db/migrations/20250527105655-add-webhooks.js (1)
server/src/db/migrations/20250202000000-add-retry-count.js (1)
tableInfo(5-5)
server/src/db/migrations/20250202000000-add-retry-count.js (1)
server/src/db/migrations/20250527105655-add-webhooks.js (2)
tableInfo(5-5)tableInfo(40-40)
server/src/db/migrate.js (1)
server/src/db/models/index.js (1)
__dirname(10-10)
server/src/db/migrations/20250327111003-add-airtable-columns.js (2)
server/src/db/migrations/20250202000000-add-retry-count.js (1)
tableInfo(5-5)server/src/db/migrations/20250527105655-add-webhooks.js (2)
tableInfo(5-5)tableInfo(40-40)
🔇 Additional comments (7)
server/src/models/Run.ts (1)
50-50: LGTM! Consistent with schema and migration.The optional retryCount property aligns with the nullable column definition (lines 125–129) and the migration in
server/src/db/migrations/20250202000000-add-retry-count.js, which adds a nullable integer column with a default of 0.server/src/db/migrations/20250327111003-add-airtable-columns.js (1)
70-90: LGTM! Idempotent down migration.The down migration now mirrors the up migration's idempotency by checking column existence before removal. This prevents errors if the migration is rolled back multiple times or if the columns were never added.
server/src/db/migrations/20250527105655-add-webhooks.js (2)
5-27: LGTM! Idempotent up migration with proper index guard.The up migration correctly uses
describeTableto check column existence andshowIndexto verify the index before adding it. This prevents duplicate columns or indexes and ensures idempotency.
30-43: LGTM! Idempotent down migration.The down migration mirrors the up migration's safety checks by verifying index and column existence before removal, preventing errors on repeated rollbacks.
server/src/db/migrate.js (2)
3-4: LGTM! Correct CommonJS conversion.The module correctly uses
requireforchild_processandpath, aligning with the CommonJS context required by the npm script invocation inserver/docker-entrypoint.sh.
9-12: cwd resolution and configuration path are correct
The cwd resolves to the repo root, where.sequelizercandserver/src/db/config/database.jsexist, sosequelize-cliwill locate the config without issues.server/src/db/migrations/20250202000000-add-retry-count.js (1)
4-14: LGTM! Idempotent up migration.The up migration correctly checks for the existence of the
retryCountcolumn before adding it, ensuring idempotency and preventing duplicate column errors.
| # Run database migrations using npm script | ||
| echo "Running database migrations..." | ||
| npm run migrate | ||
|
|
||
| # Run the server normally | ||
| if [ $? -eq 0 ]; then | ||
| echo "✅ Migrations completed successfully!" | ||
| else | ||
| echo "⚠️ Migration failed, but continuing to start server..." | ||
| fi |
There was a problem hiding this comment.
Consider failing fast on migration errors.
The script continues to start the server even if migrations fail (line 36). This could lead to runtime errors if the schema is out of sync with the application code (e.g., missing retryCount column).
Consider changing the behavior to exit on migration failure to prevent cascading errors:
if [ $? -eq 0 ]; then
echo "✅ Migrations completed successfully!"
else
- echo "⚠️ Migration failed, but continuing to start server..."
+ echo "❌ Migration failed. Exiting to prevent runtime errors."
+ exit 1
fiAlternatively, if continuing on failure is intentional (e.g., for local development), document the rationale in a comment.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Run database migrations using npm script | |
| echo "Running database migrations..." | |
| npm run migrate | |
| # Run the server normally | |
| if [ $? -eq 0 ]; then | |
| echo "✅ Migrations completed successfully!" | |
| else | |
| echo "⚠️ Migration failed, but continuing to start server..." | |
| fi | |
| # Run database migrations using npm script | |
| echo "Running database migrations..." | |
| npm run migrate | |
| if [ $? -eq 0 ]; then | |
| echo "✅ Migrations completed successfully!" | |
| else | |
| echo "❌ Migration failed. Exiting to prevent runtime errors." | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In server/docker-entrypoint.sh around lines 29 to 37, the script currently
continues to start the server when migrations fail which can cause runtime
errors; update the else branch to fail fast by exiting with a non-zero code
(e.g., exit 1) so the container stops on migration failure, or if the intent is
to allow startup despite migration errors (local/dev), add a clear comment
explaining that choice and why it's safe and desired.
| down: async (queryInterface, Sequelize) => { | ||
| await queryInterface.removeColumn('run', 'retryCount'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Make the down migration idempotent.
The down migration unconditionally removes the retryCount column without checking if it exists. This is inconsistent with the guarded patterns in other migrations (e.g., 20250327111003-add-airtable-columns.js and 20250527105655-add-webhooks.js) and will fail if the column was never added or was already removed.
Apply this diff to make the down migration idempotent:
down: async (queryInterface, Sequelize) => {
- await queryInterface.removeColumn('run', 'retryCount');
+ const tableInfo = await queryInterface.describeTable('run');
+ if (tableInfo.retryCount) {
+ await queryInterface.removeColumn('run', 'retryCount');
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| down: async (queryInterface, Sequelize) => { | |
| await queryInterface.removeColumn('run', 'retryCount'); | |
| } | |
| down: async (queryInterface, Sequelize) => { | |
| const tableInfo = await queryInterface.describeTable('run'); | |
| if (tableInfo.retryCount) { | |
| await queryInterface.removeColumn('run', 'retryCount'); | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/src/db/migrations/20250202000000-add-retry-count.js around lines 17 to
19, the down migration unconditionally calls queryInterface.removeColumn('run',
'retryCount') which can fail if the column does not exist; update the down
migration to first inspect the table (e.g., using
queryInterface.describeTable('run') or a similar metadata call) and only call
removeColumn if the returned description shows retryCount exists, so the down
migration becomes idempotent and mirrors the guarded pattern used in other
migrations.
1 participant
What this PR does?
Summary by CodeRabbit