feat(functions): load .env files and pass to functions discovery in runtimeDelegate - #10936
Conversation
…untimeDelegate ### Description Updates functions discovery (runtimeDelegate.discoverBuild) across deployment (prepare.ts), emulator (functionsEmulator.ts), and IAC export (export.ts) to load existing .env files using functionsEnv.loadUserEnvs and pass the loaded environment variables to the discovery process. Also updates the Dart runtime delegate to include envs in spawn options when running build_runner. ### Scenarios Tested - Verified loadCodebases loads .env variables and passes them to discoverBuild. - Verified functionsEmulator passes loaded userEnvs to discoverBuild. - Verified getInternalIac in export.ts loads .env variables and passes them to discoverBuild. - Ran unit test suite: npx mocha src/deploy/functions/prepare.spec.ts src/emulator/functionsEmulator.spec.ts src/functions/env.spec.ts src/functions/iac/export.spec.ts src/deploy/functions/runtimes/dart/index.spec.ts. ### Sample Commands - firebase deploy --only functions - firebase emulators:start --only functions
There was a problem hiding this comment.
Code Review
This pull request implements loading of user-defined environment variables (.env files) and passes them to the functions discovery process (discoverBuild) across deployment, emulation, and IAC export phases. The review feedback highlights an inconsistency in environment variable precedence in src/emulator/functionsEmulator.ts where userEnvs is spread after environment, which inverts the precedence compared to other parts of the codebase. Spreading userEnvs first is suggested to maintain consistency.
ajperel
left a comment
There was a problem hiding this comment.
Does this change mean our guidance around how parameters are evaluated at deploytime and runtime could change? Like calling .value() is probably safe in both contexts now? Though it's not bad to still do the old way? Should we eventually update documentation?
https://firebase.google.com/docs/functions/config-env#parameter-values
| const buildRunnerProcess = spawn(this.bin, ["run", "build_runner", "build"], { | ||
| cwd: this.sourceDir, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| // TODO: Including process.env was a mistake; only known envs should be included after a breaking change. |
There was a problem hiding this comment.
I'm a little confused. You're adding both here.... was process.env implicitly loaded before? Or why can't we just start with the state of only ..envs?
There was a problem hiding this comment.
Yes, it was implicit before. We actually need to change this in a bunch of places in Dart. In Python I'm not sure how virtualenv is doing this from a quick scan. Node explicitly strips unrelated env to avoid working on the dev machine and not in Cloud Build
There was a problem hiding this comment.
Ok. We can coordinate on the breaking change since Dart is still an experiment.
|
Allowing .value at global scope will still break. There have theoretically always been three paths: .config (deprecated), params (still supported and a recommended best practice), and raw .env (now more powerful)
|
ajperel
left a comment
There was a problem hiding this comment.
Thanks for the quick work!
| const buildRunnerProcess = spawn(this.bin, ["run", "build_runner", "build"], { | ||
| cwd: this.sourceDir, | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| // TODO: Including process.env was a mistake; only known envs should be included after a breaking change. |
There was a problem hiding this comment.
Ok. We can coordinate on the breaking change since Dart is still an experiment.
…with a multi-region mapping (#3101) This partially reverts #3066, by agreement with the firebase-tools team. #3066 removed the DATABASE_REGION param because its raw value was used as the function region, which hard-failed deploys for multi-region databases (#3017). But the CLI's region inference the kit fell back to is unreliable for param-declared databases: region resolution runs before param substitution and silently defaults to us-central1 (firebase/firebase-tools#11020). Since the extension-to-kit migration exports DATABASE_REGION into the user's .env, the decision is to keep using it, with the mapping that makes it safe where the pre-#3066 code failed. The restored param now places all three functions: Firestore multi-region locations map to a Cloud Run region (nam5/nam7 to us-central1, eur3 to europe-west1, mirroring the CLI's own FIRESTORE_DUAL_REGION_TO_REGION_MAPPING); regional locations pass through; unset keeps today's no-region behavior. The region is read from process.env at module load, which requires firebase-tools >= 15.28.0 (firebase/firebase-tools#10936) to be present during discovery; on older CLIs it degrades to the no-region fallback rather than failing. Tests pin all mapping cases and the unset case, and breaking the mapping fails them. No live multi-region deploy was run this time; #3066's no-region behavior was live-verified on nam5.
Description
Updates functions discovery (runtimeDelegate.discoverBuild) across deployment (prepare.ts), emulator (functionsEmulator.ts), and IAC export (export.ts) to load existing .env files using functionsEnv.loadUserEnvs and pass the loaded environment variables to the discovery process. Also updates the Dart runtime delegate to include envs in spawn options when running build_runner.
Scenarios Tested
Sample Commands