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

dario-piotrowicz · 2023-01-25T14:33:03Z

What this PR solves / how to test:

Associated docs issues/PR:

[insert associated docs issue(s)/PR(s)]

Author has included the following, where applicable:

Tests
Changeset

Reviewer has performed the following, where applicable:

Checked for inclusion of relevant tests
Checked for inclusion of a relevant changeset
Checked for creation of associated docs updates
Manually pulled down the changes and spot-tested

Fixes # [insert issue number].

changeset-bot · 2023-01-25T14:33:07Z

🦋 Changeset detected

Latest commit: 240c1b2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
wrangler	Patch

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

rozenmd

LGTM 👍

codecov · 2023-01-25T14:49:37Z

Codecov Report

Merging #2623 (240c1b2) into main (416babf) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2623      +/-   ##
==========================================
- Coverage   73.19%   73.19%   -0.01%     
==========================================
  Files         159      159              
  Lines        9847     9848       +1     
  Branches     2622     2624       +2     
==========================================
  Hits         7208     7208              
- Misses       2639     2640       +1

Impacted Files	Coverage Δ
packages/wrangler/src/d1/execute.tsx	`16.80% <0.00%> (-0.14%)`	⬇️

JacobMGEvans

The confirm infers automation environments, CI/CD, non-TTY, etc... Then does an automatic skip of prompts. Is there another reason besides those for this flag?

github-actions · 2023-01-25T15:43:17Z

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/runs/4006950162/npm-package-wrangler-2623

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/prs/2623/npm-package-wrangler-2623

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/runs/4006950162/npm-package-wrangler-2623 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/runs/4006950162/npm-package-cloudflare-pages-shared-2623

rozenmd · 2023-01-25T16:27:03Z

the same as wrangler init -y @JacobMGEvans, "wrangler, I know what I'm doing, just give me the thing"

dario-piotrowicz · 2023-01-25T16:50:44Z

@JacobMGEvans yeah that's like @rozenmd mentioned, specifically I have some d1 apps I want to share, they have a dev script with runs multiple stuff in parallel, the issue there is that the prompt makes things awkward (what if the developer doesn't accept for example? or the d1 prompt can gets lost in other cli text)

but yeah it boils down to the fact that I am providing the command to execute and I know that it's going to create the db, there is no reason to ask me (of the devs I share the apps with) to confirm

JacobMGEvans · 2023-01-25T17:17:50Z

@JacobMGEvans yeah that's like @rozenmd mentioned, specifically I have some d1 apps I want to share, they have a dev script with runs multiple stuff in parallel, the issue there is that the prompt makes things awkward (what if the developer doesn't accept for example? or the d1 prompt can gets lost in other cli text)

but yeah it boils down to the fact that I am providing the command to execute and I know that it's going to create the db, there is no reason to ask me (of the devs I share the apps with) to confirm

Thank you for the elaboration.

JacobMGEvans

Given the clarification that this is more to handle User workflows and not automation environments, the implementation makes sense.

JacobMGEvans

~~This needs an associated cloudflare-docs PR~~ Uses preexisting --yes flag

Any particular reason there isn't a test, like it's hard to test?

rozenmd · 2023-01-25T18:01:12Z

This needs an associated cloudflare-docs PR

@JacobMGEvans no it doesn't? it's a bug fix

JacobMGEvans · 2023-01-25T18:25:25Z

This needs an associated cloudflare-docs PR

@JacobMGEvans no it doesn't? it's a bug fix

Ah I see you're using an existing top --yes not adding one for the fix. Not sure why I thought it was adding one 😆

JacobMGEvans

It is a simple fix, if we can write a test that covers the scenario that was previously breaking to help prevent the same bug or similar in the future that would be awesome 😃

JacobMGEvans · 2023-01-25T20:35:30Z

packages/wrangler/src/d1/execute.tsx

+			!shouldPrompt ||
+			(await confirm(`About to create ${readableRelative(dbPath)}, ok?`));


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

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 🙂

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 😆

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.

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

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.

dario-piotrowicz requested a review from a team as a code owner January 25, 2023 14:33

