Cloud Run changes to support callable functions#10168
Cloud Run changes to support callable functions#10168brittanycho wants to merge 2 commits intomainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the deployment process to fully support callable functions on Cloud Run. It establishes a critical mechanism to reserve unique URLs for these functions by creating a temporary placeholder Google Cloud Functions v2 function. This ensures that the necessary endpoint infrastructure is in place before the primary Cloud Run service is deployed, streamlining the overall deployment workflow for callable functions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces the functionality to reserve callable URLs for Cloud Run functions. This is achieved by adding a new reserveCallableUrl method in fabricator.ts which creates a placeholder GCFv2 function using a 'hello world' zip from a GCS bucket. The implementation includes logic to ignore ALREADY_EXISTS errors during this process. Corresponding unit tests have been added in fabricator.spec.ts to cover the new createEndpoint behavior for Cloud Run and the reserveCallableUrl method's functionality, including error handling. Review feedback suggests improving the robustness of GCS URL parsing, adhering to the style guide by avoiding any for error types and type assertions, and updating test cases to use FirebaseError for better type safety and consistency.
| apiFunction.buildConfig.source = { | ||
| storageSource: { | ||
| bucket: HELLO_WORLD_GCS_URL.split("/")[2], | ||
| object: HELLO_WORLD_GCS_URL.split("/").slice(3).join("/"), | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Parsing the GCS URL by splitting the string is a bit fragile. If the URL format changes (e.g., more path segments), this could break.
Using a regular expression would be more robust for extracting the bucket and object path.
const gcsUrlMatch = HELLO_WORLD_GCS_URL.match(/gs:\/\/([^\/]+)\/(.*)/);
if (!gcsUrlMatch) {
// This should not happen if the constant is well-formed.
throw new FirebaseError(`Invalid GCS URL for hello world source: ${HELLO_WORLD_GCS_URL}`);
}
const [, bucket, object] = gcsUrlMatch;
apiFunction.buildConfig.source = {
storageSource: {
bucket,
object,
},
};| } catch (err: any) { | ||
| if (err.status === 409) { | ||
| // Already exists, which is fine. | ||
| return; | ||
| } | ||
| throw err; | ||
| } |
There was a problem hiding this comment.
The repository style guide states: 'Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards.' Using err: any violates this rule.
You should use err: unknown and then check its type. Since gcfV2.createFunction is expected to throw a FirebaseError, you can check for that specifically.
| } catch (err: any) { | |
| if (err.status === 409) { | |
| // Already exists, which is fine. | |
| return; | |
| } | |
| throw err; | |
| } | |
| } catch (err: unknown) { | |
| if (err instanceof FirebaseError && err.status === 409) { | |
| // Already exists, which is fine. | |
| return; | |
| } | |
| throw err; | |
| } |
| throw err; | ||
| } | ||
| }) | ||
| .catch(rethrowAs(endpoint, "reserve URL" as any)); |
There was a problem hiding this comment.
Using as any here bypasses a type-check because 'reserve URL' is not a valid OperationType.
To fix this properly, you should add 'reserve URL' to the OperationType union in src/deploy/functions/release/reporter.ts. This will improve type safety and remove the need for as any.
After updating OperationType, you can remove as any from this line.
| const err = new Error("Already exists"); | ||
| (err as any).status = 409; | ||
| gcfv2.createFunction.rejects(err); |
There was a problem hiding this comment.
To align with the suggested improvement in fabricator.ts to catch FirebaseError specifically, this test should be updated to reject with a FirebaseError instance. This makes the test more realistic and ensures it remains valid after the production code is updated.
Also, using (err as any) is a violation of the style guide. Creating a FirebaseError is cleaner.
You'll need to import FirebaseError from ../../../error.
| const err = new Error("Already exists"); | |
| (err as any).status = 409; | |
| gcfv2.createFunction.rejects(err); | |
| const err = new FirebaseError("Already exists", { status: 409 }); | |
| gcfv2.createFunction.rejects(err); |
| const err = new Error("Server Error"); | ||
| (err as any).status = 500; | ||
| gcfv2.createFunction.rejects(err); |
There was a problem hiding this comment.
Similar to the other test case, this should be updated to reject with a FirebaseError instance to align with the suggested improvements in fabricator.ts and adhere to the style guide.
| const err = new Error("Server Error"); | |
| (err as any).status = 500; | |
| gcfv2.createFunction.rejects(err); | |
| const err = new FirebaseError("Server Error", { status: 500 }); | |
| gcfv2.createFunction.rejects(err); |
2 participants
Cloud Run changes to support callable functions