Implement `triggers deploy` command by RamIdeas · Pull Request #5426 · cloudflare/workers-sdk

RamIdeas · 2024-03-28T16:19:59Z

What this PR solves / how to test

This PR implements the wrangler triggers deploy command behind the --experimental-versions flag.

I extracted the logic from wrangler deploy which I've now made call into the extracted function. The test coverage for wrangler deploy should cover wrangler triggers deploy.

The only difference this makes to wrangler deploy is:

~~the log line "Published <worker-name> (<timing>)" is now "Deployed <worker-name> triggers (<timing>)"~~ no longer different
and, instead of getting the value available_on_subdomain from the response of the PUT script call, it now makes an additional GET call to ".../subdomain" to get the value

~~It is possible to mitigate these two differences but I don't think the log line change or minimal added latency necessitates it. But I'm open to others' opinions :)~~ The only difference is the latency

CR: https://jira.cfdata.org/browse/CR-850334

Author has addressed the following

Tests
- TODO (before merge)
- Included
- Not necessary because:
Changeset (Changeset guidelines)
- TODO (before merge)
- Included
- Not necessary because:
Public documentation
- TODO (before merge)
- Cloudflare docs PR(s): Versions, Deployments, Gradual Deployments cloudflare-docs#13429
- Not necessary because:

changeset-bot · 2024-03-28T16:20:04Z

🦋 Changeset detected

Latest commit: 638387e

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

This PR includes changesets to release 2 packages

Name	Type
wrangler	Minor
@cloudflare/vitest-pool-workers	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

github-actions · 2024-03-28T16:29:01Z

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/8472555798/npm-package-wrangler-5426

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

wrangler@3.39.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240320.0
`workerd`	1.20240320.1	1.20240320.1
`workerd --version`	1.20240320.1	2024-03-20

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

codecov · 2024-03-28T16:44:32Z

Codecov Report

Attention: Patch coverage is 74.02597% with 40 lines in your changes are missing coverage. Please review.

Project coverage is 72.29%. Comparing base (70861a7) to head (44c2f27).
Report is 1 commits behind head on main.

❗ Current head 44c2f27 differs from pull request most recent head 638387e. Consider uploading reports for the commit 638387e to get more accurate results

Additional details and impacted files

@@            Coverage Diff            @@
##           main    #5426       +/-   ##
=========================================
+ Coverage      0   72.29%   +72.29%     
=========================================
  Files         0      325      +325     
  Lines         0    16765    +16765     
  Branches      0     4290     +4290     
=========================================
+ Hits          0    12120    +12120     
- Misses        0     4645     +4645

Files	Coverage Δ
...angler/src/__tests__/helpers/mock-upload-worker.ts	`98.96% <100.00%> (ø)`
packages/wrangler/src/deploy/deploy.ts	`93.23% <100.00%> (ø)`
packages/wrangler/src/metrics/send-event.ts	`100.00% <ø> (ø)`
packages/wrangler/src/index.ts	`90.30% <75.00%> (ø)`
packages/wrangler/src/triggers/index.ts	`27.77% <27.77%> (ø)`
packages/wrangler/src/triggers/deploy.ts	`76.85% <76.85%> (ø)`

... and 319 files with indirect coverage changes

… `wrangler deploy` but different in `wrangler triggers deploy`

penalosa · 2024-03-28T21:25:44Z

+
+This command extracts the trigger deployment logic from `wrangler deploy` and allows users to update their currently deployed Worker's triggers without doing another deployment. This is primarily useful for users of `wrangler versions upload` and `wrangler versions deploy` who can then run `wrangler triggers deploy` to apply trigger changes to their currently deployed Worker Versions.
+
+The command can also be used even if not using the `wrangler versions ...` commands. And, in fact, are already using it implicitly when running `wrangler deploy`.


This is missing a word. Maybe "developers are already using it implicitly when running wrangler deploy?

Fixed in #5436

penalosa · 2024-03-28T21:26:18Z

These snapshots should not be changed, but I assume this has been fixed in a followup?

Yes fixed in 006f1ae c234dad

penalosa · 2024-03-28T21:27:11Z

Same comment as for deploy.test.ts

