[Wrangler/D1] Add CLI flag to skip migration confirmation#10598
[Wrangler/D1] Add CLI flag to skip migration confirmation#10598Ehbraheem wants to merge 6 commits intocloudflare:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d98f1dd The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Ehbraheem
force-pushed
the
wrangler/d1-migration-confirmation-skip
branch
from
3c9bb7f to
f42c299
Compare
|
@penalosa, please help take a look at this PR when you're free. It's not a big change that'll take your time 🙏 |
| "auto-apply": { | ||
| type: "boolean", | ||
| description: "Automatically apply all pending migrations without prompt", | ||
| default: false, | ||
| }, |
There was a problem hiding this comment.
I don't think we want to enable this only at the D1 level. There were few discussions internally for a "non-interactive" mode for wrangler, and therefore it's better to have a more consistent plan across products instead of doing something just for D1 migrations.
There was a problem hiding this comment.
Yeah @lambrospetrou, you’re right, it does make sense to aim for a consistent approach to non-interactive mode across Wrangler.
That said, I think starting with D1 makes sense since there’s already visible demand for this feature and the change here is relatively small.
It could also serve as a lightweight way to gauge how users respond to non-interactive workflows before rolling out a broader plan.
So rather than blocking this on the bigger initiative, this could be a useful first step to validate its impact and guide that wider effort.
There was a problem hiding this comment.
@lambrospetrou from a Wrangler perspective, this seems pretty reasonable. We need a wider solution here, but I don't think we should block on that. I have a mild preference towards a potentially dangerous flag like this being called --force, but otherwise this seems conceptually good to me. Once someone from D1 has reviewed the code I can review and get this landed.
lambrospetrou
left a comment
lambrospetrou
left a comment
There was a problem hiding this comment.
Before merging please do the suggested changes, to use --force-non-interactive.
| @@ -49,9 +49,17 @@ export const d1MigrationsApplyCommand = createCommand({ | |||
| "Specify directory to use for local persistence (you must use --local with this flag)", | |||
| requiresArg: true, | |||
| }, | |||
| "auto-apply": { | |||
There was a problem hiding this comment.
| "auto-apply": { | |
| "force-non-interactive": { |
| }, | ||
| positionalArgs: ["database"], | ||
| async handler({ database, local, remote, persistTo, preview }, { config }) { | ||
| async handler( | ||
| { database, local, remote, persistTo, preview, autoApply }, |
There was a problem hiding this comment.
| { database, local, remote, persistTo, preview, autoApply }, | |
| { database, local, remote, persistTo, preview, forceNonInteractive }, |
| @@ -123,12 +131,19 @@ export const d1MigrationsApplyCommand = createCommand({ | |||
| logger.log("Migrations to be applied:"); | |||
| logger.table(unappliedMigrations.map((m) => ({ name: m.name }))); | |||
|
|
|||
| const ok = await confirm( | |||
| `About to apply ${unappliedMigrations.length} migration(s) | |||
| if (!autoApply) { | |||
There was a problem hiding this comment.
| if (!autoApply) { | |
| if (!forceNonInteractive) { |
Ehbraheem
force-pushed
the
wrangler/d1-migration-confirmation-skip
branch
from
5643923 to
63646c3
Compare
@lambrospetrou Thanks for the review. I've addressed all the suggestions in a single commit to avoid vain changes. You can take a look again. |
Ehbraheem
force-pushed
the
wrangler/d1-migration-confirmation-skip
branch
from
a9c2bbc to
1fca9e9
Compare
|
follow up |
|
Hi @Ehbraheem, thank you very much for the PR and sorry for having left this PR go stale I think the changes look good, but ideally the flag should be called This would also be consistent with similar flags in the repo: workers-sdk/packages/wrangler/src/vectorize/delete.ts Lines 23 to 28 in b7841db Would you be able to make this change? (and solve the merge conflict?) Please let me know, otherwise I'm happy to take over this PR 🙏 |
Ok, makes sense. I'll look into that |
Ehbraheem
force-pushed
the
wrangler/d1-migration-confirmation-skip
branch
from
1fca9e9 to
c47b8f0
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| expect(commands.some((c) => c.includes("CREATE TABLE users"))).toBe(true); | ||
| expect(commands.some((c) => c.includes("INSERT INTO users"))).toBe(true); | ||
| expect(commands.some((c) => c.includes("INSERT INTO migrations"))).toBe( | ||
| true | ||
| ); |
There was a problem hiding this comment.
🔴 Test assertions check for SQL content that is never written to the migration file
The test "applies migrations from a migration directory containing .sql files" creates a migration via runWrangler("d1 migrations create db test"), which only writes a comment line (-- Migration number: 0001 \t <timestamp>\n) to the file (packages/wrangler/src/d1/migrations/create.ts:70-72). However, the assertions at lines 307-311 expect CREATE TABLE users, INSERT INTO users, and INSERT INTO migrations to appear in the captured API request bodies. None of these SQL statements exist anywhere in the migration file or in the appended tracking insert (which uses INSERT INTO d1_migrations per packages/wrangler/src/d1/constants.ts:2). All three assertions will always fail, making this test dead-on-arrival.
Was this helpful? React with 👍 or 👎 to provide feedback.
Ehbraheem
left a comment
Ehbraheem
left a comment
There was a problem hiding this comment.
@dario-piotrowicz You can check now.
workers-devprod
dismissed
lambrospetrou’s stale review
Changes in owned files since last approval
|
/bonk can you update the formatting on this PR so that it passes |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the PR! The core implementation is sound and the --force flag is a welcome addition. I have a few issues that should be addressed before merging.
Summary of issues:
- Broken test — The "applies migrations from a migration directory containing .sql files" test asserts SQL content (
CREATE TABLE users,INSERT INTO users) that never exists in the generated migration file.d1 migrations createonly writes a comment line, so these assertions will always fail. - Changeset version level — Adding a new CLI flag is a new feature, which should be
minor, notpatch. - Minor style issues — Extraneous blank lines, and new tests don't follow the existing pattern of destructuring
expectfrom the test context. - Missing
--forcealias — Other--forceflags in the codebase consistently includealias: "y"for convenience.
| expect(commands.some((c) => c.includes("CREATE TABLE users"))).toBe(true); | ||
| expect(commands.some((c) => c.includes("INSERT INTO users"))).toBe(true); | ||
| expect(commands.some((c) => c.includes("INSERT INTO migrations"))).toBe( | ||
| true | ||
| ); |
There was a problem hiding this comment.
Bug: These assertions will always fail. d1 migrations create only writes a comment line (-- Migration number: 0001 ...), so CREATE TABLE users and INSERT INTO users will never appear in the request body.
You need to either:
- Write actual SQL content into the migration file after
d1 migrations create(before runningapply), or - Remove these incorrect assertions and only assert on what the migration actually contains (the comment line and the
INSERT INTO migrationsstatement injected byapply.ts).
Option 1 is probably what was intended — add something like this after line 304:
| expect(commands.some((c) => c.includes("CREATE TABLE users"))).toBe(true); | |
| expect(commands.some((c) => c.includes("INSERT INTO users"))).toBe(true); | |
| expect(commands.some((c) => c.includes("INSERT INTO migrations"))).toBe( | |
| true | |
| ); | |
| await runWrangler("d1 migrations create db test"); | |
| // Write actual SQL content into the migration file | |
| const fs = await import("node:fs"); | |
| const migrationsDir = "migrations"; | |
| const files = fs.readdirSync(migrationsDir); | |
| const migrationFile = files.find((f: string) => f.endsWith(".sql")); | |
| if (migrationFile) { | |
| fs.writeFileSync( | |
| `${migrationsDir}/${migrationFile}`, | |
| "CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT);\nINSERT INTO users (name) VALUES ('test');\n" | |
| ); | |
| } |
| @@ -228,6 +229,162 @@ Your database may not be available to serve requests during the migration, conti | |||
| await runWrangler("d1 migrations apply db --remote"); | |||
| expect(std.out).toBe(""); | |||
| }); | |||
|
|
|||
| it("applies migrations from a migration directory containing .sql files", async () => { | |||
There was a problem hiding this comment.
Style: Existing tests in this file destructure expect from the test context (e.g. async ({ expect }) => {). The two new tests use global expect instead. Per the wrangler testing conventions, tests should not import expect from vitest — they should use the test context parameter.
| it("applies migrations from a migration directory containing .sql files", async () => { | |
| it("applies migrations from a migration directory containing .sql files", async ({ expect }) => { |
| ); | ||
| }); | ||
|
|
||
| it("should not prompt when --force is passed", async () => { |
There was a problem hiding this comment.
Same as above — destructure expect from the test context:
| it("should not prompt when --force is passed", async () => { | |
| it("should not prompt when --force is passed", async ({ expect }) => { |
|
|
||
| await runWrangler("d1 migrations create db test"); | ||
|
|
||
| await runWrangler("d1 migrations apply db --remote"); |
There was a problem hiding this comment.
This test doesn't use --force and runs in non-interactive mode (setIsTTY(false)). When not interactive, confirm() automatically returns the fallback value (true) without prompting, so the migration will apply — but this means the test isn't really testing anything specific to this PR. Is this test meant to be a standalone test for .sql file migrations, or was it intended to use --force?
Also, since this test doesn't call mockConfirm(), it relies on the confirm() CI fallback behavior. If that's intentional, consider adding a comment explaining that.
.changeset/auto-apply-migrations.md
Outdated
| @@ -0,0 +1,15 @@ | |||
| --- | |||
| "wrangler": patch | |||
There was a problem hiding this comment.
Adding a new CLI flag (--force) is a new feature, which should be versioned as minor per the changeset guidelines ("minor: New features, new CLI commands, new configuration options").
| "wrangler": patch | |
| "wrangler": minor |
| getUnappliedMigrations, | ||
| initMigrationsTable, | ||
| } from "./helpers"; | ||
| import { getMigrationsPath, getUnappliedMigrations, initMigrationsTable } from "./helpers"; | ||
| import type { ParseError } from "@cloudflare/workers-utils"; | ||
|
|
There was a problem hiding this comment.
Nit: Extraneous blank line.
| @@ -12,6 +12,7 @@ import { msw } from "../helpers/msw"; | |||
| import { runInTempDir } from "../helpers/run-in-tmp"; | |||
| import { runWrangler } from "../helpers/run-wrangler"; | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Nit: Extraneous blank line introduced.
| force: { | ||
| type: "boolean", | ||
| description: "Automatically apply all pending migrations without prompt", | ||
| default: false, | ||
| }, |
There was a problem hiding this comment.
For consistency with other --force flags in the codebase (e.g. vectorize/delete.ts, kv/index.ts), consider adding alias: "y" so users can use -y as a shorthand.
| force: { | |
| type: "boolean", | |
| description: "Automatically apply all pending migrations without prompt", | |
| default: false, | |
| }, | |
| force: { | |
| type: "boolean", | |
| alias: "y", | |
| description: "Automatically apply all pending migrations without prompt", | |
| default: false, | |
| }, |
|
Review submitted. Here's a summary of the feedback: Key issues found:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
Ehbraheem
force-pushed
the
wrangler/d1-migration-confirmation-skip
branch
from
73c3340 to
d8a267a
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
There was a problem hiding this comment.
Pull request overview
Adds a --force flag to wrangler d1 migrations apply to bypass the interactive confirmation and apply pending migrations non-interactively (useful for automation/CI), with accompanying tests and a changeset entry.
Changes:
- Add
--forceboolean flag tod1 migrations applyto skip the confirmation prompt and log that force mode was used. - Extend D1 migration tests to cover
--forcebehavior (and additional migration-application behavior). - Add a changeset documenting the new flag.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/wrangler/src/d1/migrations/apply.ts | Adds --force arg and conditional logic to bypass confirmation + log informational message. |
| packages/wrangler/src/tests/d1/migrate.test.ts | Adds test coverage for --force (and additional migration apply behavior). |
| .changeset/auto-apply-migrations.md | Documents the new --force flag in release notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setIsTTY(false); | ||
| const std = mockConsoleMethods(); | ||
|
|
There was a problem hiding this comment.
mockConsoleMethods() registers beforeEach/afterEach hooks and uses module-level spy variables. Calling it inside an individual it(...) block can register extra hooks for subsequent tests and potentially re-spy on already-spied console methods, leading to order-dependent/flaky behavior. Prefer using the suite-level mockStd declared at the top of this file (and getAndClearOut() to reset output between assertions) instead of creating a new console mock here.
|
|
||
| expect(std.out).toContain( | ||
| "--force passed, applying 1 migration(s) without prompt" | ||
| ); |
There was a problem hiding this comment.
This test is named “should not prompt when --force is passed”, but it only asserts that the force info message is present. If the command accidentally still calls confirm() (which logs the prompt text in non-interactive mode), this assertion would still pass. Consider asserting that the output does not contain the confirm prompt text (e.g. "About to apply" / "Using fallback value"), or use the dialogs mock to fail the test if confirm() is invoked.
| ); | |
| ); | |
| expect(std.out).not.toContain("About to apply"); |
Ehbraheem
force-pushed
the
wrangler/d1-migration-confirmation-skip
branch
from
600628a to
d98f1dd
Compare
|
@Ehbraheem can you address the review comments from Bonk? It seems like your tests have some bugs |
Fixes #5017.
Add a new --force boolean flag to
d1 migrations applythat skips the interactiveconfirmation and applies any pending migrations non-interactively. This is useful
for CI and automation where a prompt is undesirable.
Usage example:
Behavior:
--forceis present the command will not prompt and will proceed to applyall pending migrations. It logs a short informational message indicating force
was used.