fix(firestore-bigquery-export): stop using DATABASE_REGION as the function region - #3066
Merged
Merged
Conversation
…ction region DATABASE_REGION offers Firestore locations, including the multi-regions eur3/nam5/nam7, which are not Cloud Run regions; a function deployed with region set to one of them cannot deploy at all. The Firebase CLI already resolves a deploy region for functions that declare none (the already deployed region, else the database's mapped region for the Firestore trigger, else us-central1) and pins the trigger's Eventarc location to the database via the database option, so the parameter had no remaining consumer: drop the region option and remove DATABASE_REGION and ExportConfig.location. Fixes #3017
Contributor
There was a problem hiding this comment.
Code Review
This pull request removes the DATABASE_REGION parameter and the ExportConfig.location property from the firestore-bigquery-export kit. This change prevents deployment failures caused by mapping Firestore multi-region locations (such as eur3, nam5, or nam7) to Cloud Run regions. Instead, the Firebase CLI will automatically resolve the deploy region for each function and pin the Firestore trigger to the database's own region. The documentation, configuration logic, and tests have been updated accordingly, and a new test suite has been added to verify that no exported functions explicitly set a region. There are no review comments, so I have no additional feedback to provide.
This was referenced Sep 1, 2026
cabljac
added a commit
that referenced
this pull request
Sep 2, 2026
…3083) Two CI fixes found via #3066's failing run. The emulator step failed on every kit with a test:emulator script: firebase-tools 15 requires Java 21+ and the runner's default JAVA_HOME is 17 - the step now points JAVA_HOME at the preinstalled 21. And the changed-kit detection diffed against the event's base.sha, which goes stale when the base branch advances after the PR's last push, pulling base-side kits into the matrix (that is how counter and chatbot failed #3066's run); detection now three-dot diffs against the live base ref's merge-base. YAML validated; the detection command dry-run locally. Like its predecessors, the workflow only fully proves itself on the next kit PR.
cabljac
added a commit
that referenced
this pull request
Sep 2, 2026
Follow-up to #3083, which traded one detection bug for a worse one: the three-dot diff has no reachable merge base on the depth-1 merge-ref checkout, and the git failure inside command substitution silently produced an empty matrix - observed live on #3066's rerun, where all kit testing was skipped while the job reported green. Two-dot against the freshly fetched live base ref is exact for a merge-ref checkout (the merge ref is built on the live tip), and the diff now runs as its own step line so a git failure fails the job instead of skipping every kit. YAML validated; the command dry-run locally against origin/kits.
cabljac
added a commit
that referenced
this pull request
Sep 2, 2026
…ccess and onCompletion (#3071) The kit README's Events section claimed the function publishes `onStart`, `onError`, `onSuccess`, and `onCompletion`. The code publishes `onStart` and `onError` only: `src/handlers.ts` calls `recordStartEvent` and `recordErrorEvent`, and nothing calls `recordSuccessEvent` or `recordCompletionEvent`. The extension emitted `onSuccess` from its `syncBigQuery` task queue handler, which the kit deliberately removed, and the README's differences section already documents that. So the README was the wrong side; this corrects the Events section to list `onStart` and `onError` and point at the differences entry. Re-adding `onSuccess` emission (parity with the extension) is a separate decision, not attempted here. Doc-only change, so no tests. Note #3066 also edits this README; its hunks (config table, provisioning, region sections) do not overlap this one. Fixes #3018
cabljac
added a commit
that referenced
this pull request
Sep 2, 2026
…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.
cabljac
added a commit
that referenced
this pull request
Sep 8, 2026
…for function placement (#3102) Fixes #3069. The kit passed the raw DATABASE_REGION value as the function's region, and the param's select offers the Firestore multi-regions eur3/nam5/nam7, which are not Cloud Run regions, so multi-region deploys hard-failed. The issue proposed dropping the region option (the #3066 pattern). This PR deliberately deviates: by agreement with the firebase-tools team, DATABASE_REGION stays and is mapped instead, because the CLI's own inference is unreliable for param-declared databases (firebase/firebase-tools#11020) and the extension-to-kit migration exports DATABASE_REGION into the user's .env. Multi-regions map to a region inside them (nam5/nam7 to us-central1, eur3 to europe-west1, mirroring the CLI's FIRESTORE_DUAL_REGION_TO_REGION_MAPPING); regional locations pass through; unset or empty means no region option and the CLI fallback. #3101 is the sibling fix for firestore-bigquery-export. Tests pin all mapping cases plus the unset case; breaking the mapping fails them. Caveats: no live multi-region deploy was run for this kit, and the module-load process.env read needs firebase-tools >= 15.28.0 during discovery (older CLIs degrade to the no-region fallback). The approved #3089 also touches src/config.ts; this change edits only envDeployOptions at the end of the file, so overlap is limited to a trivial CHANGELOG conflict.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The kit passed
DATABASE_REGIONas theregionoption on all three functions, and that select offers the Firestore multi-regionseur3/nam5/nam7, which are not Cloud Run regions, so a multi-region database could not deploy at all.The #2974 claim that kits build function location in checks out in firebase-tools 15.28.2: an endpoint that declares no region is resolved by
resolveDefaultRegionsForBuild(lib/deploy/functions/prepare.js) - the already-deployed region wins, else a trigger-derived default, else us-central1;FIREBASE_FUNCTIONS_DEFAULT_REGIONoverrides the default (lib/api.js:71). For this kit the trigger-derived path (firestore.getDefaultRegion,FIRESTORE_DUAL_REGION_TO_REGION_MAPPING) cannot fire today: resolution runs on the unresolved build wheredatabaseis still the CEL string{{ params.DATABASE }}, the lookup throws and is swallowed at debug level, so on a first deploy every function lands in us-central1. The trigger's Eventarc location is pinned to the database independently (lib/deploy/functions/services/firestore.js,ensureFirestoreTriggerRegion).So this drops the
regionoption and removesDATABASE_REGIONandExportConfig.locationoutright; nothing else consumed them. A new tests/index.test.ts pins (repro-first) that no exported function sets a region and the trigger still bindsdatabase; the built module's discovery manifest was verified locally to emit no region. No live multi-region deploy was attempted.firestore-send-emailshares this conflation (its config.ts region: params.databaseRegion) and is left for its own fix.Fixes #3017