Repository navigation
Support cross-account service bindings - #542
Draft
irvinebroque wants to merge 3 commits into
Draft
irvinebroque wants to merge 3 commits into
irvinebroque wants to merge 3 commits into
Conversation
commit: |
Contributor
|
| mean | stddev | min | max | |
|---|---|---|---|---|
| PR | 264.0 ms | 4.9 ms | 255.8 ms | 274.1 ms |
| base | 265.6 ms | 4.8 ms | 255.5 ms | 276.1 ms |
Δ (PR vs base): ↓ -1.6 ms (-0.6%)
Measured with hyperfine on ubuntu-latest (3 warmup runs, 20 timed runs). CI numbers carry ±a few ms of runner jitter; treat small deltas as noise.
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
Comment on lines
+28
to
+57
| await test("leaves other exotic dependencies blocked by the existing policy", () => { | ||
| for (const wanted of [ | ||
| { | ||
| alias: "@cloudflare/config", | ||
| bareSpecifier: "https://pkg.pr.new/@cloudflare/config@main", | ||
| }, | ||
| { | ||
| alias: "@cloudflare/config", | ||
| bareSpecifier: `https://example.com/@cloudflare/config@${commit}`, | ||
| }, | ||
| { | ||
| alias: "@cloudflare/config", | ||
| bareSpecifier: `https://pkg.pr.new/other/repo/@cloudflare/config@${commit.slice(0, 7)}`, | ||
| }, | ||
| { | ||
| alias: "unrelated", | ||
| bareSpecifier: `https://pkg.pr.new/unrelated@${commit}`, | ||
| }, | ||
| { | ||
| alias: "@cloudflare/config", | ||
| bareSpecifier: `https://pkg.pr.new/@cloudflare/config@${commit}?other=true`, | ||
| }, | ||
| ]) { | ||
| assert.equal(resolver.canResolve(wanted), false); | ||
| assert.throws( | ||
| () => resolver.resolve(wanted), | ||
| /Unapproved Workers SDK prerelease/ | ||
| ); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
pnpm invokes resolve() only after canResolve() succeeds, so the current assert.throws() branch is unreachable during an install. The test therefore does not prove that the existing exotic-subdependency policy blocks these sources and would still pass if that policy were removed. Limit this unit test to the resolver's actual rejection path; cover the pnpm policy with an install-level fixture if it needs regression coverage:
Suggested change
| await test("leaves other exotic dependencies blocked by the existing policy", () => { | |
| for (const wanted of [ | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: "https://pkg.pr.new/@cloudflare/config@main", | |
| }, | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: `https://example.com/@cloudflare/config@${commit}`, | |
| }, | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: `https://pkg.pr.new/other/repo/@cloudflare/config@${commit.slice(0, 7)}`, | |
| }, | |
| { | |
| alias: "unrelated", | |
| bareSpecifier: `https://pkg.pr.new/unrelated@${commit}`, | |
| }, | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: `https://pkg.pr.new/@cloudflare/config@${commit}?other=true`, | |
| }, | |
| ]) { | |
| assert.equal(resolver.canResolve(wanted), false); | |
| assert.throws( | |
| () => resolver.resolve(wanted), | |
| /Unapproved Workers SDK prerelease/ | |
| ); | |
| } | |
| }); | |
| await test("rejects unapproved Workers SDK prerelease sources", () => { | |
| for (const wanted of [ | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: "https://pkg.pr.new/@cloudflare/config@main", | |
| }, | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: `https://example.com/@cloudflare/config@${commit}`, | |
| }, | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: `https://pkg.pr.new/other/repo/@cloudflare/config@${commit.slice(0, 7)}`, | |
| }, | |
| { | |
| alias: "unrelated", | |
| bareSpecifier: `https://pkg.pr.new/unrelated@${commit}`, | |
| }, | |
| { | |
| alias: "@cloudflare/config", | |
| bareSpecifier: `https://pkg.pr.new/@cloudflare/config@${commit}?other=true`, | |
| }, | |
| ]) { | |
| assert.equal(resolver.canResolve(wanted), false); | |
| } | |
| }); |
Contributor
|
🤖 I've done a thorough review of your PR. Pins Workers SDK prereleases to preserve cross-account service bindings.
|
Contributor
|
LGTM! |
Contributor
|
LGTM! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Support cross-account service bindings in
cloudflare.config.tsand Build Output by consuming the shared SDK changes in cloudflare/workers-sdk#16764. Preserve account selection through upload, migration, and remote development.Pin one SDK commit for premerge testing, keep the existing container patches, and restrict prerelease resolution to those exact packages and commit. Replace the pins with SDK releases before the stable release.
Validation: frozen install,
pnpm check, and 35 focused tests pass.