fix(kits): restore extension select params for yes/no config parity - #3148
Conversation
Replace boolean declarations with labeled string selects so extension configuration values remain valid during kit migrations.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![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 migrates boolean configuration parameters across multiple kits (including delete-user-data, firestore-bigquery-export, firestore-genai-chatbot, firestore-send-email, firestore-vector-search, speech-to-text, and storage-resize-images) from defineBoolean to defineString with string-based select options (such as 'yes'/'no' or 'true'/'false'). This ensures backward compatibility with predecessor extensions' .env configurations. Additionally, the PR updates the respective README documentation and introduces comprehensive configuration parity tests to verify that these string values are parsed correctly. There are no review comments provided, and I have no additional feedback to offer as the changes are well-implemented and thoroughly tested.
The `config-parity.test.ts` name added a third convention next to the existing `config.test.ts` / `config-runtime.test.ts` split, so the cases move into that split instead. Coverage is unchanged: every declaration, option label, option value, option order, parsed value, and unset-variable case is kept verbatim. - firestore-genai-chatbot, firestore-vector-search, speech-to-text had no config test at all, so the file becomes `tests/config.test.ts`. - firestore-bigquery-export and firestore-send-email already have a `tests/config.test.ts` that fakes `firebase-functions/params`, so these cases, which need the real params, become `tests/config-runtime.test.ts`, the name delete-user-data already uses for the same reason.
firestore-bigquery-export: #3145 landed the same fix for `USE_NEW_SNAPSHOT_QUERY_SYNTAX` and `EXCLUDE_OLD_DATA`, and recorded the decision that `WILDCARD_IDS` stays `defineBoolean` because the extension's select there is `true` / `false`, which `BooleanParam` already parses (`!!process.env[name] && process.env[name] === "true"`). Resolved by taking `kits` for the whole kit, so this branch no longer changes firestore-bigquery-export at all: `src/config.ts` and `README.md` are the `kits` versions, and `tests/config-runtime.test.ts` is dropped since `tests/config.test.ts` on `kits` already covers the two yes/no params. storage-resize-images/tests/config.test.ts: kept both sides, the content-filter type imports from #3064 and this branch's `declaration()` helper.
The previous commit converted all ten extension selects to defineString with manual === "true" coercion. Only the three yes/no params need that: BooleanParam.runtimeValue() is `env === "true"`, so for the seven params whose extension values are already true/false the conversion changed no runtime value, and the declared default drives only the deploy-time prompt (an unset variable still reads false, matching the predecessor). select<T> is generic, so a BooleanParam carries the extension's option labels directly. Reverts OAUTH_SECURE, DO_BACKFILL, UPDATE_ON_CONFIGURE, ENABLE_AUTOMATIC_PUNCTUATION, MAKE_PUBLIC, IS_ANIMATED and REGENERATE_TOKEN to defineBoolean with select<boolean>, restoring the labels without giving up the typed param or hand-writing coercion. ENABLE_AUTO_DISCOVERY, ENABLE_DISCUSSION_OPTION_OVERRIDES and ENABLE_GENKIT_MONITORING stay defineString, matching #3145's scope. Tests now pin type: "boolean" with boolean-valued options; the runtime and unset-variable assertions are unchanged. Adds the per-kit CHANGELOG entries the branch was missing.
dcfd9c5 to
9385a86
Compare
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
Nice one, this is the right split. I checked the values against the legacy yaml for delete-user-data, send-email and storage-resize-images and they line up: the three literal yes/no params go to defineString, the true/false ones stay defineBoolean with labels, and defineBoolean's runtime is env === "true" so those are label-only. I could not check chatbot, vector-search or speech-to-text the same way, their legacy sources are not in this repo, so worth a quick eyeball from you that the yes/no values there match what the extension shipped.
One blocker: it conflicts with kits now. #3122 added kits/firestore-vector-search/tests/config.test.ts for the instance-id test, and yours creates a different file at the same path, plus the CHANGELOG top line. Fold your declaration tests into the existing file rather than replacing it.
Parity comparison was AI-led and I only spot-checked it, so push back if anything above does not match what you saw.
| optional(params.storageBucket.value()) ?? process.env.STORAGE_BUCKET, | ||
| storagePaths: optional(params.storagePaths.value()), | ||
| enableAutoDiscovery: params.enableAutoDiscovery.value(), | ||
| enableAutoDiscovery: params.enableAutoDiscovery.value() === "yes", |
There was a problem hiding this comment.
nit: bare === "yes" is exact parity with the extension, so fine to keep. Just flagging that #3145 landed a trimming/lowercasing yesNo() helper in bigquery-export (src/config.ts:640) for the same shape, so we now have two conventions. Not for this PR, but I think we should pick one and apply it everywhere in a follow-up, otherwise a hand-edited YES behaves differently per kit. Same applies to the two in chatbot config.ts:377-378.
| } | ||
|
|
||
| value(): string { | ||
| if (process.env[this.name] !== undefined) { |
There was a problem hiding this comment.
nit: reading process.env first in FakeStringParam.value() changes what every string param in this suite resolves to, not just the new one. CI is green so nothing regressed, but worth a look that no other test here was silently relying on the default path.
…nsion-select-params Conflicts, both in kits/firestore-vector-search: - CHANGELOG.md: kept both unreleased entries (FIREBASE_KIT_INSTANCE_ID from the base, the Yes/No label fix from this branch). - tests/config.test.ts: kept the base file's suites and env stubs and added this branch's inherited-select suite on top, switched to vi.stubEnv, since configFromEnv() now requires FIREBASE_CONFIG and FIREBASE_KIT_INSTANCE_ID.
…PUBLIC The CLI's select prompt passes the declared `default` straight to inquirer while stringifying every option value (firebase-tools `promptSelect`), so a non-string default matches no option and the first option is highlighted. `MAKE_PUBLIC` is the only select in this kit whose extension default is not the first option: the prompt highlighted `Yes`, so accepting it stored `MAKE_PUBLIC=true` and made every resized image public, where the extension stored `false`. Declare the param as a string with `default: "false"` and option values `"true"` / `"false"`, and coerce with `=== "true"` as the extension did. The stored values and the runtime result are unchanged, so no existing `.env` needs editing. `IS_ANIMATED` and `REGENERATE_TOKEN` keep `defineBoolean`: their default is `true` and `Yes` is their first option, so the highlighted option already matches the extension.
2 participants
Summary
The kits migration turned every extension yes/no select into
defineBoolean.BooleanParamonly parses the literaltrue(src/params/types.ts:717), so an.envcopied from an extension that storedyes/nosilently reads as off. Thetrue/falseselects parsed fine but lost their option labels.Rule applied per param:
yes/no:defineString+select({ Yes: "yes", No: "no" }), coerced inconfigFromEnv()with the extension's=== "yes". Stored values unchanged; kit config types stay boolean.true/falseand the default is the first option: keepdefineBoolean, restore labels withselect<boolean>({ Yes: true, No: false }). Label-only, no runtime change.MAKE_PUBLIC(storedtrue/false, defaultfalse= second option):defineStringwithdefault: "false", coerced with=== "true". The CLI'spromptSelectpassesdefaultraw but stringifies option values (params.tsdefault: resolvedDefault as stringvsvalue: option.value.toString()), so a boolean default matches nothing and "Yes" was preselected, making every resized image public on Enter.Same line as #3145, which kept
WILDCARD_IDSboolean and converted the yes/no params.firestore-bigquery-exportis no longer touched here.Affected params
ENABLE_AUTO_DISCOVERYyes, No=nonodefineString=== "yes"ENABLE_DISCUSSION_OPTION_OVERRIDESyes, No=nonodefineString=== "yes"ENABLE_GENKIT_MONITORINGyes, No=nonodefineString=== "yes"OAUTH_SECUREtrue, No=falsetruedefineBooleanBooleanParamDO_BACKFILLtrue, No=falsedefineBooleanBooleanParamUPDATE_ON_CONFIGUREtrue, No=falsedefineBooleanBooleanParamENABLE_AUTOMATIC_PUNCTUATIONtrue, Disabled=falsetruedefineBooleanBooleanParamMAKE_PUBLICtrue, No=falsefalsedefineString=== "true"IS_ANIMATEDtrue, No (1st frame only)=falsetruedefineBooleanBooleanParamREGENERATE_TOKENtrue, No=falsetruedefineBooleanBooleanParamChanges
IS_ANIMATED"Yes" label (kit had "True") and speech-to-text "Enabled" / "Disabled".yestotrue(delete-user-data, firestore-genai-chatbot, firestore-send-email).tests/config-runtime.test.tsbecause itsconfig.test.tsfakesfirebase-functions/params.Verification
npx tsc -bclean andnpx vitest rungreen in all six kits (send-email 158, vector-search 124, resize-images 172 + 6 skipped, delete-user-data 102, chatbot 58 + 3 skipped, speech-to-text 37). Prettier clean.Follow-up
#3145 coerces yes/no with a trimming, lowercasing
yesNo()helper; the 3 yes/no params here use the extension's bare=== "yes". Aligning them is a behavior change, so it is left for a follow-up.Accepted differences and inherited behavior
required: trueon the yes/no params is not reproduced. Platform difference, no value change.OAUTH_SECUREandENABLE_AUTOMATIC_PUNCTUATIONadvertise defaulttrue, but an unset variable reads asfalsein both the extension and the kit. Reproduced for parity and pinned by tests.UPDATE_ON_CONFIGUREwas declared but unused by the extension runtime; the kit uses it. Pre-existing, out of scope.