Miniflare config followups - #15130
Conversation
🦋 Changeset detectedLatest commit: 1cfafe6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 |
|
✅ All changesets look good |
|
No deploy-time reliance on workflow
I've reviewed both commits thoroughly. The changes are consistent, all references are cleaned up, downstream code degrades gracefully, and the tests properly cover the new warning behavior. No logic bugs, security issues, backward-compat violations, or incorrect API behavior found. LGTM |
deb7e00 to
a990bc3
Compare
a990bc3 to
ad7762d
Compare
ad7762d to
1cfafe6
Compare
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (normalizedConfig.workflows?.length) { | ||
| normalizedConfig.workflows = normalizedConfig.workflows.map( | ||
| ({ remote: _, ...binding }) => binding | ||
| ); | ||
| } |
There was a problem hiding this comment.
🟡 Leftover remote setting on a Workflow binding makes deploy report a fake configuration change
The step that hides the Workflow binding's remote setting before comparing local and deployed configuration was deleted (removeRemoteConfigFieldFromBindings in packages/deploy-helpers/src/deploy/helpers/config-diffs.ts:186-190), so any project that still has that setting in its config file is told its deployed configuration differs when it does not.
Impact: Users upgrading with an old remote entry on a workflow see a bogus, non-destructive-looking config difference (and possibly an extra confirmation prompt) during deploy.
Why the stale field survives validation and reaches the diff
validateWorkflowBinding in packages/workers-utils/src/config/validation.ts:2987-2995 no longer lists remote as an allowed key, but validateAdditionalProperties only emits a warning — it does not delete the key, and normalizeAndValidateConfig passes the raw workflow objects through untouched (packages/workers-utils/src/config/validation.ts:1700-1712). The resolved local config therefore still contains remote: true|false on each workflow entry. In normalizeRemoteConfigAsResolvedLocal the API-returned workflows array overrides the local one wholesale, and the API never returns remote, so diffJsonObjects reports a difference. The test at packages/wrangler/src/__tests__/deploy/get-remote-config-diff.test.ts:507-513 had to drop remote: false for the "no diff" expectation to keep passing, which confirms the new behaviour.
Prompt for agents
Removing `remote` from the Workflow binding type also removed the normalization step in packages/deploy-helpers/src/deploy/helpers/config-diffs.ts that stripped `remote` from `normalizedConfig.workflows` before diffing local against remote config. Because config validation only warns about unexpected fields (validateAdditionalProperties in packages/workers-utils/src/config/validation.ts) and does not remove them, existing user configs that still contain `remote` on workflow bindings keep that key in the resolved local config, producing a spurious diff during `wrangler deploy`. Consider either keeping the stripping step for backwards compatibility, or making config normalization actually drop the deprecated `remote` key from workflow bindings.
Was this helpful? React with 👍 or 👎 to provide feedback.
| --- | ||
| "@cloudflare/config": patch | ||
| --- |
There was a problem hiding this comment.
🟡 Changeset for the new R2 local credentials option is classified as a bug fix instead of a feature
The changeset adding the new localDev.experimentalS3Credentials configuration option to @cloudflare/config is declared as patch, but repository changeset rules classify new configuration options / new API capabilities as minor.
Impact: The package will be released with a version bump that understates the added functionality.
Rule reference
REVIEW.md — "Minor (new features): ... new commands/flags/options, new API capabilities or exports, behavior changes that add functionality." .changeset/README.md also lists "new configuration options" and "changes to pre-1.0 features" as minor. The change adds a brand-new localDev.experimentalS3Credentials field to the shared R2 binding shape (packages/config/src/bindings.ts:380-387, packages/config/src/schema.ts:59-69).
| --- | |
| "@cloudflare/config": patch | |
| --- | |
| --- | |
| "@cloudflare/config": minor | |
| --- |
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Each commit contains a fixup
namerequried for AE binding - workerd requires this, but it is not required at deploy time i think? either way, to prevent workerd crashing, we set a default.A picture of a cute animal (not mandatory, but encouraged)