Skip to content

validate container configuration in wrangler #9774

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Aug 5, 2025
Merged

validate container configuration in wrangler #9774

merged 14 commits into from
Aug 5, 2025

Conversation

nikitassharma
Copy link
Member

@nikitassharma nikitassharma commented Jun 26, 2025 •
edited
Loading

Fixes CC-5513

Validate instance type against account limits to give early feedback to the user.

  • Tests
    • Tests included
    • Tests not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: improved error handling
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: not a v3 feature

@nikitassharma nikitassharma requested a review from a team as a code owner June 26, 2025 21:34
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Jun 26, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: 3828bee

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

This PR includes changesets to release 3 packages
Name Type
wrangler Patch
@cloudflare/vite-plugin Patch
@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

petebacondarwin
petebacondarwin approved these changes Jul 2, 2025
View reviewed changes
Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

One small nit. But this looks good.

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jul 2, 2025
@pkg-pr-new pkg.pr.new
Copy link

pkg-pr-new bot commented Jul 2, 2025 •
edited
Loading

create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@9774

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@9774

miniflare

npm i https://pkg.pr.new/miniflare@9774

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@9774

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@9774

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@9774

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@9774

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@9774

wrangler

npm i https://pkg.pr.new/wrangler@9774

commit: 3828bee

@petebacondarwin petebacondarwin added the skip-v3-pr Skip validation of presence of a v3 backport PR label Jul 2, 2025
emily-shen
emily-shen reviewed Jul 3, 2025
View reviewed changes
Copy link
Contributor

@emily-shen emily-shen left a comment

Choose a reason for hiding this comment

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

i noticed that the instance type is not being used in ensureDiskLimits when building and validating a container, is that intentional? i notice this is mostly targeting cloudchamber-only commands

@nikitassharma nikitassharma requested a review from a team as a code owner July 7, 2025 19:05
@nikitassharma
Copy link
Member Author

nikitassharma commented Jul 7, 2025 •
edited
Loading

i noticed that the instance type is not being used in ensureDiskLimits when building and validating a container, is that intentional?

@emily-shen thanks for the catch! I updated it so that ensureDiskLimits goes to the instance type disk size first before falling back on defaults

i notice this is mostly targeting cloudchamber-only commands

the changes to apply will be used by wrangler deploy

gpanders
gpanders reviewed Jul 7, 2025
View reviewed changes
gpanders
gpanders reviewed Jul 7, 2025
View reviewed changes
gpanders
gpanders reviewed Jul 7, 2025
View reviewed changes
emily-shen
emily-shen reviewed Jul 8, 2025
View reviewed changes
gpanders
gpanders reviewed Jul 8, 2025
View reviewed changes
@nikitassharma nikitassharma force-pushed the nsharma/CC-5513 branch 2 times, most recently from 2550bc0 to 2a45f1f Compare July 23, 2025 17:51
@nikitassharma nikitassharma changed the title CC-5513 Validate instance type in wrangler Validate instance type in wrangler Jul 23, 2025
@nikitassharma nikitassharma requested a review from a team July 23, 2025 18:03
@nikitassharma nikitassharma changed the base branch from main to nsharma/custom-instance-type July 28, 2025 21:08
emily-shen
emily-shen reviewed Jul 29, 2025
View reviewed changes
Copy link
Contributor

@emily-shen emily-shen left a comment •
edited
Loading

Choose a reason for hiding this comment

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

for containers (as opposed to the cloudchamber only commands) could we move this check to cloudchamber/build.ts in buildAndMaybePush? this way containers build -p also gets this. Also, the existing ensureDiskLimits seems to overlap with this, so it would be good to consolidate everything.

@nikitassharma nikitassharma deleted the branch cloudflare:main July 30, 2025 15:12
@github-project-automation github-project-automation bot moved this from Untriaged to Done in workers-sdk Jul 30, 2025
@nikitassharma nikitassharma reopened this Jul 30, 2025
@github-project-automation github-project-automation bot moved this from Done to Untriaged in workers-sdk Jul 30, 2025
@nikitassharma nikitassharma changed the base branch from nsharma/custom-instance-type to main July 30, 2025 19:03
nikitassharma and others added 3 commits July 30, 2025 14:16
@nikitassharma
validate instance type in containers/deploy.ts
0f533ea
@nikitassharma
clean up
b57d64c
@nikitassharma @emily-shen
Update .changeset/twenty-seas-cross.md
10af50a 
Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>
@emily-shen emily-shen marked this pull request as draft July 31, 2025 09:38
@nikitassharma nikitassharma marked this pull request as ready for review July 31, 2025 20:16
@nikitassharma
Copy link
Member Author

Refactored the PR to update functionality. Here's a summary of the goal of these changes:

  1. Maintains changes from the original PR to refactor instance type processing into instance-type/instance-type.ts
  2. As a change from the original PR, updates validation against account limits to work for both instance types and custom limits. This is done in ensureContainerLimits
  3. As a change from the original PR, we are no longer validating the configuration against account limits for cloudchamber only commands
  4. Consolidates ensureDiskLimits with the other limit checks. A large amount of the functionality here was moved into ensureContainerLimits and the remainder was moved to a new function ensureImageFitsLimits

@nikitassharma nikitassharma changed the title Validate instance type in wrangler validate container configuration in wrangler Aug 1, 2025
@github-project-automation github-project-automation bot moved this from Untriaged to Approved in workers-sdk Aug 1, 2025
emily-shen
emily-shen approved these changes Aug 4, 2025
View reviewed changes
@github-actions github-actions bot mentioned this pull request Aug 4, 2025
Closed
@emily-shen emily-shen mentioned this pull request Aug 4, 2025
Closed
nikitassharma and others added 5 commits August 4, 2025 10:10
@nikitassharma @emily-shen
Update packages/wrangler/src/cloudchamber/limits.ts
2e95c38 
Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>
@nikitassharma @emily-shen
Apply suggestions from code review
083158f 
Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com>
@nikitassharma
update comment
a173926
@nikitassharma
Merge branch 'main' into nsharma/CC-5513
e4ecaa5
@nikitassharma
fix tests
3828bee
@emily-shen emily-shen merged commit 48853a6 into cloudflare:main Aug 5, 2025
68 of 73 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in workers-sdk Aug 5, 2025
@workers-devprod workers-devprod mentioned this pull request Aug 5, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emily-shen emily-shen emily-shen approved these changes

@gpanders gpanders gpanders approved these changes

@ichernetsky-cf ichernetsky-cf ichernetsky-cf approved these changes

@petebacondarwin petebacondarwin Awaiting requested review from petebacondarwin

Assignees
No one assigned
Labels
skip-v3-pr Skip validation of presence of a v3 backport PR
Projects
workers-sdk
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nikitassharma @petebacondarwin @gpanders @emily-shen @ichernetsky-cf