Repository navigation
Conversation
App Hosting and Cloud Run each had their own copy of the local build steps: copy the project to a scratch folder, build it with Universal Maker, tar the output, and delete the scratch folder. Both now call buildLocally() in src/deploy/apphosting/localBuild.ts. Each product keeps only what's specific to it: its build env, its Node.js check input, and how it rolls out the build. App Hosting now tars the build in its prepare step, right after building, instead of in its deploy step. The scratch folder is deleted right after the build, including when the build fails.
There was a problem hiding this comment.
Code Review
This pull request refactors the local build process for App Hosting and Cloud Run by introducing a unified buildLocally function in a new localBuild.ts module. This consolidates the steps of preparing a scratch directory, running the build, and packaging the output into a tarball, simplifying the LocalBuild interface to use a single archive path. The feedback recommends refactoring the buildLocally function signature to accept a single options object instead of five positional arguments, in accordance with the repository's style guide, and provides corresponding updates for its callers and tests.
| export async function buildLocally( | ||
| projectId: string, | ||
| projectDir: string, | ||
| config: AppHostingSingle, | ||
| env: EnvMap, | ||
| options: { scratchDir: string; nonInteractive?: boolean; allowLocalBuildSecrets?: boolean }, | ||
| ): Promise<PackagedLocalBuild> { | ||
| const { scratchDir } = options; |
There was a problem hiding this comment.
According to the repository style guide (GEMINI.md, line 55), functions accepting more than three arguments should accept a single options object instead of individual positional arguments. This improves maintainability and makes future signature changes easier.
export interface BuildLocallyOptions {
projectId: string;
projectDir: string;
config: AppHostingSingle;
env: EnvMap;
scratchDir: string;
nonInteractive?: boolean;
allowLocalBuildSecrets?: boolean;
}
export async function buildLocally(options: BuildLocallyOptions): Promise<PackagedLocalBuild> {
const {
projectId,
projectDir,
config,
env,
scratchDir,
nonInteractive,
allowLocalBuildSecrets,
} = options;References
- Functions accepting more than three arguments should accept a single options object ({ project, force }). (link)
| const { archive, buildConfig } = await buildLocally( | ||
| projectId, | ||
| localBuildScratchDir, | ||
| rootDir, | ||
| cfg, | ||
| buildEnv[cfg.backendId] || {}, | ||
| { | ||
| scratchDir: localBuildScratchDir, | ||
| nonInteractive: options.nonInteractive, | ||
| allowLocalBuildSecrets: !!options.allowLocalBuildSecrets, | ||
| rootDir: cfg.rootDir, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Update the call to buildLocally to pass a single options object, matching the refactored signature.
const { archive, buildConfig } = await buildLocally({
projectId,
projectDir: rootDir,
config: cfg,
env: buildEnv[cfg.backendId] || {},
scratchDir: localBuildScratchDir,
nonInteractive: options.nonInteractive,
allowLocalBuildSecrets: !!options.allowLocalBuildSecrets,
});| const { archive, buildConfig } = await buildLocally( | ||
| context.projectId, | ||
| scratchDir, | ||
| options.config.projectDir, | ||
| cfg, | ||
| toLocalBuildEnv(svc.buildEnv || {}), | ||
| // Unlike App Hosting, Cloud Run doesn't ask users to confirm build secrets in local builds. | ||
| { rootDir: config.rootDir, allowLocalBuildSecrets: true }, | ||
| { | ||
| // The build runs in a scratch copy of the source, which is deleted afterwards. | ||
| scratchDir: fs.mkdtempSync(path.join(os.tmpdir(), `run-local-build-${serviceId}-`)), | ||
| // Unlike App Hosting, Cloud Run doesn't ask users to confirm build secrets in local builds. | ||
| allowLocalBuildSecrets: true, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Update the call to buildLocally to pass a single options object, matching the refactored signature.
const { archive, buildConfig } = await buildLocally({
projectId: context.projectId,
projectDir: options.config.projectDir,
config: cfg,
env: toLocalBuildEnv(svc.buildEnv || {}),
scratchDir: fs.mkdtempSync(path.join(os.tmpdir(), `run-local-build-${serviceId}-`)),
allowLocalBuildSecrets: true,
});| return buildLocally("my-project", projectDir, config, env, { | ||
| scratchDir, | ||
| allowLocalBuildSecrets: true, | ||
| }); |
| expect(buildLocallyStub).to.have.been.calledWithMatch( | ||
| "my-project", | ||
| scratchDir, | ||
| "/project", | ||
| { backendId: "web", rootDir: "apps/web", localBuild: true }, | ||
| {}, | ||
| { rootDir: "apps/web" }, | ||
| { scratchDir }, | ||
| ); |
There was a problem hiding this comment.
Update the assertion to match the single options object signature of buildLocally.
| expect(buildLocallyStub).to.have.been.calledWithMatch( | |
| "my-project", | |
| scratchDir, | |
| "/project", | |
| { backendId: "web", rootDir: "apps/web", localBuild: true }, | |
| {}, | |
| { rootDir: "apps/web" }, | |
| { scratchDir }, | |
| ); | |
| expect(buildLocallyStub).to.have.been.calledWithMatch({ | |
| projectId: "my-project", | |
| projectDir: "/project", | |
| config: { backendId: "web", rootDir: "apps/web", localBuild: true }, | |
| env: {}, | |
| scratchDir, | |
| }); |
Description
Stacked on #11282. App Hosting and Cloud Run each had their own copy of the local build steps: copy the project to a scratch folder, build it with Universal Maker, tar the output, and delete the folder. A fix like #11396 had to be made in both copies.
Both now call
buildLocally()inapphosting/localBuild.ts. Each product keeps only its own build env, Node.js version check, and rollout.Behavior changes:
Tip: hide whitespace when reviewing
apphosting/deploy.ts; most of its diff is re-indentation.Scenarios Tested
localBuild.spec.tsruns the shared copy → build → tar → cleanup on a real temp project, with only Universal Maker faked. It covers cleanup after a failed build, and a failed cleanup not hiding the build error.Sample Commands
# With "localBuild": true on the service or backend in firebase.json: firebase deploy --only run:web firebase deploy --only apphosting:web