Cloud Run No Build Initial MVP Implementation v2 #9706
Conversation
Summary of ChangesHello @brittanycho, 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 lays the groundwork for deploying functions to Cloud Run using a 'no-build' approach. It extends the existing functions deployment framework to support direct deployment of container images or source code archives, bypassing the traditional build process. Key changes include new configuration options for Cloud Run services, integration with the Cloud Run V2 API for lifecycle management, and the introduction of a dedicated runtime delegate for discovering and deploying these new function types. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. 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 a 'no-build' deployment option for Cloud Run, which seems to be the initial MVP targeting Dart. The changes are comprehensive, touching upon type definitions, source packaging to support .tar.gz, and new deployment logic using the Cloud Run v2 API. The overall implementation is solid, but I've identified several areas for improvement regarding code comments, code duplication, and minor code style issues. My review comments provide specific suggestions to enhance maintainability and clarity.
| export type MemoryOption = 128 | 256 | 512 | 1024 | 2048 | 4096 | 8192 | 16384 | 32768; | ||
| const allMemoryOptions: MemoryOption[] = [128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768]; | ||
|
|
||
| // Run is an automatic migration from gcfv2 and is not used on the wire. |
There was a problem hiding this comment.
This comment appears to be outdated. With the changes in this pull request, the 'run' platform is now being used. Please update or remove this comment to avoid confusion.
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
This JSDoc comment is empty. Please add a meaningful description for the createReadStream function or remove the comment block if it's not needed.
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
This JSDoc comment is empty. Please add a meaningful description for the uploadSourceV2 function or remove the comment block if it's not needed.
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
This JSDoc comment is empty. Please add a meaningful description for the getFunctionsConfig function or remove the comment block.
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
This JSDoc comment is empty. Please add a meaningful description for the prepareFunctionsUpload function or remove the comment block.
| /** | ||
| * | ||
| */ |
There was a problem hiding this comment.
This JSDoc comment is empty. Please add a meaningful description for the convertToSortedKeyValueArray function or remove the comment block.
| async createRunService(endpoint: backend.Endpoint): Promise<void> { | ||
| const storageSource = this.sources[endpoint.codebase!]?.storage; | ||
| if (!storageSource) { | ||
| logger.debug("Precondition failed. Cannot create a Cloud Run function without storage"); | ||
| throw new Error("Precondition failed"); | ||
| } | ||
| const service: Omit<runV2.Service, runV2.ServiceOutputFields> = { | ||
| name: `projects/${endpoint.project}/locations/${endpoint.region}/services/${endpoint.id}`, | ||
| template: { | ||
| containers: [ | ||
| { | ||
| name: "worker", | ||
| image: "scratch", | ||
| command: endpoint.command, | ||
| args: endpoint.args, | ||
| baseImageUri: endpoint.baseImageUri, | ||
| sourceCode: { | ||
| cloudStorageSource: { | ||
| bucket: storageSource.bucket, | ||
| object: storageSource.object, | ||
| generation: storageSource.generation ? String(storageSource.generation) : undefined, | ||
| }, | ||
| }, | ||
| resources: { | ||
| limits: { | ||
| cpu: String(endpoint.cpu || 1), | ||
| memory: `${endpoint.availableMemoryMb || 256}Mi`, | ||
| }, | ||
| cpuIdle: true, | ||
| startupCpuBoost: true, | ||
| }, | ||
| }, | ||
| ], | ||
| maxInstanceRequestConcurrency: endpoint.concurrency || 80, | ||
| scaling: { | ||
| minInstanceCount: endpoint.minInstances || 0, | ||
| maxInstanceCount: endpoint.maxInstances || 100, | ||
| }, | ||
| }, | ||
| client: "cli-firebase", | ||
| labels: { ...endpoint.labels, "goog-managed-by": "firebase-functions" }, | ||
| annotations: {}, | ||
| }; | ||
|
|
||
| await this.executor | ||
| .run(async () => { | ||
| const op = await runV2.createService( | ||
| endpoint.project, | ||
| endpoint.region, | ||
| endpoint.id, | ||
| service, | ||
| ); | ||
| endpoint.uri = op.uri; | ||
| endpoint.runServiceId = endpoint.id; | ||
| }) | ||
| .catch(rethrowAs(endpoint, "create")); | ||
|
|
||
| await this.setInvoker(endpoint); | ||
| } | ||
|
|
||
| async updateRunService(update: planner.EndpointUpdate): Promise<void> { |
There was a problem hiding this comment.
The logic for creating the service object inside createRunService is nearly identical to the logic inside updateRunService. To reduce code duplication and improve maintainability, consider extracting this logic into a private helper method. This helper could take an endpoint and storage source, and return the runV2.Service object. Both createRunService and updateRunService could then call this helper.
| // If runtime is specified, use it. Otherwise default to "dart3". | ||
| // "dart" is often used as a generic alias, map it to "dart3" | ||
| let runtime = context.runtime || "dart3"; | ||
| if ((runtime as string) === "dart") { |
There was a problem hiding this comment.
The type cast (runtime as string) is redundant here, as runtime is guaranteed to be a string at this point. You can simplify this to if (runtime === "dart").
| if ((runtime as string) === "dart") { | |
| if (runtime === "dart") { |
1 participant
Cloud Run No Build Initial MVP Implementation v2