-
Notifications
You must be signed in to change notification settings - Fork 1k
add more validation to containers config #9934
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
Conversation
🦋 Changeset detectedLatest commit: ddbc4dc The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
bfc02e6 to
006ff7d
Compare
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
dario-piotrowicz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🙂
96057a1 to
d865917
Compare
d865917 to
c70513e
Compare
IRCody
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @nikitassharma's suggestion but other than that LGTM.
c70513e to
fd70f4f
Compare
fd70f4f to
ddbc4dc
Compare
| diagnostics, | ||
| `${field}.configuration`, | ||
| Object.keys(containerAppOptional.configuration), | ||
| ["image", "secrets", "labels", "disk", "vcpu", "memory_mib"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think vcpu and memory_mib are included in configuration?
workers-sdk/packages/wrangler/src/config/environment.ts
Lines 109 to 114 in 64af5da
| configuration?: { | |
| image?: string; | |
| labels?: { name: string; value: string }[]; | |
| secrets?: { name: string; type: "env"; secret: string }[]; | |
| disk?: { size: string }; | |
| }; |
Users shouldn't be able to set these directly in the Wrangler config anyway, they are set by the instance type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these fields are being used right now for customers that need custom instance types, see #9962 for better handling for custom instance types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do they need to be added to the type definition then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd strongly prefer to have them in the type definition so that we're not trying to read values that typescript doesn't expect to be there. they're hidden from the wrangler config schema, so we don't need to worry about showing this to the general user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, they should be in the type definition
Fixes CC-5480