fix(wrangler): fix `build-env` to exit with code rather than throw fatal error #5572

CarmenPopoviciu · 2024-04-09T20:20:21Z

What this PR solves / how to test

Currently pages functions build-env throws a fatal error if a config file does not exit, or if it is
invalid. This causes issues for the CI system. We
should instead exit with a specific code, if any of those situations arises.

You can test by running npx wrangler pages function build-env [directory] --outfile=<file-name.json>

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): https://github.com/cloudflare/cloudflare-docs/pull/...
- Not necessary because: not public facing api

How this PR was tested

with valid Pages config file

with non-Pages config file + WRANGLER_LOG="debug"

with invalid Pages config file + WRANGLER_LOG="debug"

with invalid non-Pages config file + WRANGLER_LOG="debug"

with no config file + WRANGLER_LOG="debug"

PREVIEW OF CI ERR MESSAGING

changeset-bot · 2024-04-09T20:20:26Z

🦋 Changeset detected

Latest commit: 0c5d39e

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-04-09T20:23:23Z

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/8646310607/npm-package-wrangler-5572

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

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

Or you can use npx with this latest build directly:

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

Additional artifacts:

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

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

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

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

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

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

wrangler@3.49.0 includes the following runtime dependencies:

Package	Constraint	Resolved
`miniflare`	workspace:*	3.20240405.0
`workerd`	1.20240405.0	1.20240405.0
`workerd --version`	1.20240405.0	2024-04-05

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

packages/wrangler/src/pages/build-env.ts

petebacondarwin

Please can we remove the calls to process.exit()

petebacondarwin

I wonder if, separately to this we should mention in the logs (especially for when there is a warning) that wrangler.toml usage is beta?

packages/wrangler/src/pages/build-env.ts

RamIdeas · 2024-04-10T14:49:53Z

packages/wrangler/src/pages/build-env.ts

+			true
+		);
+	} catch (err) {
+		logger.log("Invalid Pages configuration file found. Exiting.");


This should be a warning using logger.warn which will write to stderr instead of stdout

This shouldn't be a warning, which will show up as a scary messaging, when this is an expected case. This should be a debug log

I think this underlines the need for the text of these messages to be less scary themselves.

I went ahead and done the following wrt messaging:

introduced a one liner BETA log (Checking for configuration in a wrangler.toml (BETA)) as per our conversation. Keeping in wrangler so we can remove more easily when time comes

used logger.debug instead of logger.log in the wrangler command. This gets rid of the double messaging we were seeing in the CI logs. Makes running the build-env command in the terminal a bit unfriendly if not run with WRANGLER_LOG="debug". However, since this is not a public API, I'm OK with the tradeoff

Updated the CI logs as per our conversation

I'll update description with how the final messaging looks like in CI, once all things are built

I assume the (BETA) message is in pages-infra?

I kept the (BETA) message in wrangler, so it stays within our scope, and can remove more easily when time comes

packages/wrangler/src/pages/build-env.ts

…tal error Currently `pages functions build-env` throws a fatal error if a config file does not exit, or if it is invalid. This causes issues for the CI system. We should instead exit with a specific code, if any of those situations arises.

petebacondarwin · 2024-04-11T11:57:43Z

packages/wrangler/src/pages/build-env.ts

+			true
+		);
+	} catch (err) {
+		logger.log("Invalid Pages configuration file found. Exiting.");


I assume the (BETA) message is in pages-infra?

jahands · 2024-06-07T23:49:47Z

packages/wrangler/src/pages/build-env.ts

+		);
+	} catch (err) {
+		logger.debug("wrangler.toml file is invalid. Exiting.");
+		process.exitCode = EXIT_CODE_INVALID_PAGES_CONFIG;


Why did we do this format instead of throwing a fatal error with a custom error code like we do for other errors communicated to Pages CI? e.g. https://github.com/cloudflare/workers-sdk/blob/main/packages/wrangler/src/config/index.ts#L93

We don't exit with FatalErrors any more (that one you linked is caught by this catch handler)

CarmenPopoviciu requested review from a team as code owners April 9, 2024 20:20

CarmenPopoviciu force-pushed the carmen/build-env branch from 031565a to a3371e2 Compare April 9, 2024 20:23

penalosa approved these changes Apr 10, 2024

View reviewed changes

petebacondarwin reviewed Apr 10, 2024

View reviewed changes

packages/wrangler/src/pages/build-env.ts Outdated Show resolved Hide resolved

petebacondarwin requested changes Apr 10, 2024

View reviewed changes

petebacondarwin approved these changes Apr 10, 2024

View reviewed changes

RamIdeas reviewed Apr 10, 2024

View reviewed changes

packages/wrangler/src/pages/build-env.ts Show resolved Hide resolved

RamIdeas reviewed Apr 10, 2024

View reviewed changes

packages/wrangler/src/pages/build-env.ts Show resolved Hide resolved

CarmenPopoviciu added 3 commits April 11, 2024 08:53

PR review fixes

100f109

PR review fixes2

0c5d39e

petebacondarwin approved these changes Apr 11, 2024

View reviewed changes

CarmenPopoviciu force-pushed the carmen/build-env branch from bc804d9 to 0c5d39e Compare April 11, 2024 12:01

petebacondarwin approved these changes Apr 11, 2024

View reviewed changes

CarmenPopoviciu merged commit 65aa21c into main Apr 11, 2024
15 checks passed

CarmenPopoviciu deleted the carmen/build-env branch April 11, 2024 13:03

workers-devprod mentioned this pull request Apr 11, 2024

Version Packages #5582

Merged

jahands reviewed Jun 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(wrangler): fix `build-env` to exit with code rather than throw fatal error #5572

fix(wrangler): fix `build-env` to exit with code rather than throw fatal error #5572

CarmenPopoviciu commented Apr 9, 2024 •

edited

Loading

changeset-bot bot commented Apr 9, 2024 •

edited

Loading

github-actions bot commented Apr 9, 2024 •

edited

Loading

petebacondarwin left a comment

petebacondarwin left a comment

RamIdeas Apr 10, 2024

penalosa Apr 10, 2024

petebacondarwin Apr 11, 2024

CarmenPopoviciu Apr 11, 2024

petebacondarwin Apr 11, 2024

CarmenPopoviciu Apr 11, 2024

petebacondarwin Apr 11, 2024

jahands Jun 7, 2024

penalosa Jun 10, 2024

fix(wrangler): fix build-env to exit with code rather than throw fatal error #5572

fix(wrangler): fix build-env to exit with code rather than throw fatal error #5572

Conversation

CarmenPopoviciu commented Apr 9, 2024 • edited Loading

What this PR solves / how to test

Author has addressed the following

How this PR was tested

changeset-bot bot commented Apr 9, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 9, 2024 • edited Loading

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fix(wrangler): fix `build-env` to exit with code rather than throw fatal error #5572

fix(wrangler): fix `build-env` to exit with code rather than throw fatal error #5572

CarmenPopoviciu commented Apr 9, 2024 •

edited

Loading

changeset-bot bot commented Apr 9, 2024 •

edited

Loading

github-actions bot commented Apr 9, 2024 •

edited

Loading