Conversation
…ploy
remoteconfig's --dry-run previously performed no server-side validation
at all: the release phase (the only place the real template PUT happens)
is skipped for every target when options.dryRun is true, and the
client-side validateInputRemoteConfigTemplate() only checks etag/conditions
shape, never touching condition expression content. This meant an invalid
percent() condition expression would pass --dry-run silently and only
fail on a real deploy.
database's prepare phase already validates realtime database rules
unconditionally (rtdb.updateRules(..., { dryRun: true })), regardless of
whether the overall deploy is a dry run. This makes prepare.ts call
publishTemplate(..., { validateOnly: true }) the same way, always, so
condition expression errors are caught before any target's release runs
in a multi-target deploy, not just when --dry-run is passed.
deployTemplate/publishTemplate gained a validateOnly option that sends
?validateOnly=true to the Remote Config template PUT endpoint, matching
the parameter name and casing already used by the Data Connect Admin API
client (dataconnect/client.ts).
|
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 ensures that Remote Config templates are validated against the Remote Config API's validate_only mode during the prepare phase, preventing invalid condition expression syntax from being silently accepted during dry runs. Feedback suggests utilizing the existing logLabeledBullet and logLabeledSuccess logging utilities from src/utils.ts instead of manually constructing colored labels with colorette, which also allows for the removal of the unused colorette import.
| utils.logBullet(clc.bold(clc.cyan("remoteconfig: ")) + "validating template..."); | ||
| await publishTemplate(projectNumber, template, template.etag, { validateOnly: true }); | ||
| utils.logSuccess(clc.bold(clc.green("remoteconfig: ")) + "template is valid"); |
There was a problem hiding this comment.
Instead of manually constructing colored labels with colorette, use the existing logging utilities logLabeledBullet and logLabeledSuccess from src/utils.ts. This aligns with the repository's style guide to reuse existing utilities and keeps the logging format consistent across different deploy targets.\n\nYou can also remove the unused import * as clc from "colorette"; at the top of the file.
utils.logLabeledBullet("remoteconfig", "validating template...");\n await publishTemplate(projectNumber, template, template.etag, { validateOnly: true });\n utils.logLabeledSuccess("remoteconfig", "template is valid");References
- Look for existing utilities first: Before writing common helper functions (e.g., for logging, file system operations, promises, string manipulation), check
src/utils.tsto see if a suitable function already exists. (link)
- Pass through options.force to the validate-only publishTemplate call in
prepare.ts, so If-Match semantics stay consistent between the validate-only
request and the real publish in release.ts.
- Have release.ts pass an explicit { force: options.force } instead of the
raw Options object, so a stray validateOnly field from a scripted `require
("firebase-tools")` caller can't silently turn a real deploy into a
validate-only no-op.
- Strengthen the validateOnly rejection test to assert on the propagated
error message, so a regression that swallows the server's validation
error text would be caught.
- Add nested @PARAM entries for options.force/options.validateOnly to clear
the jsdoc/check-param-names warnings introduced by the previous commit.
- Tighten the prepare.ts comment (previous wording had a dangling clause)
and fix the CHANGELOG entry, which said "during dry-run" and used the
wrong (snake_case) parameter name; validation now runs on every prepare,
dry-run or not.
Address Gemini Code Assist review feedback: reuse existing logging helpers instead of manually composing colored labels with colorette, matching the style used elsewhere in the deploy pipeline.
b9c4c27 to
eafac84
Compare
2 participants
Description
Closes #11125
firebase deploy --only remoteconfig --dry-runcurrently performs no server-side validation, so an invalidpercent()condition expression silently passes dry-run and only fails on a real deploy.Root cause: the deploy pipeline's
releasephase (the only place the real template PUT happens) is skipped for every target whenoptions.dryRunis true, and remoteconfig's client-sidevalidateInputRemoteConfigTemplate()never inspects condition expression content.Key changes:
prepare.tsnow callspublishTemplate(..., { validateOnly: true })unconditionally (matchingdatabase's unconditionalrtdb.updateRules(..., { dryRun: true })check), so expression errors surface before any target'sreleaseruns, not just when--dry-runis passed.deployTemplate/publishTemplategained avalidateOnlyoption that sends?validateOnly=trueto the Remote Config template PUT endpoint (same param name/casing asdataconnect/client.ts's Data Connect Admin API client).Scenarios Tested
npm run mocha -- 'src/deploy/remoteconfig/*.spec.ts'— all 5 passing, including two new cases covering thevalidateOnlyquery param and error propagationnpx tsc --project tsconfig.compile.json— cleannpm run lint:changed-files— 0 errors (pre-existing warnings only)npm link):--dry-run: printsremoteconfig: validating template.../remoteconfig: template is valid, thenDry run complete!--dry-run: fails with400 [VALIDATION_ERROR]: ... Was expecting one of: '(' '!' 'true' 'false' 'app' 'dateTime' 'device' 'percent' 'version'firebase-tools@15.30.2: printsDry run complete!with no error, confirming the reported bugSample Commands
No command or flag changes. Existing usage is unaffected:
now additionally prints, on success:
and on an invalid condition expression, fails with a
FirebaseErrordescribing the syntax error instead of silently completing.