Integrate FAH Local Builds with Universal Maker#10382
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for 'Universal Maker' local builds, integrates Secret Manager secret resolution for local environments, and refactors ABIU configuration. Key feedback includes fixing a blocking issue in the build watcher, adhering to style guides for YAML parsing and error handling, and optimizing the bundling process to avoid slow file copies of node_modules.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the "Universal Maker" for App Hosting local builds, enabling standalone binary builds through a new experiment. It also enhances secret management by implementing a utility to resolve secrets from Secret Manager during local builds, adding a security confirmation prompt, and introducing the --allow-local-build-secrets flag. Furthermore, the PR removes explicit "Automatic Base Image Updates" (ABIU) configuration flags, integrating ABIU status into the runtime selection instead. Review feedback suggests strengthening test assertions for binary execution, removing unknown type assertions to comply with the style guide, and generalizing build artifact packaging logic to remove framework-specific assumptions.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for a new 'Universal Maker' standalone binary for local builds in App Hosting, controlled via an experimental flag. It adds the runUniversalMaker function to handle the binary execution, artifact management, and metadata parsing, while also updating the tar archive creation logic to support flattened directory structures for .apphosting outputs. Feedback focuses on improving the robustness of the build process by checking the binary's exit status, ensuring proper cleanup of stale artifacts and temporary directories, and removing hardcoded environment variables that might conflict with user configurations.
* Add a file cleanup callback * Clean up the file handling * Add error handling
…versal maker to share some code.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the 'Universal Maker' binary for local builds in App Hosting, including logic for downloading, validating, and executing the tool. It also improves temporary file management during deployments by ensuring cleanup via callbacks. Several improvements were identified regarding the safety of command construction, the use of blocking synchronous calls in an asynchronous environment, and the need for better error handling and type safety in accordance with the repository's style guide.
| throw new FirebaseError(`Failed to parse build_output.json: ${(e as Error).message}`); | ||
| } | ||
|
|
||
| let finalRunCommand = `${umOutput.command} ${umOutput.args.join(" ")}`; |
| const res = childProcess.spawnSync( | ||
| universalMakerBinary, | ||
| ["-application_dir", projectRoot, "-output_dir", projectRoot, "-output_format", "json"], | ||
| { | ||
| env: { | ||
| ...process.env, | ||
| X_GOOGLE_TARGET_PLATFORM: "fah", | ||
| FIREBASE_OUTPUT_BUNDLE_DIR: bundleOutput, | ||
| }, | ||
| stdio: "pipe", | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Using spawnSync blocks the Node.js event loop until the process exits. For a build process that can take a significant amount of time, this can make the CLI feel unresponsive. Consider using an asynchronous spawn wrapped in a Promise to allow the event loop to remain active, which is also better for handling process signals like SIGINT.
| metadata: { | ||
| language: umOutput.language, | ||
| runtime: umOutput.runtime, | ||
| framework: framework || "nextjs", |
| export function validateSize(filepath: string, expectedSize: number): Promise<void> { | ||
| return new Promise((resolve, reject) => { | ||
| const stat = fs.statSync(filepath); | ||
| return stat.size === expectedSize | ||
| ? resolve() | ||
| : reject( | ||
| new FirebaseError( | ||
| `download failed, expected ${expectedSize} bytes but got ${stat.size}`, | ||
| { exit: 1 }, | ||
| ), | ||
| ); | ||
| }); | ||
| } |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ploy" flow as well
… the "deploy" flow as well" This reverts commit 59a79f5.
Description
This takes the existing local builds solution and uses the Universal Maker binary (which runs all relevant buildpacks) instead of hackily running the apphosting adapter manually.
The Universal Maker is a more well-supported tool and behaves more similar to Cloud Builds so we can have more confidence about framework support and fidelity.
Major changes
Scenarios Tested
Created a local build with the
Sample Commands