Conversation
…to the local build process
1. Fix for injecting auto-init variables into the build 2. Fixes how we handle dependencies, nodejs paths, modulepaths, etc. This needs closer attention/fixes. 3. Adds env var handling (not secrets) and determines which env vars to pass down to the build
… variants) and include them in the final artifact
…zip_deploy_aryanf
…ctually a local build (instead of assuming true.)
…ore strictly. We also remove some hardcoded values and we generalize the code so that it does not affect source deploys. The goal is to prepare this PR so that we can safely submit it to main (behind the experiment flag.)
…ive. Only run it if it's a local build and the local build experiment flag is enabled.
Summary of ChangesHello, 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 introduces the foundational support for local builds within App Hosting, enabling developers to perform application builds on their local machines prior to deployment. It streamlines the deployment pipeline by integrating a new tar-based archiving mechanism for these local builds and ensures proper handling of environment variables during the build process. The changes are carefully integrated into the existing deployment and preparation steps, with the new functionality gated behind an experiment flag. 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. Changelog
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 foundational implementation for local builds in App Hosting. It correctly adds logic to use a tar archive for build artifacts, gates the feature behind an experimental flag, and updates the deployment flow to handle these new local builds. The changes are well-structured and include relevant tests. However, there appear to be some gaps in the handling of environment variables for local builds. Specifically, build-time environment variables are not passed to the local build process, and runtime environment variables are not correctly passed to the release/rollout phase. These issues could lead to incorrect builds and deployments.
…ools into zip_deploy_barebones
…ools into zip_deploy_barebones
…ools into zip_deploy_barebones
…ools into zip_deploy_barebones
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces barebones support for local builds in App Hosting, allowing the deployment of pre-built artifacts. Key changes include updating dependencies, adding a new createTarArchive utility for packaging local build outputs, and modifying the deployment and release processes to differentiate between source and local build deployments. New test cases have been added to cover the local build functionality and experiment flag enforcement. The documentation for localBuild and the main deployment function has also been improved.
| const targetDir = targetSubDir ? path.join(rootDir, targetSubDir) : rootDir; | ||
| // We must ignore firebase-debug.log or weird things happen if you're in the public dir when you deploy. | ||
| // const ignore = config.ignore || [".git"]; | ||
| const ignore = ["firebase-debug.log", "firebase-debug.*.log", ".git"]; |
There was a problem hiding this comment.
The createTarArchive function's ignore array is hardcoded and does not respect the config.ignore property or .gitignore files, unlike the createArchive function. This inconsistency means that user-defined ignore patterns in firebase.json or .gitignore files will not be applied to local builds, potentially leading to unintended files being included in the tarball or larger-than-necessary archives. This deviates from expected behavior and could expose sensitive files or increase deployment times unnecessarily.
const ignore = config.ignore || ["node_modules", ".git"];
ignore.push("firebase-debug.log", "firebase-debug.*.log");
const gitIgnorePatterns = parseGitIgnorePatterns(rootDir);
ignore.push(...gitIgnorePatterns);There was a problem hiding this comment.
createTarArchive is meant to package a built output directory. The .gitignore might specifically ignore compiled files.
There was a problem hiding this comment.
I might rename createArchive and createTarArchive to make them clear that they are for different products/features (source deploy vs. local builds.)
There was a problem hiding this comment.
Actually, I need to think more about whether or not we should respect "config.ignore".
There was a problem hiding this comment.
For this initial change, we do not need to handle it but I filed something to track it for the future
…ools into zip_deploy_barebones
|
I manually tested a local build and it was correctly deployed. Of course, env vars didn't work yet but I confirmed that:
|
|
|
||
| getProjectNumberStub.resolves("000000000000"); | ||
| upsertBucketStub.resolves("some-bucket"); | ||
| (experiments.assertEnabled as any).restore(); |
There was a problem hiding this comment.
From linter:
Unsafe member access .restore on an any value
Unexpected any. Specify a different type
| * @param targetSubDir - Optional subdirectory to simplify (e.g. if we only want to zip 'dist'). | ||
| * @returns A promise that resolves to the absolute path of the created temporary tarball. | ||
| */ | ||
| export async function createTarArchive( |
There was a problem hiding this comment.
Rename createTarArchive to "createLocalBuildTarArchive" and rename "createArchive" to "createSourceDeployArchive"
| const targetDir = targetSubDir ? path.join(rootDir, targetSubDir) : rootDir; | ||
| // We must ignore firebase-debug.log or weird things happen if you're in the public dir when you deploy. | ||
| // const ignore = config.ignore || [".git"]; | ||
| const ignore = ["firebase-debug.log", "firebase-debug.*.log", ".git"]; |
There was a problem hiding this comment.
clean up commented code
| * @param context - The deployment context containing backend configs, locations, and storage URIs. | ||
| * @param options - CLI options. | ||
| */ | ||
| export default async function (context: Context, options: Options): Promise<void> { |
There was a problem hiding this comment.
shouldn't we enforce the experiment flag on release.ts as well if the user has local build backends? We can skip those backends if the experiment flag is enabled and log a warning?
There was a problem hiding this comment.
I decided not to, we enforce the experiment check during deploy.ts, which is earlier.
| }; | ||
|
|
||
| const orchestrateRolloutStub = sinon.stub(rollout, "orchestrateRollout").resolves(); | ||
| sinon.stub(backend, "getBackend").resolves({ uri: "foo.apphosting.com" } as any); |
There was a problem hiding this comment.
Unsafe argument of type any assigned to a parameter of type Backend | undefined
3 participants
Description
Scenarios Tested
Sample Commands