Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: create d1 dir on wrangler d1 execute command with --yes/-y option #2623

Merged
merged 1 commit into from
Jan 26, 2023
Merged

fix: create d1 dir on wrangler d1 execute command with --yes/-y option #2623

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/khaki-suns-boil.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

fix d1 directory not being created when running the `wrangler d1 execute` command with the `--yes`/`-y` flag
8 changes: 4 additions & 4 deletions packages/wrangler/src/d1/execute.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,10 @@ async function executeLocally(
logger.log
);

if (!existsSync(dbDir) && shouldPrompt) {
const ok = await confirm(
`About to create ${readableRelative(dbPath)}, ok?`
);
if (!existsSync(dbDir)) {
const ok =
!shouldPrompt ||
(await confirm(`About to create ${readableRelative(dbPath)}, ok?`));
Comment on lines +205 to +206
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we please get a test that covers the bug, for posterity 😄

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I considered that and asked @rozenmd about it, if I understood correctly he did mention that there isn't a proper test suite for d1 commands and that we could go with the fix as is.

If you want I could look into adding a test now but I'd have to set up the test suite (to some degree at least) I guess but since I am not familiar with things in this codebase nor I am really involved in d1 I'd imagine that it would be better for someone else to start (and I can add the test at that point?).

or I can give it a shot now if you think that'd be ok, I would be up for it 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I considered that and asked @rozenmd about it, if I understood correctly he did mention that there isn't a proper test suite for d1 commands and that we could go with the fix as is.

If you want I could look into adding a test now but I'd have to set up the test suite (to some degree at least) I guess but since I am not familiar with things in this codebase nor I am really involved in d1 I'd imagine that it would be better for someone else to start (and I can add the test at that point?).

or I can give it a shot now if you think that'd be ok, I would be up for it 🙂

If you are willing to try, I am willing to help as well. Anything we both can't figure out we can kick towards Max 😆

Copy link
Contributor

@JacobMGEvans JacobMGEvans Jan 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally if you want to just get this fix in and do a fast follow Test PR (since none previously existed) I would support that in this particular case given the circumstances.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah sorry, I just now say the comment about merging the PR anyways, thanks 🙂👍

if (!ok) return null;
await mkdir(dbDir, { recursive: true });
}
Expand Down