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 max asset count message #6627

Merged
merged 2 commits into from
Sep 6, 2024
Merged

Fix max asset count message #6627

merged 2 commits into from
Sep 6, 2024

Conversation

GregBrimble
Copy link
Contributor

What this PR solves / how to test

Fixes reporting back just the 20k limit again.

image

Author has addressed the following

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

@GregBrimble GregBrimble requested a review from a team as a code owner September 4, 2024 01:26
Copy link

changeset-bot bot commented Sep 4, 2024

🦋 Changeset detected

Latest commit: 6215151

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

This PR includes changesets to release 4 packages
Name Type
miniflare Patch
@cloudflare/pages-shared Patch
@cloudflare/vitest-pool-workers Patch
wrangler 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

@GregBrimble GregBrimble force-pushed the fix-max-asset-count-message branch from 54db19b to 89d5bf8 Compare September 4, 2024 01:27
Copy link
Contributor

github-actions bot commented Sep 4, 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/10724002843/npm-package-wrangler-6627

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

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

Or you can use npx with this latest build directly:

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

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.20240821.1
workerd 1.20240821.1 1.20240821.1
workerd --version 1.20240821.1 2024-08-21

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

@emily-shen
Copy link
Contributor

emily-shen commented Sep 4, 2024

Fixes reporting back just the 20k limit again.

I'm a bit confused - doesn't seem like any of the changed code is affecting the 20k limit constant? Was the error not surfacing properly from inside the promise loop? D:

Does the same change also need to be done to this file? (yes I really need to move the duplicated code to a shared file somewhere 🥲 )

if (counter >= MAX_ASSET_COUNT) {

@petebacondarwin
Copy link
Contributor

Fixes reporting back just the 20k limit again.

I'm a bit confused - doesn't seem like any of the changed code is affecting the 20k limit constant? Was the error not surfacing properly from inside the promise loop? D:

Does the same change also need to be done to this file? (yes I really need to move the duplicated code to a shared file somewhere 🥲 )

if (counter >= MAX_ASSET_COUNT) {

I think the point is that if we throw the error as we find the 20,000th file then we will always say that we found 20,000 files.
Whereas if we wait until the loop is complete, it will report the actual number of files found.

@GregBrimble GregBrimble merged commit 5936282 into main Sep 6, 2024
21 checks passed
@GregBrimble GregBrimble deleted the fix-max-asset-count-message branch September 6, 2024 18:09
@workers-devprod workers-devprod mentioned this pull request Sep 6, 2024
penalosa pushed a commit that referenced this pull request Sep 9, 2024
* Fixes asset count error message to properly report count of assets

* Error if > 20000 assets rather than >= 20000

Co-authored-by: Pete Bacon Darwin <[email protected]>

---------

Co-authored-by: Pete Bacon Darwin <[email protected]>
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.

4 participants