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 22 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.
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.
…runtime (i.e. during the interactive CLI flow) but after that the internal methods do not allow optional parameters for values like the service region. This makes it less likely to silently use a default value instead of the user-provided one.
…d Run already stores them on the service object.
| .option("-s, --service <serviceId>", "Cloud Run service ID") | ||
| .option("--service-id <serviceId>", "Cloud Run service ID") | ||
| .option("--primary-region <region>", "primary region for Cloud Run") | ||
| .option("--region <region>", "region for Cloud Run") | ||
| .option("--root-dir <rootDir>", "root directory for source code") | ||
| .option("--output-dir <outputDir>", "output directory for built artifacts") | ||
| .help(HELP) | ||
| .action(initAction); |
There was a problem hiding this comment.
We shouldnt add the cloud-run specific command args to the init command for ALL features. These flags are only relevant if running firebase init run, is there any way we can clean this up.
| // HACK: Windows Node has issues with selectables as the first prompt, so we | ||
| // add an extra confirmation prompt that fixes the problem | ||
| // TODO: see if this issue still persists in the new prompt library. | ||
| if (process.platform === "win32") { | ||
| if (!(await confirm("Are you ready to proceed?"))) { | ||
| throw new FirebaseError("Aborted by user.", { exit: 1 }); | ||
| } | ||
| } | ||
|
|
||
| if (feature) { | ||
| setup.featureArg = true; | ||
| setup.features = [feature]; | ||
| } else { | ||
| setup.features = await checkbox<string>({ | ||
| message: | ||
| "Which Firebase features do you want to set up for this directory? " + | ||
| "Press Space to select features, then Enter to confirm your choices.", | ||
| choices: choices.filter((c) => !c.hidden), | ||
| validate: (choices) => { | ||
| if (choices.length === 0) { | ||
| return ( | ||
| "Must select at least one feature. Use " + | ||
| clc.bold(clc.underline("SPACEBAR")) + | ||
| " to select features, or specify a feature by running " + | ||
| clc.bold("firebase init [feature_name]") | ||
| ); | ||
| } | ||
| return true; | ||
| }, | ||
| }); | ||
| } | ||
| if (!setup.features || setup.features?.length === 0) { | ||
| throw new FirebaseError( | ||
| "Must select at least one feature. Use " + | ||
| clc.bold(clc.underline("SPACEBAR")) + | ||
| " to select features, or specify a feature by running " + | ||
| clc.bold("firebase init [feature_name]"), | ||
| ); | ||
| } | ||
|
|
||
| // Always set up project | ||
| setup.features.unshift("project"); | ||
|
|
||
| // If there is more than one account, add an account choice phase | ||
| const allAccounts = getAllAccounts(); | ||
| if (allAccounts.length > 1) { | ||
| setup.features.unshift("account"); | ||
| } | ||
|
|
||
| // "hosting:github" is a part of "hosting", so if both are selected, "hosting:github" is ignored. | ||
| if (setup.features.includes("hosting") && setup.features.includes("hosting:github")) { | ||
| setup.features = setup.features.filter((f) => f !== "hosting:github"); | ||
| } | ||
| // "dataconnect:sdk" is a part of "dataconnect", so if both are selected, "dataconnect:sdk" is ignored. | ||
| if (setup.features.includes("dataconnect") && setup.features.includes("dataconnect:sdk")) { | ||
| setup.features = setup.features.filter((f) => f !== "dataconnect:sdk"); | ||
| } | ||
|
|
||
| // Always prompt for agent skills at the end of init | ||
| setup.features.push("agentSkills"); | ||
|
|
||
| await init(setup, config, options); | ||
| await postInitSaves(setup, config); | ||
|
|
There was a problem hiding this comment.
Splitting up the init function is good, and it can be done in a pre-requisite PR first.
Basically, I'll just extract this out for readability / faster reviw
| export const DEFAULT_RUN_IGNORE = [ | ||
| "node_modules", | ||
| ".git", | ||
| ".next", | ||
| ".run", | ||
| "firebase-debug.log", | ||
| "firebase-debug.*.log", | ||
| ".env*.local", | ||
| "apphosting.local.yaml", | ||
| "**/*.secret.local", | ||
| ]; |
There was a problem hiding this comment.
scrutinize these, why should we be this opinionated?
| /** | ||
| * Ensures an Artifact Registry repository exists, creating it if not. | ||
| */ | ||
| export async function ensureRepository( |
There was a problem hiding this comment.
rename to ensureRepositoryExists
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for configuring and deploying Cloud Run services directly via the Firebase CLI, including integration with apphosting.yaml settings such as scaling, VPC access, and environment variables. Feedback on the changes highlights three critical issues: defaulting supportGitIgnore to true in archiveDirectory is a breaking change that could affect other deploy targets; the lifecycle hook filtering for run will incorrectly reject configurations when deploying with --only run; and polling Cloud Build by extracting a buildId is fragile and should be replaced by polling the buildOperation resource directly.
| path: sourceDirectory, | ||
| ignoreStrings: options.ignore, | ||
| ignoreSymlinks: true, | ||
| supportGitIgnore: options.supportGitIgnore ?? true, |
There was a problem hiding this comment.
Changing the default of supportGitIgnore to true for all callers of archiveDirectory is a breaking change. Other deploy targets (like Functions or Hosting) that use archiveDirectory might not expect .gitignore to be automatically respected, which could silently exclude files from their deployments. It is safer to default this to false to preserve backward compatibility, while allowing Cloud Run to explicitly opt-in.
| supportGitIgnore: options.supportGitIgnore ?? true, | |
| supportGitIgnore: options.supportGitIgnore ?? false, |
| } else if (target === "run") { | ||
| return targetConfigs.filter((config: any) => { | ||
| return !config.serviceId || onlyTargets.includes(config.serviceId); | ||
| }); |
There was a problem hiding this comment.
If the user runs firebase deploy --only run, onlyTargets will contain ["run"] (or similar depending on parsing, but it won't contain the specific serviceId). Since config.serviceId is always defined, !config.serviceId is false, and onlyTargets.includes(config.serviceId) is false. Thus, the filter will reject all configs, and none of the lifecycle hooks (predeploy/postdeploy) for run will execute. We should also check if the general target "run" is in the list of only targets.
| } else if (target === "run") { | |
| return targetConfigs.filter((config: any) => { | |
| return !config.serviceId || onlyTargets.includes(config.serviceId); | |
| }); | |
| } else if (target === "run") { | |
| return targetConfigs.filter((config: any) => { | |
| return !config.serviceId || onlyTargets.includes(config.serviceId) || onlyTargets.includes("run"); | |
| }); |
| const op = res.body.buildOperation; | ||
| const buildId = | ||
| (typeof op === "object" && op?.metadata?.build?.id) || | ||
| (typeof op === "string" | ||
| ? op | ||
| .split("/") | ||
| .pop() | ||
| ?.replace(/^build-/, "") | ||
| : ""); | ||
| if (buildId) { | ||
| let latestBuild: { status?: string; statusDetail?: string; logUrl?: string } | undefined; | ||
| await pollOperation<any>({ | ||
| pollerName: "Cloud Build Poller", | ||
| apiOrigin: cloudbuildOrigin(), | ||
| apiVersion: "v1", | ||
| operationResourceName: `projects/${projectId}/locations/${location}/builds/${buildId}`, | ||
| masterTimeout: 15 * 60 * 1000, | ||
| backoff: 2000, | ||
| maxBackoff: 10000, | ||
| onPoll: (res: any) => { | ||
| latestBuild = res; | ||
| }, | ||
| doneFn: (buildRes: any) => { | ||
| const status = buildRes?.status; | ||
| return ( | ||
| status === "SUCCESS" || | ||
| status === "FAILURE" || | ||
| status === "INTERNAL_ERROR" || | ||
| status === "TIMEOUT" || | ||
| status === "CANCELLED" | ||
| ); | ||
| }, | ||
| }); | ||
|
|
||
| if (latestBuild && latestBuild.status !== "SUCCESS") { | ||
| const detail = latestBuild.statusDetail ? `: ${latestBuild.statusDetail}` : ""; | ||
| const consoleLink = | ||
| latestBuild.logUrl || | ||
| `https://console.cloud.google.com/cloud-build/builds;region=${location}/${buildId}?project=${projectId}`; | ||
| throw new FirebaseError( | ||
| `Cloud Build failed with status ${latestBuild.status}${detail}\nView Cloud Build logs at: ${consoleLink}`, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Polling builds/${buildId} directly is extremely fragile and prone to 404 errors if buildOperation is returned as a string that doesn't match the expected build- prefix format. Instead of extracting the buildId and polling the Cloud Build Build resource, we should poll the buildOperation resource name directly (which is guaranteed to be valid). The operation's metadata contains the build object, so we can still access the build status, status detail, and log URL from op.metadata.build on each poll.
const op = res.body.buildOperation;
const operationResourceName = typeof op === "string" ? op : op.name;
if (operationResourceName) {
let latestBuild: { status?: string; statusDetail?: string; logUrl?: string } | undefined;
await pollOperation<any>({
pollerName: "Cloud Build Poller",
apiOrigin: cloudbuildOrigin(),
apiVersion: "v1",
operationResourceName,
masterTimeout: 15 * 60 * 1000,
backoff: 2000,
maxBackoff: 10000,
onPoll: (opRes: any) => {
latestBuild = opRes?.metadata?.build;
},
doneFn: (opRes: any) => {
const status = opRes?.metadata?.build?.status;
return (
status === "SUCCESS" ||
status === "FAILURE" ||
status === "INTERNAL_ERROR" ||
status === "TIMEOUT" ||
status === "CANCELLED"
);
},
});
if (latestBuild && latestBuild.status !== "SUCCESS") {
const detail = latestBuild.statusDetail ? `: ${latestBuild.statusDetail}` : "";
const consoleLink =
latestBuild.logUrl ||
`https://console.cloud.google.com/cloud-build/builds?project=${projectId}`;
throw new FirebaseError(
`Cloud Build failed with status ${latestBuild.status}${detail}\nView Cloud Build logs at: ${consoleLink}`,
);
}
}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