Prototype Tool to Create Cloud Run Apps with Firebase CLI - #10898
Prototype Tool to Create Cloud Run Apps with Firebase CLI#10898falahat wants to merge 10 commits into
Conversation
…veiwed by humans and brought up to bar. Testing: This was tested manually by deploying a Cloud Run app
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
![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 support for configuring and deploying Cloud Run services via the Firebase CLI, including integration with App Hosting configurations, initialization features, and end-to-end tests. The feedback highlights several critical issues: hardcoded absolute paths in the E2E test script, a bug in constructing the secret resource path for Cloud Run environment variables, and a violation of best practices regarding revision-level versus service-level scaling. Additionally, the modification to updateService in src/gcp/runv2.ts is flagged as highly risky for existing Cloud Functions v2 deployments. Finally, the reviewer recommends removing an accidentally committed backup file (deploy.ts.bak), adding validation for serviceId, and adhering to the repository style guide by throwing FirebaseError instead of generic Error objects.
| if (!projectId) { | ||
| throw new Error("Project ID must be set before initializing Cloud Run."); | ||
| } |
There was a problem hiding this comment.
Throwing a generic Error for expected, user-facing errors violates the repository style guide. Please throw a FirebaseError instead.
| if (!projectId) { | |
| throw new Error("Project ID must be set before initializing Cloud Run."); | |
| } | |
| const projectId = setup.projectId; | |
| if (!projectId) { | |
| throw new FirebaseError("Project ID must be set before initializing Cloud Run."); | |
| } |
Do Not read apphosting.local.yaml by accident Add timeout to artifact registry actions Track Cloud Build operations/results better
* Deduplicated Test CLI Process Wrapper * Standardized Secret Name Parsing * Standardized GCP API Verification * Resolved RunConfig Type Naming Collisions * Materialized Target Configuration & Target Filtering * Updated firebase.json schema to include the "run" section * Cleaned Init Feature Scaffolding * Gated ABIU Base Image Updates
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for deploying Cloud Run services via the Firebase CLI, adding configuration schemas, initialization prompts, deployment lifecycle hooks, and an E2E test suite. The code reviewer provided valuable feedback focused on aligning the implementation with repository style guides. Key recommendations include replacing manual polling loops with the central pollOperation utility, avoiding the use of the :latest tag for container images to prevent deployment overwrites, eliminating as any type casts by properly typing options and configurations, and ensuring FirebaseError throws specify non-zero exit codes for precondition violations.
| const op = res.body.buildOperation; | ||
| const opName = typeof op === "string" ? op : op?.name || ""; | ||
| const rawId = | ||
| opName | ||
| .split("/") | ||
| .pop() | ||
| ?.replace(/^build-/, "") || ""; | ||
| const buildId = (op as any)?.metadata?.build?.id || rawId; | ||
| if (buildId) { | ||
| const cloudbuildClient = new Client({ | ||
| urlPrefix: cloudbuildOrigin(), | ||
| auth: true, | ||
| apiVersion: "v1", | ||
| }); | ||
| const startTime = Date.now(); | ||
| const timeoutMs = 15 * 60 * 1000; | ||
| let buildSuccess = false; | ||
| while (Date.now() - startTime < timeoutMs) { | ||
| try { | ||
| const buildStatusRes = await cloudbuildClient.get<{ | ||
| status: string; | ||
| statusDetail?: string; | ||
| }>(`/projects/${projectId}/locations/${location}/builds/${buildId}`); | ||
| const status = buildStatusRes.body?.status; | ||
| if (status === "SUCCESS") { | ||
| logger.info(`[run:submitBuild] Cloud Build ${buildId} completed with SUCCESS.`); | ||
| buildSuccess = true; | ||
| break; | ||
| } | ||
| if ( | ||
| status === "FAILURE" || | ||
| status === "INTERNAL_ERROR" || | ||
| status === "TIMEOUT" || | ||
| status === "CANCELLED" | ||
| ) { | ||
| throw new FirebaseError( | ||
| `Cloud Build failed with status ${status}: ${buildStatusRes.body?.statusDetail || ""}`, | ||
| ); | ||
| } | ||
| } catch (err: any) { | ||
| if (err instanceof FirebaseError && err.message.startsWith("Cloud Build failed")) { | ||
| throw err; | ||
| } | ||
| if (err.status && err.status >= 400 && err.status < 500) { | ||
| throw err; | ||
| } | ||
| logger.debug(`[run:submitBuild] Polling retry on transient error: ${err.message}`); | ||
| } | ||
| await new Promise((resolve) => setTimeout(resolve, 3000)); | ||
| } | ||
| if (!buildSuccess) { | ||
| throw new FirebaseError(`Cloud Build ${buildId} timed out after 15 minutes.`, { exit: 1 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of implementing a manual polling loop with setTimeout to wait for the Cloud Build to complete, we should leverage the repository's central pollOperation utility. Cloud Build's build submission returns a standard Long Running Operation (LRO) that can be polled directly. This reduces code complexity, improves robustness (by using standard backoff and error handling), and adheres to the repository style guide rule to look for existing utilities first.
const op = res.body.buildOperation;
const opName = typeof op === "string" ? op : op?.name;
if (opName) {
await pollOperation({
apiOrigin: cloudbuildOrigin(),
apiVersion: "v1",
operationResourceName: opName,
masterTimeout: 15 * 60 * 1000,
});
}References
- Look for existing utilities first: Before writing common helper functions, check if a suitable function already exists. (link)
| await artifactregistry.ensureRepository(projectId, region, "cloud-run-source-deploy"); | ||
|
|
||
| // Construct image URI | ||
| const imageUri = `${region}-docker.pkg.dev/${projectId}/cloud-run-source-deploy/${service.serviceId}:latest`; |
There was a problem hiding this comment.
Using the :latest tag for the container image URI in Cloud Run deployments is risky. If multiple deployments run concurrently or sequentially, they will overwrite the :latest tag. Furthermore, if Cloud Run needs to restart a container or scale up, it might pull the newer :latest image instead of the one that was originally deployed with the revision, leading to silent production issues. It is highly recommended to use a unique tag (such as a timestamp or commit SHA) for each deployment.
| const imageUri = `${region}-docker.pkg.dev/${projectId}/cloud-run-source-deploy/${service.serviceId}:latest`; | |
| const imageTag = `${Date.now()}`; | |
| const imageUri = `${region}-docker.pkg.dev/${projectId}/cloud-run-source-deploy/${service.serviceId}:${imageTag}`; |
| if ((appHostingConfig as any)?.scripts?.build) { | ||
| buildEnv["GOOGLE_NODE_RUN_SCRIPTS"] = (appHostingConfig as any).scripts.build; | ||
| } |
There was a problem hiding this comment.
Avoid using as any as an escape hatch. Instead of casting appHostingConfig to any to access scripts, we should define the scripts property on the AppHostingYamlConfig class in src/apphosting/yaml.ts. This ensures type safety and adheres to the repository style guide.
| if ((appHostingConfig as any)?.scripts?.build) { | |
| buildEnv["GOOGLE_NODE_RUN_SCRIPTS"] = (appHostingConfig as any).scripts.build; | |
| } | |
| if (appHostingConfig?.scripts?.build) { | |
| buildEnv["GOOGLE_NODE_RUN_SCRIPTS"] = appHostingConfig.scripts.build; | |
| } |
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
| if ((runConfig as any).vpcAccess) { | ||
| service.template.vpcAccess = (runConfig as any).vpcAccess; | ||
| } |
There was a problem hiding this comment.
Avoid using as any as an escape hatch. Instead of casting runConfig to any to access vpcAccess, we should add vpcAccess to the AppHostingRunConfig interface in src/apphosting/config.ts. This ensures type safety and adheres to the repository style guide.
| if ((runConfig as any).vpcAccess) { | |
| service.template.vpcAccess = (runConfig as any).vpcAccess; | |
| } | |
| if (runConfig.vpcAccess) { | |
| service.template.vpcAccess = runConfig.vpcAccess; | |
| } |
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
| const runtimeOpt = ((options as any).runtime || (options as any).baseImage) as string | undefined; | ||
| const clearOpt = !!((options as any).clearRuntime || (options as any).clearBaseImage); |
There was a problem hiding this comment.
Avoid using as any as an escape hatch. We can define a RunDeployOptions interface in src/deploy/run/args.ts that extends Options with the Cloud Run specific flags (like runtime, baseImage, clearRuntime, clearBaseImage, primaryRegion, and region). Typing options as RunDeployOptions in prepare and deploy will eliminate all as any casts in this file.
| const runtimeOpt = ((options as any).runtime || (options as any).baseImage) as string | undefined; | |
| const clearOpt = !!((options as any).clearRuntime || (options as any).clearBaseImage); | |
| const runtimeOpt = (options.runtime || options.baseImage); | |
| const clearOpt = !!(options.clearRuntime || options.clearBaseImage); |
References
- Never use
anyorunknownas an escape hatch. Define proper interfaces/types or use type guards. (link)
| if (!projectId) { | ||
| throw new FirebaseError("Project ID must be set before initializing Cloud Run."); | ||
| } |
There was a problem hiding this comment.
When throwing a FirebaseError due to a violation of a precondition (such as projectId being missing during initialization), we should specify a non-zero exit code (e.g., { exit: 1 }) to adhere to the repository style guide.
| if (!projectId) { | |
| throw new FirebaseError("Project ID must be set before initializing Cloud Run."); | |
| } | |
| const projectId = setup.projectId; | |
| if (!projectId) { | |
| throw new FirebaseError("Project ID must be set before initializing Cloud Run.", { exit: 1 }); | |
| } |
References
- Throw
FirebaseError(src/error.ts) for expected, user-facing errors. If the error is due to a violation of a precondition (e.g. something that is null but should never be), specify a non-zero exit code. (link)
| const sourceDir = options.config | ||
| ? options.config.path(config.source || config.rootDir || ".") | ||
| : process.cwd(); |
There was a problem hiding this comment.
Since options.config is guaranteed to be defined at this point (otherwise rawRunConfigs would be undefined and the function would have thrown an error on line 33), we can safely simplify this to directly call options.config.path without the ternary check.
const sourceDir = options.config.path(config.source || config.rootDir || ".");2 participants
This initial draft was ai-generated and must still be reviewed carefully by humans. It has been tested manually:
Description
Scenarios Tested
Sample Commands