dario-piotrowicz force-pushed the fix-mkdir-execute-d1 branch from 1b23117 to 15f29e9 Compare January 25, 2023 14:33

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

Verified

This commit was signed with the committer’s verified signature.

snyk-bot Snyk bot

GPG key ID: 5009E0832F4D3DD3

Verified
Learn about vigilant mode

240c1b2

dario-piotrowicz force-pushed the fix-mkdir-execute-d1 branch from 15f29e9 to 240c1b2 Compare January 25, 2023 14:35

rozenmd approved these changes Jan 25, 2023

View reviewed changes

JacobMGEvans suggested changes Jan 25, 2023

View reviewed changes

dario-piotrowicz requested a review from JacobMGEvans January 25, 2023 16:58

JacobMGEvans approved these changes Jan 25, 2023

View reviewed changes

JacobMGEvans suggested changes Jan 25, 2023

View reviewed changes

rozenmd requested a review from JacobMGEvans January 25, 2023 20:19

JacobMGEvans previously requested changes Jan 25, 2023

View reviewed changes

dario-piotrowicz requested a review from JacobMGEvans January 25, 2023 21:36

rozenmd merged commit 04d8a31 into cloudflare:main Jan 26, 2023

github-actions bot mentioned this pull request Jan 26, 2023

Version Packages #2595

Merged

dario-piotrowicz deleted the fix-mkdir-execute-d1 branch January 28, 2023 22:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

dario-piotrowicz commented Jan 25, 2023 •

edited by JacobMGEvans

Loading

changeset-bot bot commented Jan 25, 2023 •

edited

Loading

rozenmd left a comment

codecov bot commented Jan 25, 2023 •

edited

Loading

JacobMGEvans left a comment •

edited

Loading

github-actions bot commented Jan 25, 2023 •

edited

Loading

rozenmd commented Jan 25, 2023

dario-piotrowicz commented Jan 25, 2023

JacobMGEvans commented Jan 25, 2023

JacobMGEvans left a comment

JacobMGEvans left a comment •

edited

Loading

rozenmd commented Jan 25, 2023

JacobMGEvans commented Jan 25, 2023

JacobMGEvans left a comment

JacobMGEvans Jan 25, 2023

dario-piotrowicz Jan 25, 2023

JacobMGEvans Jan 25, 2023

JacobMGEvans Jan 25, 2023 •

edited

Loading

dario-piotrowicz Jan 28, 2023

		!shouldPrompt \|\|
		(await confirm(`About to create ${readableRelative(dbPath)}, ok?`));

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

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

Conversation

dario-piotrowicz commented Jan 25, 2023 • edited by JacobMGEvans Loading

changeset-bot bot commented Jan 25, 2023 • edited Loading

🦋 Changeset detected

rozenmd left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 25, 2023 • edited Loading

Codecov Report

JacobMGEvans left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Jan 25, 2023 • edited Loading

rozenmd commented Jan 25, 2023

dario-piotrowicz commented Jan 25, 2023

JacobMGEvans commented Jan 25, 2023

JacobMGEvans left a comment

Choose a reason for hiding this comment

JacobMGEvans left a comment • edited Loading

Choose a reason for hiding this comment

rozenmd commented Jan 25, 2023

JacobMGEvans commented Jan 25, 2023

JacobMGEvans left a comment

Choose a reason for hiding this comment

JacobMGEvans Jan 25, 2023

Choose a reason for hiding this comment

dario-piotrowicz Jan 25, 2023

Choose a reason for hiding this comment

JacobMGEvans Jan 25, 2023

Choose a reason for hiding this comment

JacobMGEvans Jan 25, 2023 • edited Loading

Choose a reason for hiding this comment

dario-piotrowicz Jan 28, 2023

Choose a reason for hiding this comment

dario-piotrowicz commented Jan 25, 2023 •

edited by JacobMGEvans

Loading

changeset-bot bot commented Jan 25, 2023 •

edited

Loading

codecov bot commented Jan 25, 2023 •

edited

Loading

JacobMGEvans left a comment •

edited

Loading

github-actions bot commented Jan 25, 2023 •

edited

Loading

JacobMGEvans left a comment •

edited

Loading

JacobMGEvans Jan 25, 2023 •

edited

Loading