feat(gcp-to-aws): last-mile usability — fill-in checklist, placeholder guards, rollback runbook - #184
feat(gcp-to-aws): last-mile usability — fill-in checklist, placeholder guards, rollback runbook#184herosjourney wants to merge 6 commits into
Conversation
…laceholder guards, rollback runbook Evidence-based from a real run's outputs: 20 TODOs scattered across 10 files with only a grep hint; terraform validate 'passed' while TODO-ops@example.com would apply as the account security contact; the entire rollback story was one log line. - docs: README 'TODO Items' grep hint becomes a consolidated Fill-in Checklist — one row per value (not per occurrence), each with a concrete 'where to get it' command or console path; mirrored in MIGRATION_GUIDE Prerequisites; self-check now verifies checklist completeness - infra: contact emails become fill-once variables (no default) referenced by alternate contacts AND budget alerts; REQUIRED validation blocks reject placeholder tokens so failure happens at terraform plan, not silently at apply; backend ACCOUNT_ID (can't use vars) covered by the checklist row - docs: rollback subsection must contain triggers with concrete criteria, exact DNS commands, an explicit answer to the post-cutover-writes data question, and RTO — never a one-line 'rollback via DNS' - scripts: at --execute, check all user-supplied env vars up front and exit listing every missing one; dry-run runs without them - report: first Next Step states execution readiness (N values to fill) when placeholders exist — bridges decision to execution
leon1418
left a comment
leon1418
left a comment
There was a problem hiding this comment.
[🤖 AI review 🤖]
Summary
Evidence-driven last-mile usability fix for generated migration artifacts. The three problems are real, the fixes are well-scoped, and each one converts a soft convention into a machine-enforced failure at the earliest safe moment. Design is sound.
4 files, +61/−13. No schema, flow, or gate changes. CI clean (build + 4 security scanners). MERGEABLE but BEHIND main.
Findings: 1 Nit, 1 Optional
Both are non-blocking. No design, functionality, security, or correctness issues found.
Sibling PR interactions
- #181 touches
generate-artifacts-scripts.mdat lines 83–130 (db_size cross-check + runtime measurement). #184 adds at lines 62–76 (fail-fast env var check in the Script Rules section). No textual conflict — different hunks, different sections. Either merge order works. - #186 heavily refactors
generate-artifacts-report.md(replaces lines 36–280 with a 43-line delegation toreport-decision-core.md). #184's one-line change at line 100 of that file ("Next steps" execution-readiness bridge) will textually conflict — the paragraph #184 appends to is deleted by #186 and the rule moved elsewhere. Whichever merges second must rebase; the intent should be preserved insidereport-decision-core.md. - #186 also modifies
generate-artifacts-docs.md(TCO→run-rate, timeline→migration-shape). #184's checklist and rollback sections are in non-overlapping hunks. Low conflict risk but the "Timeline" reference at line 357 (unchanged by #184) will be renamed by #186. - #180, #182, #183, #185 touch no files in common with #184.
Merge order recommendation
Merge #184 before #186 (the report file refactor). #184's one-line report change is trivial to preserve during #186's rebase, but rebasing the inverse (large deletion then re-add) is more error-prone.
Validation gaps
- The manual test plan item (regenerate artifacts, verify checklist count, test
terraform planrejection, verify rollback completeness) is unchecked. No automated asserter covers the new checklist-completeness self-check rule or the rollback contract. - The
strcontainsdependency is safe — the spec already requires>= 1.5.0.
| if [ "$DRY_RUN" = false ]; then | ||
| missing=() | ||
| for v in SOURCE_HOST TARGET_HOST TARGET_DB_PASSWORD; do | ||
| [ -z "${!v:-}" ] && missing+=("$v") |
There was a problem hiding this comment.
[🤖 AI review 🤖]
Nit: The indirect expansion ${!v:-} is valid bash 4+ (confirmed empirically), but the script template also specifies set -euo pipefail which makes unset variables fatal via -u. With nounset active, ${!v:-} on an unset variable expands to empty string safely (the :- default handles it) — so this is fine. However, the comment list SOURCE_HOST TARGET_HOST TARGET_DB_PASSWORD is an example — the actual variable list varies per script. Consider adding a one-line note like # Adjust the list above to match this script's required env vars to make it obvious this is a template to copy-edit, not a universal constant.
| @@ -345,7 +364,7 @@ After generating documentation, verify: | |||
| 1. **Commands are syntactically correct**: All bash commands use correct syntax | |||
| 1. **No unresolved placeholders**: All `[placeholder]` values are replaced with actual data from artifacts | |||
There was a problem hiding this comment.
[🤖 AI review 🤖]
Optional: The self-check item 8 still reads "Rollback instructions match: Rollback steps match generation plan" — unchanged by this PR. Given the substantial new rollback contract (triggers, exact commands, data-question posture, RTO), this self-check assertion is now underpowered. It could be strengthened to verify the four MUST-contain elements are present, not just that "steps match" generically. Not blocking because the rollback spec itself is the enforcing mechanism and this is a pre-existing check, but the asymmetry between the detailed contract (line 178–183) and the vague self-check (line 369) may let an incomplete rollback section slip through.
… self-check Note that the execute-path env-var list is per-script copy-edit, and require the docs self-check to verify all four rollback-contract elements (triggers, exact commands, data posture, RTO) rather than a generic "steps match". Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed both review notes: Nit (scripts fail-fast pattern): Added an explicit note that the Optional (rollback self-check): Strengthened item 8 to require the four MUST-contain contract elements (triggers, exact commands + TTL, post-cutover data posture, RTO) and to fail a one-line "rollback via DNS". |
2 participants
Summary
The problem, in plain English: Everything before Generate produces decisions; Generate produces the things a founder actually runs against production — and that last mile has three traps, all observed in a real run's outputs rather than hypothesized. First: the run left 20 account-specific TODOs scattered across 10 files, and the only navigation aid is a
grepcommand in the README — miss one and consequences range from broken (ACCOUNT_IDin the backend bucket name) to silently wrong. Second, the trap inside the first:validation-report.jsonsaid passed whileTODO-ops@example.comsat in the config as the AWS account's security contact —terraform validatechecks syntax, soapplywould ship example.com contacts and a broken ECR URI without a word. Third: the generated cutover script's entire rollback story was one log line ("rollback = restore DNS to Cloud Run") — at the highest-stress, lowest-judgment moment of the migration, with an unanswered data question underneath it (rows written to the AWS database after cutover don't exist in Cloud SQL; a bare DNS rollback silently loses them).Changes (all in
references/phases/generate/):generate-artifacts-docs.md): the README's "TODO Items" grep hint becomes a consolidated table — one row per value (the same billing email in four places is one row), each with file locations and a concrete "where to get it" (command, console path, or artifact reference — never "your value here"). Mirrored in MIGRATION_GUIDE's Prerequisites; the grep one-liner stays as a done-check ("should return nothing"). Self-check now verifies every placeholder in the artifacts appears in the checklist.generate-artifacts-infra.md): contact emails becomevar.operations_email/var.billing_email/var.security_email(no default), referenced by both the alternate contacts and the budget-alert subscribers — each value entered exactly once in tfvars instead of edited in four places. Every non-inferable variable gets a REQUIREDvalidationblock rejecting placeholder tokens (TODO,example.com,ACCOUNT_ID,<), with error messages that name the tfvars key and point at the checklist — so the failure happens loudly atterraform plan, not silently atapply. The backend block can't use variables (Terraform limitation), so itsACCOUNT_IDis covered by a checklist row.generate-artifacts-docs.md): the Rollback Procedure subsection must contain (1) triggers with concrete criteria tied to04-validate.shchecks, (2) exact DNS commands with TTL implications, (3) an explicit answer to the post-cutover-writes data question — write-freeze + delta export, accepted loss (viable when Q12 said low-write; say so), or reverse sync by name — and (4) RTO. A one-line "rollback via DNS" fails the docs self-check ("rollback instructions match" was already there; now there's something substantive to match).generate-artifacts-scripts.md): at--execute, scripts check all user-supplied env vars up front and exit listing every missing one at once — never dying midway on the first empty var after work has started. Dry-run continues to work without them.generate-artifacts-report.md): when artifacts contain placeholders, the report's first Next Step states "Fill in the [N] account-specific values — checklist in MIGRATION_GUIDE.md." The report is the decision document; this is the one line bridging decision to execution.Why
Companion to #181/#182/#183, which hardened the decision pipeline (extraction gates, resume/recovery, orientation + answer recap). This PR is where those decisions meet production. The placeholder-guard change carries the same philosophy as #181's gate checks — convert soft conventions ("go find the TODOs") into machine-enforced failures at the earliest safe moment (
terraform plan) — and the checklist follows the same form-UX principle as #183 ("here's everything you need, in one place, before you start"). All three top findings came from inspecting an actual run's generated artifacts, not from reading the spec.Test plan
dprint check— cleanmise run lint:md— 0 errorsmise run lint:frontmatter— OKvalidationblock HCL usesstrcontains(Terraform ≥ 1.5, matches the plugin's>= 1.0floor via required_version bump not needed — function available in all currently supported Terraform releases)grep -rc TODO, (b)terraform planfails namingbilling_emailwhen tfvars keeps the placeholder, (c) the rollback subsection answers the post-cutover-writes questionOut of scope
generate-artifacts-scripts.md(different sections of the file; whichever merges second rebases trivially)Type of Change
Team Folder
migrate/