WC-1308 Reduce force-deletes unless explicitly confirmed #3426

matthewdavidrodgers · 2023-06-09T21:53:02Z

Changes the default ?force param to the delete API call to be false.

Also uses the /scripts/{scriptName}/references and /tails/by-consumer/{scriptName} endpoints to determine what resources depend on the current script. We check for other scripts that use this one as outbound worker (for dynamic dispatch), as a tail worker, as a service binding, or use a durable object implemented by this script. If any other resources rely on this worker, we require another step of confirmation, describing the dependencies, and only then do we allow doing a force delete.

for WC-1308

Author has included the following, where applicable:

Tests
Changeset (Changeset guidelines)

Reviewer is to perform the following, as 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

changeset-bot · 2023-06-09T21:53:05Z

🦋 Changeset detected

Latest commit: a71815c

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

This PR includes changesets to release 1 package

Name	Type
wrangler	Minor

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

matthewdavidrodgers · 2023-06-09T21:53:35Z

Definitely open to any suggestions on copy here - see the included test for an example

github-actions · 2023-06-09T21:55:26Z

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/workers-sdk/runs/5467339575/npm-package-wrangler-3426

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5467339575/npm-package-wrangler-3426 dev path/to/script.js

Additional artifacts:

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/5467339575/npm-package-cloudflare-pages-shared-3426

Note that these links will no longer work once the GitHub Actions artifact expires.

codecov · 2023-06-09T22:02:58Z

Codecov Report

Merging #3426 (a71815c) into main (12930ae) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3426      +/-   ##
==========================================
+ Coverage   75.09%   75.24%   +0.14%     
==========================================
  Files         190      190              
  Lines       11110    11160      +50     
  Branches     2919     2944      +25     
==========================================
+ Hits         8343     8397      +54     
+ Misses       2767     2763       -4

Impacted Files	Coverage Δ
packages/wrangler/src/delete.ts	`100.00% <100.00%> (ø)`

... and 3 files with indirect coverage changes

.changeset/twelve-pumpkins-crash.md

packages/wrangler/src/delete.ts

penalosa · 2023-06-28T17:44:37Z

Actually, looking at this again—the delete command should take a --force flag to override the checking behaviour

lrapoport-cf · 2023-06-28T17:50:17Z

Actually, looking at this again—the delete command should take a --force flag to override the checking behaviour

@penalosa should merge be blocked on this? if so, can you update your approval :)

Force flag

penalosa

This should have a --force flag

penalosa

Looks good to me. Let's have another person in @cloudflare/wrangler take a pass before merging though

packages/wrangler/src/__tests__/delete.test.ts

packages/wrangler/src/deployment-bundle/worker.ts

petebacondarwin

Nice PR! Really clean and understandable code.

Can you confirm that the new requests are to stable public APIs? So that Wrangler won't get broken by them being changed or removed.

I think it would be good to only ask about deleting if there is no --force flag, and also can we move the new types into the delete.ts file.

Otherwise LGTM!

Changes the default ?force param to the delete API call to be false. Also uses the /scripts/{scriptName}/references and /tails/by-consumer/{scriptName} endpoints to determine what resources depend on the current script. We check for other scripts that use this one as outbound worker (for dynamic dispatch), as a tail worker, as a service binding, or use a durable object implemented by this script. If any other resources rely on this worker, we require another step of confirmation, describing the dependencies, and only then do we allow doing a force delete.

petebacondarwin

❤️

petebacondarwin · 2023-07-06T10:06:55Z

Um last thought... is this a breaking change for users that previously expected their deletions to be forced through, even if that is bad for them?

matthewdavidrodgers · 2023-07-06T18:52:04Z

Um last thought... is this a breaking change for users that previously expected their deletions to be forced through, even if that is bad for them?

erm, I don't know. I suppose you could classify it as a breaking change, albeit not a very impactful one

petebacondarwin · 2023-07-06T19:32:21Z

OK so I think the opinion is that this does not deserve a major versio bump.
Looks good to me. Let's

Changes the default ?force param to the delete API call to be false. Also uses the /scripts/{scriptName}/references and /tails/by-consumer/{scriptName} endpoints to determine what resources depend on the current script. We check for other scripts that use this one as outbound worker (for dynamic dispatch), as a tail worker, as a service binding, or use a durable object implemented by this script. If any other resources rely on this worker, we require another step of confirmation, describing the dependencies, and only then do we allow doing a force delete.