Yes fixed in 006f1ae c234dad

penalosa · 2024-03-28T21:32:29Z

+		.option("dispatch-namespace", {
+			describe:
+				"Name of a dispatch namespace to deploy the Worker to (Workers for Platforms)",
+			type: "string",
+		});


I realise these options are copied from deploy, but this argument doesn't make sense for triggers

Fixed in #5436

penalosa · 2024-03-28T21:33:15Z

+	const configPath = args.config || findWranglerToml();
+	const config = readConfig(configPath, args);


You don't need to resolve the config path outside of readConfig(). The first argument should be undefined here

Fixed in #5436

penalosa · 2024-03-28T21:39:14Z

+	for (const route of routes) {
+		if (typeof route !== "string" && route.custom_domain) {
+			if (route.pattern.includes("*")) {
+				throw new UserError(
+					`Cannot use "${route.pattern}" as a Custom Domain; wildcard operators (*) are not allowed`
+				);
+			}
+			if (route.pattern.includes("/")) {
+				throw new UserError(
+					`Cannot use "${route.pattern}" as a Custom Domain; paths are not allowed`
+				);
+			}
+			customDomainsOnly.push(route);
+		} else {
+			routesOnly.push(route);
+		}
+	}


You've added logic for publishing routes here, but haven't removed it from wrangler deploy. Are you sure this won't cause duplicate deploys?

Definitely not causing duplicate deploys. This was left for the validation. The arrays are dead code which have been removed in #5436

This reverts commit 9343714.

lrapoport-cf · 2024-03-28T23:22:06Z

for posterity, i am approving this PR -- the concerns raised are addressed in the linked PRs/commits, which themselves have been approved

RamIdeas requested a review from a team as a code owner March 28, 2024 16:20

RamIdeas force-pushed the triggers-command branch from a868cc2 to 9ae60ad Compare March 28, 2024 16:25

RamIdeas added the e2e Run wrangler + vite-plugin e2e tests on a PR label Mar 28, 2024

RamIdeas added 5 commits March 28, 2024 17:45

implement triggers deploy command

a297fcc

extract trigger logic from deploy

b369286

update tests

f2dae80

add changeset

8223819

fix checks

44c2f27

RamIdeas force-pushed the triggers-command branch from f5f793d to 44c2f27 Compare March 28, 2024 17:45

propagate experimentalVersions flag to keep deploy output the same in…

638387e

… `wrangler deploy` but different in `wrangler triggers deploy`

RamIdeas merged commit 9343714 into main Mar 28, 2024

RamIdeas deleted the triggers-command branch March 28, 2024 19:29

workers-devprod mentioned this pull request Mar 28, 2024

Version Packages #5418

Merged

penalosa reviewed Mar 28, 2024

View reviewed changes

RamIdeas added a commit that referenced this pull request Mar 28, 2024

Revert "Implement triggers deploy command (#5426)"

5689894

This reverts commit 9343714.

RamIdeas added a commit that referenced this pull request Mar 28, 2024

Fix nits from #5426 (#5436)

2d1220f

risu729 mentioned this pull request Mar 30, 2024

🐛 BUG: crons = [] in wrangler.toml doesn't remove cron triggers #5450

Closed


		This command extracts the trigger deployment logic from `wrangler deploy` and allows users to update their currently deployed Worker's triggers without doing another deployment. This is primarily useful for users of `wrangler versions upload` and `wrangler versions deploy` who can then run `wrangler triggers deploy` to apply trigger changes to their currently deployed Worker Versions.

		The command can also be used even if not using the `wrangler versions ...` commands. And, in fact, are already using it implicitly when running `wrangler deploy`.

		const configPath = args.config \|\| findWranglerToml();
		const config = readConfig(configPath, args);

Conversation

RamIdeas commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR solves / how to test

Author has addressed the following

Uh oh!

changeset-bot Bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions Bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Mar 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lrapoport-cf commented Mar 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RamIdeas commented Mar 28, 2024 •

edited

Loading

changeset-bot Bot commented Mar 28, 2024 •

edited

Loading

github-actions Bot commented Mar 28, 2024 •

edited

Loading

codecov Bot commented Mar 28, 2024 •

edited

Loading