narratives: drop the cdc backend, and the deploy.sh shrink that follo… - #476
ddebasmita-lab wants to merge 3 commits into
Conversation
…wed it Ported from the source repo, where they are e02ac6f, ffd014c, ec1d4ce and 710b874. Taken as one commit because the second and third only make sense after the first. DATA_BACKEND is now dcp or none. Cloud SQL was the reason: no connection-pool sizing anywhere in the Terraform, a shared-core db-g1-small default tier, and Cloud Run concurrency high enough that MySQL failed with "too many connections" rather than merely slow queries. Spanner has no fixed connection ceiling, no NL server to run, managed ingestion, and a Mixer that defaults to stale reads so ingestion causes no downtime. Removed: the Cloud SQL instance, database and user; the data bucket; the ingest Job; the data-plane service and the app-to-data invoker binding; image/ and sample-data/; promote-image, which existed only to mirror datacommons-services:stable; and 18 Terraform variables nothing could reach. Also the "<instance>-runtime" service account and its seven IAM bindings -- that was the data-plane container's identity, cloudsql.client and objectAdmin on the data bucket, and no app-plane container has ever needed it. MAPS_API_KEY had the same single consumer, so supplying one now warns rather than storing a secret nothing reads. Kept deliberately. Direct VPC egress, whose derived default was is_cdc: a DCP plane can be a private service with internal ingress, which is what VPC egress is for, and that plane is provisioned elsewhere so its ingress is not knowable from here. MCP 1.2.x handling in the agent, which is payload-shape handling rather than cdc plumbing -- nothing guarantees every DCP instance serves 1.3.x. The "-datacommons" and "-data-ingest" suffixes in check-state-owner.py, because a state file written before this still contains those resources, and destroying one of those states is the case that guard exists to catch. deploy.sh rejects DATA_BACKEND=cdc with an explanation rather than a bare validation error: this revision cannot describe those resources, so it cannot destroy them either, and an instance still on that backend has to be torn down with the last revision that declared them. Then a sweep for what nothing uses. The app-plane image shipped two Gemini SDKs it never imported -- the agent calls Gemini over REST with requests -- and between them google-genai and google-generativeai pulled in httpx, pydantic, websockets, tqdm and the google-api-python-client stack. requirements drops from 48 packages to 29, and is generated now rather than hand-maintained, which surfaced two real dependencies of google-cloud-secret-manager that the old freeze was missing. Six branding keys were declared in the schema, shipped in every example and read by no code; two of them are worse than clutter, because analytics.ga_tag_id and extra_css could be set, synced, and silently do nothing. Then deploy.sh, 1146 lines to 609, no behaviour change. --preflight, --destroy, --config-only and --bootstrap-secrets are 341 lines that never run during a deploy, so they move to deploy/modes/ and are sourced -- they still share the loaded instance.env and the log helpers. Comments go from 266 lines to 115, each keeping its rule and its consequence and dropping the account of how it was found. Four lines of dead code go too, and monitoring.googleapis.com is now enabled: the module creates an alert policy and an uptime check, which works today only because the API is usually on already and would fail on a fresh project. The review fixes already on this branch are untouched -- the utf-8 config read and write, the urlparse prompt-URL derivation and its tests, and the dcproxy TODO. README keeps the test_prompt_urls line that came with them. Verified here after the port: terraform fmt and validate clean; bash -n clean on deploy.sh, smoke.sh and the four mode files; 45 agent checks across five suites; 108 UI tests in 9 files; tsc --noEmit and vite build clean.
There was a problem hiding this comment.
Code Review
This pull request removes the self-hosted Cloud SQL data plane (the cdc backend) and all of its associated resources, such as the services container, ingest job, and sample data, leaving only the dcp and none backends. It also refactors deploy.sh by modularizing its sub-commands into separate scripts under deploy/modes/ and cleans up unused dependencies in both the Python agent and the React UI. The review feedback identifies a critical bug where the deployment script fails to exit after running preflight checks, an issue with API key validation failing on transient Gemini API errors, a missing curl check in the CLI tool verification, and the omission of vite from the UI's devDependencies after its removal from dependencies.
| if [ "$PREFLIGHT" = true ]; then | ||
| run_preflight |
There was a problem hiding this comment.
If --preflight is specified, the script should exit after running run_preflight instead of continuing to the rest of the deployment. Currently, there is no exit statement, so running ./deploy.sh --preflight will perform the checks and then immediately proceed to execute the full deployment (configuring gcloud, enabling APIs, building images, running Terraform, etc.), which violates the 'create nothing' guarantee of preflight.
| if [ "$PREFLIGHT" = true ]; then | |
| run_preflight | |
| if [ "$PREFLIGHT" = true ]; then | |
| run_preflight || exit 1 | |
| exit 0 | |
| fi |
| case "$gem_code" in | ||
| 200) ;; | ||
| 000) log_warn "Could not reach the Gemini API to check a key; storing it unverified." ;; | ||
| *) bad_keys="${bad_keys} ${k:0:6}...(HTTP ${gem_code})" ;; | ||
| esac |
There was a problem hiding this comment.
The Gemini API key validation currently treats any non-200 and non-000 HTTP status code as a definitive key rejection, which will abort the deployment. However, transient errors such as rate limits (HTTP 429) or temporary server issues (HTTP 5xx) do not mean the key is invalid. It is safer to only fail on definitive client errors like HTTP 400, 401, or 403, and print a warning for other status codes.
| case "$gem_code" in | |
| 200) ;; | |
| 000) log_warn "Could not reach the Gemini API to check a key; storing it unverified." ;; | |
| *) bad_keys="${bad_keys} ${k:0:6}...(HTTP ${gem_code})" ;; | |
| esac | |
| case "$gem_code" in | |
| 200) ;; | |
| 000) log_warn "Could not reach the Gemini API to check a key; storing it unverified." ;; | |
| 400|401|403) bad_keys="${bad_keys} ${k:0:6}...(HTTP ${gem_code})" ;; | |
| *) log_warn "Gemini API returned HTTP ${gem_code} for key ${k:0:6}...; storing it unverified." ;; | |
| esac |
| unset _mode | ||
|
|
||
| # Early check for required CLI tools | ||
| for cmd in gcloud terraform npm python3; do |
There was a problem hiding this comment.
The script uses curl in bootstrap-secrets.sh to validate API keys, but curl is not included in the early check for required CLI tools. If curl is missing, the script will fail at runtime during --bootstrap-secrets. It should be added to the early check list.
| for cmd in gcloud terraform npm python3; do | |
| for cmd in gcloud terraform npm python3 curl; do |
| "devDependencies": { | ||
| "@tailwindcss/vite": "^4.1.14", |
There was a problem hiding this comment.
The primary build tool vite was removed from dependencies but was not added to devDependencies. While it might be transitively installed by @tailwindcss/vite or @vitejs/plugin-react, relying on transitive dependencies for your main build tool is risky and can lead to build failures or version mismatches. It should be explicitly declared in devDependencies.
| "devDependencies": { | |
| "@tailwindcss/vite": "^4.1.14", | |
| "devDependencies": { | |
| "vite": "^6.2.0", | |
| "@tailwindcss/vite": "^4.1.14", |
Two fixes from the review bot on this PR. bootstrap-secrets.sh validates both API keys with curl, but the early tool check did not ask for it, so a machine without curl got past the check and failed mid-run. The Gemini key check treated every non-200 as a rejected key, so a 429 or a 5xx aborted the deploy and told you the key was bad. Only 400, 401 and 403 say that; everything else now warns and stores the key. This is what the DC_API_KEY check a few lines above already does. Also drops four blank lines the mode extraction left behind.
… that Documents the agent-config document, which had a row in the override table and nothing else -- no section, no example to copy, and no mention that schemas/agent-config.example.json exists. The thinking levels, the model choices, the synthesis-only fallback and the query_param_key gate were undocumented, and the four steps to turn on the knowledge base lived only in a _comment inside defaults/agent-config.json. Drops provisioning a new DCP data plane. This repository attaches to an instance that is already running; standing one up is done elsewhere, with datacommons-cli, and documenting it here implied otherwise. Attaching is now the single place that explains dcp. Drops local development, testing, verifying a deployment, design notes and contributing, leaving the twelve sections a deployer needs. This removes the test_prompt_urls.py line from the Testing section along with the section itself. Fixes what the cdc removal and the deploy-mode extraction left stale: the intro still offered a Cloud SQL plane, the repository layout listed promote-image.yaml and a docs/architecture.drawio that no longer exists and omitted deploy/modes/, and curl was missing from the prerequisites now that it validates the API keys.
…wed it
Ported from the source repo, where they are e02ac6f, ffd014c, ec1d4ce and 710b874. Taken as one commit because the second and third only make sense after the first.
DATA_BACKEND is now dcp or none. Cloud SQL was the reason: no connection-pool sizing anywhere in the Terraform, a shared-core db-g1-small default tier, and Cloud Run concurrency high enough that MySQL failed with "too many connections" rather than merely slow queries. Spanner has no fixed connection ceiling, no NL server to run, managed ingestion, and a Mixer that defaults to stale reads so ingestion causes no downtime.
Removed: the Cloud SQL instance, database and user; the data bucket; the ingest Job; the data-plane service and the app-to-data invoker binding; image/ and sample-data/; promote-image, which existed only to mirror datacommons-services:stable; and 18 Terraform variables nothing could reach. Also the "-runtime" service account and its seven IAM bindings -- that was the data-plane container's identity, cloudsql.client and objectAdmin on the data bucket, and no app-plane container has ever needed it. MAPS_API_KEY had the same single consumer, so supplying one now warns rather than storing a secret nothing reads.
Kept deliberately. Direct VPC egress, whose derived default was is_cdc: a DCP plane can be a private service with internal ingress, which is what VPC egress is for, and that plane is provisioned elsewhere so its ingress is not knowable from here. MCP 1.2.x handling in the agent, which is payload-shape handling rather than cdc plumbing -- nothing guarantees every DCP instance serves 1.3.x. The "-datacommons" and "-data-ingest" suffixes in check-state-owner.py, because a state file written before this still contains those resources, and destroying one of those states is the case that guard exists to catch.
deploy.sh rejects DATA_BACKEND=cdc with an explanation rather than a bare validation error: this revision cannot describe those resources, so it cannot destroy them either, and an instance still on that backend has to be torn down with the last revision that declared them.
Then a sweep for what nothing uses. The app-plane image shipped two Gemini SDKs it never imported -- the agent calls Gemini over REST with requests -- and between them google-genai and google-generativeai pulled in httpx, pydantic, websockets, tqdm and the google-api-python-client stack. requirements drops from 48 packages to 29, and is generated now rather than hand-maintained, which surfaced two real dependencies of google-cloud-secret-manager that the old freeze was missing. Six branding keys were declared in the schema, shipped in every example and read by no code; two of them are worse than clutter, because analytics.ga_tag_id and extra_css could be set, synced, and silently do nothing.
Then deploy.sh, 1146 lines to 609, no behaviour change. --preflight, --destroy, --config-only and --bootstrap-secrets are 341 lines that never run during a deploy, so they move to deploy/modes/ and are sourced -- they still share the loaded instance.env and the log helpers. Comments go from 266 lines to 115, each keeping its rule and its consequence and dropping the account of how it was found. Four lines of dead code go too, and monitoring.googleapis.com is now enabled: the module creates an alert policy and an uptime check, which works today only because the API is usually on already and would fail on a fresh project.
The review fixes already on this branch are untouched -- the utf-8 config read and write, the urlparse prompt-URL derivation and its tests, and the dcproxy TODO. README keeps the test_prompt_urls line that came with them.
Verified here after the port: terraform fmt and validate clean; bash -n clean on deploy.sh, smoke.sh and the four mode files; 45 agent checks across five suites; 108 UI tests in 9 files; tsc --noEmit and vite build clean.
Overview
Clear description of the changes, the problem being solved, and why this
approach was taken.
Related Issues
Fixes # (issue number)
Changes Made
desktop and mobile viewports
Testing Done
Describe the steps you took to test these changes, listing the exact commands
run and reproducible steps a reviewer can follow.
Risk & Rollback
Required for config changes, data migrations, or breaking API changes;
otherwise write "None". What can go wrong in production, and how is this
change rolled back?
Follow-ups
Anything deliberately left out of scope, and where it is tracked. Write "None"
if the change is self-contained.
Checklist
AGENTS.mdand followedCODING_GUIDELINES.md, plusFRONTEND.mdfor UI changes.that application's guide.
Note: Only Maintainers can approve and merge PRs. Expected initial review
time: 3 business days.