matthewdavidrodgers marked this pull request as ready for review June 21, 2023 16:49

matthewdavidrodgers requested review from a team as code owners June 21, 2023 16:49

matthewdavidrodgers force-pushed the reduce-force-deletes branch from 18edc5a to c6e5ec9 Compare June 21, 2023 19:00

penalosa reviewed Jun 26, 2023

View reviewed changes

matthewdavidrodgers force-pushed the reduce-force-deletes branch from c6e5ec9 to 722d8d6 Compare June 26, 2023 17:09

penalosa previously approved these changes Jun 28, 2023

View reviewed changes

penalosa self-requested a review June 28, 2023 17:43

penalosa requested changes Jun 29, 2023

View reviewed changes

matthewdavidrodgers force-pushed the reduce-force-deletes branch 4 times, most recently from fe083c6 to 60b7c20 Compare June 29, 2023 21:29

matthewdavidrodgers requested a review from penalosa June 29, 2023 22:03

matthewdavidrodgers force-pushed the reduce-force-deletes branch from 60b7c20 to 3100941 Compare June 30, 2023 15:32

penalosa approved these changes Jun 30, 2023

View reviewed changes

petebacondarwin reviewed Jul 5, 2023

View reviewed changes

packages/wrangler/src/__tests__/delete.test.ts Outdated Show resolved Hide resolved

petebacondarwin reviewed Jul 5, 2023

View reviewed changes

packages/wrangler/src/__tests__/delete.test.ts Show resolved Hide resolved

petebacondarwin reviewed Jul 5, 2023

View reviewed changes

packages/wrangler/src/deployment-bundle/worker.ts Outdated Show resolved Hide resolved

petebacondarwin requested changes Jul 5, 2023

View reviewed changes

matthewdavidrodgers force-pushed the reduce-force-deletes branch from 3100941 to a71815c Compare July 5, 2023 17:44

matthewdavidrodgers requested a review from petebacondarwin July 5, 2023 17:46

petebacondarwin approved these changes Jul 6, 2023

View reviewed changes

matthewdavidrodgers merged commit 5a74cb5 into cloudflare:main Jul 6, 2023
12 checks passed

github-actions bot mentioned this pull request Jul 6, 2023

Version Packages #3562

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WC-1308 Reduce force-deletes unless explicitly confirmed #3426

WC-1308 Reduce force-deletes unless explicitly confirmed #3426

matthewdavidrodgers commented Jun 9, 2023 •

edited

Loading

changeset-bot bot commented Jun 9, 2023 •

edited

Loading

matthewdavidrodgers commented Jun 9, 2023

github-actions bot commented Jun 9, 2023 •

edited

Loading

codecov bot commented Jun 9, 2023 •

edited

Loading

penalosa commented Jun 28, 2023

lrapoport-cf commented Jun 28, 2023

penalosa left a comment

penalosa left a comment

petebacondarwin left a comment

petebacondarwin left a comment

petebacondarwin commented Jul 6, 2023

matthewdavidrodgers commented Jul 6, 2023

petebacondarwin commented Jul 6, 2023

WC-1308 Reduce force-deletes unless explicitly confirmed #3426

WC-1308 Reduce force-deletes unless explicitly confirmed #3426

Conversation

matthewdavidrodgers commented Jun 9, 2023 • edited Loading

changeset-bot bot commented Jun 9, 2023 • edited Loading

🦋 Changeset detected

matthewdavidrodgers commented Jun 9, 2023

github-actions bot commented Jun 9, 2023 • edited Loading

codecov bot commented Jun 9, 2023 • edited Loading

Codecov Report

penalosa commented Jun 28, 2023

lrapoport-cf commented Jun 28, 2023

penalosa left a comment

Choose a reason for hiding this comment

penalosa left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin commented Jul 6, 2023

matthewdavidrodgers commented Jul 6, 2023

petebacondarwin commented Jul 6, 2023

matthewdavidrodgers commented Jun 9, 2023 •

edited

Loading

changeset-bot bot commented Jun 9, 2023 •

edited

Loading

github-actions bot commented Jun 9, 2023 •

edited

Loading

codecov bot commented Jun 9, 2023 •

edited

Loading