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 significantly enhances the App Hosting deployment workflow by introducing robust support for environment variables during local and zip deployments. It enables the definition and automatic injection of environment variables, including Firebase web app configuration, into the build environment, ensuring consistent behavior across development and deployment stages. The changes also include necessary permission checks for the App Hosting service agent and refined handling of build artifacts. 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. 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. 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 adds support for environment variables for local and zip-based deployments in App Hosting. The changes include parsing environment variables from apphosting.yaml, splitting them into build-time and runtime variables, and injecting them into the local build process. New tests are added to cover this functionality. My review includes a few suggestions: fixing a potential bug in how environment variables are merged, adhering to the style guide for error handling, and cleaning up some code duplication in both source and test files.
…ols into zip_deploy_env_vars
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…ols into zip_deploy_env_vars
…ols into zip_deploy_env_vars
…ols into zip_deploy_env_vars
…ols into zip_deploy_env_vars
|
After my recent changes, the env vars stopped working. I suspect it's because of this: I removed the "env" field from LocalBuild, because i assumed it was redundant. After all, we already have LocalBuild.BuildConfig.Env. Therefore, I just put the env vars into this field. However, I wonder if BuildConfig.Env is only for build-level configs. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly enhances App Hosting local builds and service agent role management. Key changes include the introduction of ensureAppHostingServiceAgentRoles for granting necessary permissions and the implementation of splitEnvVars to categorize environment variables for build and runtime. The localBuild process now injects these environment variables into process.env and handles NODE_PATH. Additionally, the prepare step dynamically fetches and injects Firebase web app configuration into the build environment when an appId is present. Review feedback highlights potential issues with toProcessEnv incorrectly converting numeric 0 or boolean false to empty strings, the local build being hardcoded for "nextjs", the need for more robust error logging for unknown types, type-safety improvements for numeric values in tests, and an unresolved uncertainty regarding buildInput.config behavior.
|
|
||
| function toProcessEnv(env: EnvMap): NodeJS.ProcessEnv { | ||
| return Object.fromEntries( | ||
| Object.entries(env).map(([key, value]) => [key, value.value || ""]), |
There was a problem hiding this comment.
The expression value.value || "" could unintentionally convert numeric 0 or boolean false values to an empty string. It's safer to explicitly convert value.value to a string using String(value.value) to ensure all values are correctly represented as strings in process.env.
| Object.entries(env).map(([key, value]) => [key, value.value || ""]), | |
| Object.entries(env).map(([key, value]) => [key, String(value.value || "")]), |
| } | ||
| context.backendLocalBuilds[cfg.backendId] = { | ||
| // TODO(9114): This only works for nextjs. | ||
| buildDir: outputFiles[0], |
There was a problem hiding this comment.
The TODO comment indicates that the local build is currently hardcoded for "nextjs". This is a significant functional limitation for a general App Hosting feature. It should be addressed to support other frameworks or at least clearly documented as a known limitation.
| ["roles/storage.objectViewer"], | ||
| /* skipAccountLookup= */ true, | ||
| ); | ||
| } catch (err: unknown) { |
There was a problem hiding this comment.
The err variable is of type unknown. It's safer to explicitly convert it to a string using String(err) or use a utility like getErrMsg from ../error to ensure proper logging, especially if err might not always be an Error instance.
logger.debug(`Failed to grant storage.objectViewer to ${p4saEmail}: ${String(err)}`);| NUM: { value: 12345 as any }, | ||
| BUILD_AND_RUNTIME_NUM: { value: 67890 as any, availability: ["BUILD", "RUNTIME"] }, |
There was a problem hiding this comment.
Using as any here for numeric values, while functional due to explicit string conversion in splitEnvVars, could be made more type-safe. Consider defining the EnvMap type to explicitly allow number for value if that's the intended input, or provide string literals if the input is always expected to be stringified before use.
| NUM: { value: 12345 as any }, | |
| BUILD_AND_RUNTIME_NUM: { value: 67890 as any, availability: ["BUILD", "RUNTIME"] }, | |
| NUM: { value: "12345" }, | |
| BUILD_AND_RUNTIME_NUM: { value: "67890", availability: ["BUILD", "RUNTIME"] }, |
| location: context.backendLocations[backendId], | ||
| buildInput: { | ||
| config: context.backendLocalBuilds[backendId]?.buildConfig, | ||
| // TODO: Confirm this config behavior |
There was a problem hiding this comment.
The TODO comment "Confirm this config behavior" indicates an unresolved uncertainty about the buildInput.config behavior. This should be clarified and confirmed to ensure the deployment process behaves as expected, or a plan for its resolution should be in place.
… explicitly state an availability
3 participants
Description
Scenarios Tested
Sample Commands