refactor: resolve function regions during build phase to fix VPC connectors#10471
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the region resolution logic for Cloud Functions, shifting the resolution process from the Backend representation to the Build representation. This allows for correct VPC connector string construction earlier in the deployment process. The new resolveDefaultRegionsForBuild function integrates existing region matching and trigger-based resolution. Review feedback identifies a violation of the style guide regarding the use of the any type and suggests including the endpoint ID in resolver calls to avoid potential issues. Additionally, a suggestion was made to use falsy checks for array length in accordance with project conventions.
wandamora
changed the title
|
Might want to rebase this PR to take in Joe's changes for the test cases failing. |
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
![wiz-9635d3485b[bot]](https://avatars.githubusercontent.com/u/1335026?s=60&v=4){kind=small}
wandamora
force-pushed
the
ajp-speculative-tbd-fix-yolo
branch
from
c9bded9 to
53f8bc3
Compare
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
My AI review prompt had a few more good suggestions we can consider for a follow up PR.
| } else { | ||
| // Match triggers. | ||
| try { | ||
| const fullEndpoint = { ...endpoint, id } as any; |
There was a problem hiding this comment.
This is done because endpoint (of type build.Endpoint) does not have an id field, but resolveRegionForEventTrigger expects a backend.Endpoint which requires an id. While this works, it bypasses TypeScript's type checks. Consider defining a minimal interface for what resolveRegionForEventTrigger actually needs (it seems to only need project and eventTrigger) or casting to a specific type that fulfills that contract rather than any.
| if (build.isBlockingTriggered(endpoint)) { | ||
| resolvedRegion = resolveRegionForBlockingTrigger(fullEndpoint); | ||
| } else if (build.isEventTriggered(endpoint)) { | ||
| resolvedRegion = await resolveRegionForEventTrigger(fullEndpoint); |
There was a problem hiding this comment.
If a user has many event-triggered functions, this will execute network calls (like fetching database or bucket locations) sequentially (because of the for loop it's in), which can slow down the prepare phase. Consider gathering the promises and using Promise.all to resolve them in parallel.
There was a problem hiding this comment.
Daniel once advised against using Promise.all() since transient errors could bubble up in the CLI in a way that looks cryptic to the end user.
There was a problem hiding this comment.
Ok. That's a good counter argument. I think it'd be good for us to see what the cryptic error is and if we could fix it so things can be faster. But I guess let's save that for a day when we need to improve performance.
| expect(want.endpoints["id"].region).to.deep.equal(["us-east1"]); | ||
| }); | ||
|
|
||
| it("resolves region to us-east1 and correctly formats VPC connector path with us-east1", async () => { |
There was a problem hiding this comment.
AI suggests also testing this case:
Pre-formatted VPC Connector Paths:
Scenario: An endpoint with a vpc.connector that is already a full resource path (e.g., projects/my-project/locations/us-east1/connectors/my-connector).
Purpose: Verify that the resolution logic does not attempt to prefix it again or corrupt it, even after the region is resolved. This ensures the check connector.includes("/") in build.ts works correctly in this flow.
4 participants
This moves the resolution logic to be before we resolve the build to a backend to set the defaults in functions, and the correct region can be known when building the VPC connector identfier.
Scenarios Tested
Create function with VPC connector
firebase deploy --only functions on 15.16.0Update function and use local firebase-tools build
firebase deploy --only functionsUpdate was successful.