feat: wrangler deploy prompts warning with deployment #5992

danielrs · 2024-06-06T23:57:07Z

What this PR solves / how to test

Shows a warning to avoid accidental overwrite of a percentage-based deployment.

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
E2E Tests CI Job required? (Use "e2e" label or ask maintainer to run separately)
- I don't know
- Required / Maybe required
- Not required because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s):
- Not necessary because:

changeset-bot · 2024-06-06T23:57:11Z

⚠️ No Changeset found

Latest commit: 89dca41

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2024-06-06T23:59:30Z

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/9502295694/npm-package-wrangler-5992

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9502295694/npm-package-create-cloudflare-5992 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9502295694/npm-package-cloudflare-kv-asset-handler-5992

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9502295694/npm-package-miniflare-5992

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

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9502295694/npm-package-cloudflare-vitest-pool-workers-5992

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

[email protected] includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240605.0
`workerd`	1.20240610.1	1.20240610.1
`workerd --version`	1.20240610.1	2024-06-10

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

RamIdeas · 2024-06-07T11:09:43Z

packages/wrangler/src/versions/deploy.ts

+		const latest = await fetchLatestDeployment(accountId, scriptName);
+		if (latest && latest.versions.length >= 2) {
+			logger.warn(
+				`Your latest deployment has multiple versions.\n"wrangler deploy" will upload a new version and deploy it globally immediately.`


Should this also include a mention of "wrangler versions deploy" as the alternative?

tanushree-sharma · 2024-06-09T16:33:24Z

We should return the current status of the deployment + allow users to override and wrangler deploy.

Copy (having a hard time with formatting in comments):

> wrangler deploy

▲ [WARNING] Your last deployment has multiple versions. To progress this deployment use "wrangler versions deploy" instead. 

Currently deployed versions:

Version(s):  (50%) d58c1fb8-6015-410f-8fb7-e2818788b337
                    Created:  2024-06-07T13:21:14.974Z
             
                   (50%) 2ea0b4e6-f593-41cf-8a70-f0ef64bb0b02
                   Created:  2024-06-05T22:17:53.355Z
 
"wrangler deploy" will upload a new version and deploy it globally immediately. Are you sure you want to continue? (y/n).

RamIdeas · 2024-06-12T16:31:13Z

packages/wrangler/src/versions/deploy.ts

+			const versionIds = latest.versions.map((v) => v.version_id);
+			await fetchVersions(accountId, scriptName, versionCache, ...versionIds);
+
+			// Format each version.
+
+			const formattedVersions = latest.versions.map((traffic) => {
+				const version = versionCache.get(traffic.version_id);
+				assert(version);
+				const percentage = brandColor(`(${traffic.percentage}%)`);
+				const details = formatLabelledValues(
+					{ Created: new Date(version.metadata["created_on"]).toISOString() },
+					{
+						indentationCount: 4,
+						labelJustification: "right",
+						formatLabel: (label) => gray(label + ":"),
+						formatValue: (value) => gray(value),
+					}
+				);
+				return `${percentage} ${version.id}\n${details}`;
+			});
+
+			// Format deployment.
+
+			const formattedDeployment = formatLabelledValues({
+				"Version(s)": formattedVersions.join("\n\n"),
+			});
+
+			// Print message and confirmation.


Can this all be replaced with a call to printLatestDeployment (the function right below this one)?

And then the styling will remain consistent

RamIdeas · 2024-06-12T16:32:06Z

packages/wrangler/src/versions/deploy.ts

+			return await confirm(
+				`"wrangler deploy" will upload a new version and deploy it globally immediately.\nAre you sure you want to continue?`
+			);


Can you use inputPrompt instead of confirm please? There are other usages in this file you can use for reference

petebacondarwin · 2024-06-12T16:04:06Z

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

@@ -423,6 +428,34 @@ describe("deploy", () => {
 		`);
 		});

+		it("should warn user when worker has deployment with multiple versions", async () => {


Can we have a test that checks what happens if the user says yes?
Also what happens if the console is non-interactive? Can that be tested too?

petebacondarwin

Regarding #5992 (comment)

I see now that all the tests are non-interactive.
Why are there not tests of the user interactively agreeing or disagreeing with the deployment?

petebacondarwin · 2024-06-13T10:51:11Z

packages/cli/index.ts

 	} = {}
 ) => {
 	logRaw(
 		format(msg, {
 			firstLinePrefix: gray(shape) + space() + status.warning,
 			linePrefix: gray(shapes.bar),
-			newlineBefore: true,
+			newlineBefore: newlineBefore,


NIT:

Suggested change

newlineBefore: newlineBefore,

newlineBefore,

packages/wrangler/src/deploy/deploy.ts

packages/wrangler/src/versions/deploy.ts

RamIdeas · 2024-06-13T13:25:33Z

packages/wrangler/src/versions/deploy.ts

+			const versionCache: VersionCache = new Map();
+			const versionIds = latest.versions.map((v) => v.version_id);
+			await fetchVersions(accountId, scriptName, versionCache, ...versionIds);
+
+			const versions = latest.versions.map((v) => {
+				const version = versionCache.get(v.version_id);
+				assert(version);
+				return version;
+			});
+
+			const traffic = new Map(
+				latest.versions.map((v) => [v.version_id, v.percentage])
+			);


Besides styling, reusing printLatestDeployments would have also avoided the need to duplicate this logic.

The only difference I see is the preamble "current" vs "last":

- `${leftT} Your current deployment has ${versions.length} version(s):` + `${leftT} Your last deployment has ${versions.length} version(s):`

nit: you can adapt printLatestDeployment to accept an additional param: adjective: "current" | "last" and therefore trim this function down and avoid the duplication

What I didn't like about printLatestDeployment is that it would fetch the deployment again when I already had it. Ended up refactoring to a function named printDeployment that lets you pass and print any deployment as a parameter.

I had an idea to pass a callback param like skipIf(deployment: ApiDeployment) => boolean which would enable flow control using the one api request

but this is fine for now, and I can refactor in a follow up if we decide it's worth it

Styling of the warning about multiple deployments during `wrangler deploy` should be consistent with `wrangler versions` styling.

packages/wrangler/src/versions/deploy.ts

Co-authored-by: Rahul Sethi <[email protected]>

holopin-bot · 2024-06-13T15:32:02Z

Congratulations @danielrs, you just earned a holobyte! Here it is: https://holopin.io/holobyte/clxdf4dwm87490dl3krezvud5

This badge can only be claimed by you, so make sure that your GitHub account is linked to your Holopin account. You can manage those preferences here: https://holopin.io/account.
Or if you're new to Holopin, you can simply sign up with GitHub, which will do the trick!

danielrs requested a review from a team as a code owner June 6, 2024 23:57

RamIdeas reviewed Jun 7, 2024

View reviewed changes

danielrs force-pushed the drivas/wrangler-deploy-warning-on-overwrite branch from d0cae7e to ca2cf3f Compare June 7, 2024 13:52

feat: wrangler deploy prompts warning with deployment

aa18a35

danielrs force-pushed the drivas/wrangler-deploy-warning-on-overwrite branch from ca2cf3f to aa18a35 Compare June 10, 2024 20:01

danielrs requested a review from RamIdeas June 10, 2024 20:23

RamIdeas reviewed Jun 12, 2024

View reviewed changes

petebacondarwin reviewed Jun 12, 2024

View reviewed changes

danielrs force-pushed the drivas/wrangler-deploy-warning-on-overwrite branch 2 times, most recently from cc0fd7c to a1d66af Compare June 12, 2024 21:12

petebacondarwin reviewed Jun 13, 2024

View reviewed changes

RamIdeas reviewed Jun 13, 2024

View reviewed changes

packages/wrangler/src/deploy/deploy.ts Show resolved Hide resolved

RamIdeas reviewed Jun 13, 2024

View reviewed changes

packages/wrangler/src/versions/deploy.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Jun 13, 2024

View reviewed changes

packages/wrangler/src/versions/deploy.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Jun 13, 2024

View reviewed changes

fix: wrangler deploy prompts warning with deployment styling

841bc7d

Styling of the warning about multiple deployments during `wrangler deploy` should be consistent with `wrangler versions` styling.

danielrs force-pushed the drivas/wrangler-deploy-warning-on-overwrite branch from a1d66af to 841bc7d Compare June 13, 2024 15:01

danielrs requested a review from RamIdeas June 13, 2024 15:06

RamIdeas reviewed Jun 13, 2024

View reviewed changes

packages/wrangler/src/versions/deploy.ts Outdated Show resolved Hide resolved

RamIdeas reviewed Jun 13, 2024

View reviewed changes

packages/wrangler/src/versions/deploy.ts Outdated Show resolved Hide resolved

RamIdeas approved these changes Jun 13, 2024

View reviewed changes

danielrs and others added 2 commits June 13, 2024 10:14

Fix typo in packages/wrangler/src/versions/deploy.ts

8439ca3

Co-authored-by: Rahul Sethi <[email protected]>

Fix typo in packages/wrangler/src/versions/deploy.ts

89dca41

Co-authored-by: Rahul Sethi <[email protected]>

petebacondarwin merged commit c13fb91 into cloudflare:main Jun 13, 2024
19 checks passed

workers-devprod added the contribution [Holopin] Recognizes an open-source contribution, big or small label Jun 13, 2024

petebacondarwin added a commit that referenced this pull request Jun 13, 2024

add changeset for PR #5992

53acdbc

danielrs deleted the drivas/wrangler-deploy-warning-on-overwrite branch June 13, 2024 15:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: wrangler deploy prompts warning with deployment #5992

feat: wrangler deploy prompts warning with deployment #5992

danielrs commented Jun 6, 2024

changeset-bot bot commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading

RamIdeas Jun 7, 2024

tanushree-sharma commented Jun 9, 2024 •

edited

Loading

RamIdeas Jun 12, 2024 •

edited

Loading

RamIdeas Jun 12, 2024

petebacondarwin Jun 12, 2024

petebacondarwin left a comment

petebacondarwin Jun 13, 2024

RamIdeas Jun 13, 2024

danielrs Jun 13, 2024

RamIdeas Jun 13, 2024

holopin-bot bot commented Jun 13, 2024

feat: wrangler deploy prompts warning with deployment #5992

feat: wrangler deploy prompts warning with deployment #5992

Conversation

danielrs commented Jun 6, 2024

What this PR solves / how to test

Author has addressed the following

changeset-bot bot commented Jun 6, 2024 • edited Loading

⚠️ No Changeset found

github-actions bot commented Jun 6, 2024 • edited Loading

RamIdeas Jun 7, 2024

Choose a reason for hiding this comment

tanushree-sharma commented Jun 9, 2024 • edited Loading

RamIdeas Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

RamIdeas Jun 12, 2024

Choose a reason for hiding this comment

petebacondarwin Jun 12, 2024

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Jun 13, 2024

Choose a reason for hiding this comment

RamIdeas Jun 13, 2024

Choose a reason for hiding this comment

danielrs Jun 13, 2024

Choose a reason for hiding this comment

RamIdeas Jun 13, 2024

Choose a reason for hiding this comment

holopin-bot bot commented Jun 13, 2024

changeset-bot bot commented Jun 6, 2024 •

edited

Loading

github-actions bot commented Jun 6, 2024 •

edited

Loading

tanushree-sharma commented Jun 9, 2024 •

edited

Loading

RamIdeas Jun 12, 2024 •

edited

Loading