fix(gcp-to-aws): stop generated scripts from prescribing a BigQuery data lake on S3 - #202
fix(gcp-to-aws): stop generated scripts from prescribing a BigQuery data lake on S3#202leon1418 wants to merge 7 commits into
Conversation
…ata lake on S3 The design phase's BigQuery specialist gate (design-infra.md Pass 2 step 0) sets `aws_service` to `Deferred — specialist engagement` with `no_automated_aws_target: true` and explicitly forbids naming Athena, Redshift, Glue, EMR, Lake Formation, or a prescribed "data lake on S3" for any `google_bigquery_*` resource. The generate phase then contradicted that: a "BigQuery to S3" block emitting `bq extract --destination_format=PARQUET` plus `aws s3 sync` into a target bucket was gated on `has_databases` — the very predicate the deferral keys on — so a customer told "no automated AWS target" still received the forbidden architecture. It shipped commented out, so nothing executed silently; the defect misguided rather than ran. - Replace the BigQuery export block with a deferral notice that mirrors the `specialist_engagement` wording already used by design-infra.md and generate-artifacts-docs.md (engage AWS account team and/or a data analytics migration partner), so BigQuery users get a next step instead of silence. - Add a `has_bigquery` detection flag (same name/semantics as the existing flag in discover-preview.md) and gate the notice on it. The old block fired for any database resource, so Cloud SQL-only projects were handed a BigQuery data lake plan with no BigQuery present. - Add self-check rule 6 asserting no script prescribes an AWS analytics or warehouse target for BigQuery. `has_databases` semantics are unchanged, so `has_data_migration`, the Cloud SQL and Firestore blocks, and the 05 RDS validation section behave exactly as before.
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Reviewed the complete change and the related discovery, design, and artifact-generation gates. No actionable findings.
Verified the BigQuery predicate matches the established dual-gate semantics, Deferred — specialist engagement is only introduced by the BigQuery specialist gate, executable bq extract guidance has been removed, 37 tests pass, and the Markdown structure remains valid. The change restores the intended safety boundary with minimal scope. Recommended for merge.
|
The fix here is right, but it only covers one of the two copies of this file.
I diffed it against the gcp-to-aws version this PR changes and they're identical on Could you add the same change to that file? Same three edits: the Worth knowing this isn't caught by tooling. Two smaller things, neither blocking:
The second half of the Rest of it checks out: |
…cp-bigquery-deferral-gate-scripts
…isor's copy of the scripts phase The advisor plugin vendors a byte-identical copy of generate-artifacts-scripts.md. The previous commit fixed only the gcp-to-aws copy, so the advisor path still emitted the bq-extract / S3-sync block the specialist gate exists to prevent. Same three edits: the has_bigquery flag, the deferral notice replacing the export block, and quality rule 6. The two files are byte-identical again. Nothing catches this drift automatically: sync-vendored-shared.ts only syncs skills/shared within a single plugin, so the two plugins can disagree without failing shared:check.
…gration scripts has_databases still listed google_bigquery_, so a BigQuery-only project set has_data_migration and generated the Cloud SQL to RDS and Firestore to DynamoDB blocks with no such resources in the design. BigQuery is specialist-deferred and owns no automated data steps — it now counts only toward has_bigquery. A project with BigQuery plus real databases is unaffected (the other prefixes still set the flag). Applied to both vendored copies; files remain byte-identical. Noted by ayn-builds in awslabs#202 review.
|
Both copies now, plus the invited fix: Advisor copy (7bd8947) — same three edits applied to
Predicate breadth — leaving as-is deliberately. You're right it's broader than the name; today |
…cp-bigquery-deferral-gate-scripts
…cripts' into fix/gcp-bigquery-deferral-gate-scripts
|
Note on the two merge commits: #206 moved the advisor's copy ( |
ayn-builds
left a comment
There was a problem hiding this comment.
Re-reviewed at the new path. The advisor half is in and both copies are byte-identical, drift:check is green (250 identical, 25 allowlisted, 0 drift, 0 missing), bq extract has exactly one hit per copy (the prohibition line), and I re-grepped the deferred marker today - Deferred — specialist engagement is still set only by the two BigQuery gates, so the second clause of has_bigquery can't over-trigger. Narrowing has_databases is also right where it's consumed: script 05's block at :369 is an RDS-only check.
Two notes inline, both fallout from the has_databases change I asked for, and neither blocking - the harmful guidance is gone either way and the docs callout at generate-artifacts-docs.md:112 still reaches the customer. I'd land the first one here rather than in a follow-up, since it's one line in a file that's already open and it has to be applied to both copies.
Minor: the description still says "One file changed, +22/−5" and ticks only migrate/ under Team Folder, while the diff is 2 files across both plugins.
| `google_bigtable_`, `google_redis_` (NOT `google_bigquery_` — BigQuery is specialist-deferred | ||
| and carries no automated data-migration steps; see `has_bigquery`) | ||
| - **has_bigquery**: true if ANY resource has `gcp_type` starting with `google_bigquery_` | ||
| OR `aws_service` = "Deferred — specialist engagement" |
There was a problem hiding this comment.
The new flag needs wiring into has_data_migration on line 41, which is still has_databases OR has_storage.
Now that google_bigquery_ is out of has_databases, a BigQuery-only project (BigQuery + Cloud Run, no Cloud SQL/Firestore/Bigtable/Redis, no GCS bucket) has both flags false - so line 81 applies, "Skip this script entirely if has_data_migration is false", and script 02 is never generated. The deferral notice this PR adds never renders for exactly the customer the specialist gate exists to protect. Before the change BigQuery set has_databases = true, so the block always had a script to live in.
One line:
- **has_data_migration**: has_databases OR has_storage OR has_bigquery (used for script 02)
Not blocking - the report callout at generate-artifacts-docs.md:112 fires independently, so the customer still gets the advisory. But it does mean the live-echo rationale in the description ("actually reaches the customer running 02-migrate-data.sh") doesn't hold for the pure-BigQuery case.
| # data warehouse, lake, SQL analytics, or BI cutover planning — query patterns, | ||
| # data volumes, ETL/ELT, and downstream consumers must be assessed by specialists. | ||
| # No BigQuery migration steps are generated here by design. | ||
| echo "BigQuery: AWS target deferred — specialist engagement required." |
There was a problem hiding this comment.
Worth hoisting the preamble in this PR rather than deferring it, because this change makes it reachable.
#!/usr/bin/env bash, set -euo pipefail, and the --execute plumbing all sit inside the Cloud SQL block at :97-98, gated on has_databases. Script 02 has no has_storage subsection at all - its three subsections are Cloud SQL (has_databases), this one (has_bigquery), and Firestore (has_databases).
So a BigQuery + GCS project (no relational DB) now generates an 02-migrate-data.sh that is these echo lines and nothing else: no shebang, no set -euo pipefail, and no dry-run vs --execute distinction. That contradicts Script Quality Rule 1 at :396 - "All scripts use set -euo pipefail" - two lines above the new rule 6.
This is follow-up 2 in your description. It was unreachable while BigQuery forced has_databases = true; it isn't now.
Problem
The design phase has a mandatory BigQuery specialist gate (
references/phases/design/design-infra.md:37-40): for anygcp_typestarting withgoogle_bigquery_, setaws_servicetoDeferred — specialist engagementwithno_automated_aws_target: true, and explicitly do not recommend Athena, Redshift, Glue, EMR, Lake Formation, or a prescribed "data lake on S3".The generate phase then contradicted it.
generate-artifacts-scripts.mdemitted:A customer told "no automated AWS target — engage the account team" was handed a Parquet-export-into-S3 plan. That is not near the edge of the three rules; it is precisely the "data lake on S3" architecture rule 1 forbids.
Mitigation, stated plainly: the block shipped commented out behind
# TODO: Configure. Nothing executed silently. It misguides; it does not run. Hence High-mitigated, not Critical.A second leak, found while fixing this one: the block was gated on
has_databases, which is also true forgoogle_sql_,google_firestore_,google_bigtable_, andgoogle_redis_. So a Cloud SQL-only project with zero BigQuery resources also received a "BigQuery → S3" export plan — prescribing a forbidden analytics architecture to a customer who did not even have the source system.Solution
Both halves of the finding's suggested options were needed; neither alone is sufficient.
has_bigqueryflag alone fails the primary requirement — the same forbidden block would still fire for exactly the customers the gate protects.has_databaseswould tell Cloud SQL-only customers to engage a data-analytics partner about BigQuery they don't have.So: the
bq extractblock is replaced by a specialist-engagement deferral notice, gated on a newhas_bigqueryflag.has_bigqueryis not a new invention —references/phases/discover/discover-preview.md:244already defines it with the same semantics, and the dual predicate (google_bigquery_*oraws_service == "Deferred — specialist engagement") matches the established idiom in three sibling files (generate-artifacts-docs.md:112,generate-infra.md:217,generate-artifacts-infra.md:58). Verified thatDeferred — specialist engagementis set only by the BigQuery gate anywhere in this skill, so the second clause cannot over-trigger on a non-BigQuery deferral.The notice uses live
echolines, not comments-only, so it actually reaches the customer running02-migrate-data.shin both dry-run and--executemode. Its wording is reused from the existing story (design-infra.md:40,generate-artifacts-docs.md:114,clarify.md:441) rather than invented, so the customer isn't handed a competing remediation narrative.has_databasessemantics left unchanged, deliberately.has_bigquery ⇒ has_databases ⇒ has_data_migration ⇒ script 02 exists. Removinggoogle_bigquery_would make a BigQuery-only design skip script 02 entirely — the exact silence this fix exists to prevent.has_databasesalso only decides whether a section is emitted; it never prescribes a target. The prescription lived in the block body, which is what changed.Self-check rule added (rule 6) asserting no script prescribes an AWS analytics or warehouse target for BigQuery.
Swept for sibling leaks
Grepped the whole skill for
bq/bq extract/BigQuery plus every forbidden target. This was the only defect. Everything else is a prohibition, a detection predicate, or unrelated:Redshiftatgenerate-artifacts-scripts.md:31— legitimate: matches anaws_servicea specialist may have chosen and recorded. Blanket-removing it would breakhas_databases. Left alone.pricing-cache.md:333/342(Redshift Serverless, Athena) — inert rate cards; confirmed nothing references them, andestimate-infra.md:148forbids applying them as a projected analytics stack.design-refs/storage.md:111"data lakes" — generic bucket prose, no BigQuery context.generate-artifacts-infra.md:289/291"source-glue" — glue code, false positive.Verification
migrate/plugins/migration-to-aws/tests/→ 37 passed.bq extracthas exactly one hit repo-wide — the new prohibition line itself. Nohas_bigquery-gated block names a forbidden target except to forbid it.has_bigquerydefined once (:33), referenced once (:206);has_databasesretains 4 consumers (:40,:84,:227,:368) with the BigQuery consumer removed — nothing orphaned in either direction.mise run build→ exit 0 (checkov 180/0, gitleaks clean, grype clean, dprint + markdownlint clean).Recommended follow-ups (not in this PR)
02-migrate-data.sh—has_data_migration = has_databases OR has_storage, but every subsection in script 02 is gated onhas_databases, so there is no GCS→S3 step and no shebang. Pre-existing, unrelated to BigQuery.#!/usr/bin/env bash+set -euo pipefailpreamble sits inside the Cloud SQL subsection, not at script scope — so any future narrowing ofhas_databasesyields a header-less script. Worth hoisting.has_databasesgating leaks flavor across engines (a BigQuery-only project still gets Cloud SQLpg_dumpTODOs). Not a gate violation, but the same one-flag-many-engines imprecision that produced this defect; per-engine flags would fix the class.Type of Change
Team Folder
migrate/Checklist
mise run buildlocally and it passesBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.