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

🚀 Feature Request: Expand validateAdditionalProperties to more configuration types #2586

Open
KianNH opened this issue Jan 23, 2023 · 3 comments
Assignees
Labels
caretaking Priority for caretaking enhancement New feature or request

Comments

@KianNH
Copy link
Contributor

KianNH commented Jan 23, 2023

Describe the solution

The current usage of validateAdditionalProperties is only used for a subset of configuration fields, such as build or the top-level entries.

This means that you can use the following TOML and whilst it will publish with no warnings, route won't work and nor will you get warnings about that or the unknown i_dont_exist field.

name = "test-worker"
main = "src/index.ts"
compatibility_date = "2023-01-11"

[[kv_namespaces]]
binding = "KV"
id = ""
preview_id = ""

route = { pattern = "example.com/*", zone_name = "example.com" }

[[r2_buckets]]
binding = "R2"
bucket_name = ""
preview_bucket_name = ""

i_dont_exist = "foo"

Relevant file: https://github.com/cloudflare/wrangler2/blob/main/packages/wrangler/src/config/validation.ts

In my opinion, I don't see the downside to running it on everything other than unsafe bindings?

@KianNH KianNH added the enhancement New feature or request label Jan 23, 2023
@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Jan 23, 2023
@mrbbot
Copy link
Contributor

mrbbot commented Jan 23, 2023

Hey! 👋 This seems like a good idea, especially given TOML format confusions. 👍

@mrbbot mrbbot moved this from Untriaged to Selected for Development in workers-sdk Jan 23, 2023
@lrapoport-cf lrapoport-cf moved this from Selected for Development to Backlog in workers-sdk Feb 6, 2023
@petebacondarwin petebacondarwin self-assigned this Jul 10, 2024
@jahands
Copy link
Contributor

jahands commented Sep 23, 2024

I ran into this today - turns out wrangler environments don't show the specific key that's invalid, making it harder to spot/figure out what was wrong:

▲ [WARNING] Processing wrangler.toml configuration:
- "env.staging" environment configuration

@CarmenPopoviciu
Copy link
Contributor

CarmenPopoviciu commented Sep 24, 2024

as part of this fix we should also ensure that additional invalid fields defined at environment level will result in a warning as well. For example, top level additional fields currently do result in a warning, whereas environment-level ones don't:

[invalid_field]
name = "some name"

[env.staging.invalid_field]
name = "some staging name"
Screenshot 2024-09-24 at 15 50 56

We should ensure that warning behaviour is on par across top-level/env config

@github-project-automation github-project-automation bot moved this to Untriaged in workers-sdk Sep 24, 2024
@lrapoport-cf lrapoport-cf moved this from Untriaged to Backlog in workers-sdk Sep 24, 2024
@lrapoport-cf lrapoport-cf added the caretaking Priority for caretaking label Sep 24, 2024
@CarmenPopoviciu CarmenPopoviciu moved this from Backlog to In Progress in workers-sdk Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
caretaking Priority for caretaking enhancement New feature or request
Projects
Status: In Progress
Development

No branches or pull requests

6 participants