Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(wrangler): Show feedback on Pages Function upload failure #5819

Merged
merged 6 commits into from
May 15, 2024

Conversation

CarmenPopoviciu
Copy link
Contributor

@CarmenPopoviciu CarmenPopoviciu commented May 14, 2024

What this PR solves / how to test

Today, if uploading a Pages Function, or deploying a Pages project fails for whatever reason, there’s no feedback shown to the user. Worse yet, the shown message is misleading, saying the deployment was successful, when in fact it was not:

Screenshot 2024-05-14 at 14 22 31

This PR ensures that moving forward, we provide users with:

  • the correct feedback with respect to their Pages deployment
  • the appropriate messaging depending on the status of their project's deployment status
  • the appropriate logs in case of a deployment failure

Fixes #3966 and WC-2090.

Before
Screenshot 2024-05-14 at 14 22 31

After

  • success
Screenshot 2024-05-15 at 16 45 35
  • failure
Screenshot 2024-05-15 at 16 46 42
  • failure with log level set to debug
Screenshot 2024-05-15 at 16 48 36
  • unknown deployment status
Screenshot 2024-05-15 at 16 57 16

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: no e2e tests changes were made
  • Changeset (Changeset guidelines)
    • TODO (before merge)
    • Included
    • Not necessary because:
  • Public documentation

@CarmenPopoviciu CarmenPopoviciu requested review from a team as code owners May 14, 2024 11:44
Copy link

changeset-bot bot commented May 14, 2024

🦋 Changeset detected

Latest commit: 4cf2706

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

Copy link
Member

@GregBrimble GregBrimble left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Carmen! Once you've got a sleep and cap added in to that loop, pretty much ready to merge, imo!

packages/wrangler/src/pages/deploy.tsx Outdated Show resolved Hide resolved
packages/wrangler/src/pages/deploy.tsx Outdated Show resolved Hide resolved
@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/fn-upload-failure branch 2 times, most recently from a6f2ebc to 85b2070 Compare May 14, 2024 18:42
Copy link
Contributor

github-actions bot commented May 14, 2024

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/9099454586/npm-package-wrangler-5819

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

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

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9099454586/npm-package-wrangler-5819 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9099454586/npm-package-create-cloudflare-5819 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9099454586/npm-package-cloudflare-kv-asset-handler-5819
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9099454586/npm-package-miniflare-5819
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9099454586/npm-package-cloudflare-pages-shared-5819
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9099454586/npm-package-cloudflare-vitest-pool-workers-5819

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


wrangler@3.56.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240512.0
workerd 1.20240512.0 1.20240512.0
workerd --version 1.20240512.0 2024-05-12

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

@CarmenPopoviciu CarmenPopoviciu force-pushed the carmen/fn-upload-failure branch 4 times, most recently from 04a3438 to b82a877 Compare May 15, 2024 12:40
packages/wrangler/src/pages/deploy.tsx Outdated Show resolved Hide resolved
logger.log(
`✨ Deployment complete! However, we couldn't ascertain the final status of your deployment.\n\n` +
`⚡️ Visit your deployment at ${deploymentResponse.url}\n` +
`⚡️ Check the deployment logs on the Cloudflare dashboard: https://dash.cloudflare.com/${accountId}/pages/view/${projectName}/${deploymentResponse.id}`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are gonna be no logs here for DU users, they could see a green tick and red triangle but that's it. I probably wouldn't bother linking to dash

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, but given that we couldn't tell whether the deployment was successful or failed from within wrangler, I think there might be some value in linking to Dash. I changed the wording to Check the deployment details on ...

packages/wrangler/src/pages/deployments.tsx Outdated Show resolved Hide resolved
Carmen Popoviciu added 5 commits May 15, 2024 17:02
Today, if uploading a Pages Function, or deploying a Pages
project fails for whatever reason, there’s no feedback shown
to the user. Worse yet, the shown message is misleading,
saying the deployment was successful, when in fact it was not:

```
✨ Deployment complete!
```

This commit ensures that we provide users with:

- the correct feedback with respect to their Pages deployment
- the appropriate messaging depending on the status of their
project's deployment status
- the appropriate logs in case of a deployment failure
@CarmenPopoviciu
Copy link
Contributor Author

thanks everyone for reviewing this PR and providing valuable feedback 🙏

@CarmenPopoviciu CarmenPopoviciu merged commit 63f7acb into main May 15, 2024
19 checks passed
@CarmenPopoviciu CarmenPopoviciu deleted the carmen/fn-upload-failure branch May 15, 2024 17:01
@wouterds
Copy link

wouterds commented May 15, 2024

Bless you, finally I get a clear error message what's going wrong with my deploy 😌

Screenshot 2024-05-15 at 19 56 26

Edit: this PR gets an honorable mention

@CarmenPopoviciu
Copy link
Contributor Author

whoohooo! much awesomeness! so so happy this PR helps!

@mvitorino
Copy link

Shouldn't this provide a non-zero return code so ci/cd and other scripts would fail?
I get this:

npm run deploy  && echo "Deployment was successful" || echo "Deployment failed"

> XXXX@0.1.0 deploy
> wrangler pages deploy ./build/client

▲ [WARNING] Warning: Your working directory is a git repo and has uncommitted changes

  To silence this warning, pass in --commit-dirty=true

✨ Compiled Worker successfully
🌍  Uploading... (40/40)

✨ Success! Uploaded 0 files (40 already uploaded) (0.54 sec)

✨ Uploading Functions bundle
🌎 Deploying...
✘ [ERROR] Failed to publish your Function. Got error: Your R2 bucket does not exist (workers.api.error.bucket_not_found)

❌ Deployment failed!
🪵  Logs were written to "/Users/.../Library/Preferences/.wrangler/logs/wrangler-2024-05-17_15-15-33_047.log"
Deployment was successful

@CarmenPopoviciu
Copy link
Contributor Author

CarmenPopoviciu commented May 17, 2024

ugh...not sure how I missed that 😅. Fix PR is here: #5862

Thx so much for catching that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

🐛 BUG: wrangler logs success for pages deploy even with Functions failures
6 participants