pal-e-docs

Review: Critical: Migrate basketball-api Postgres to CNPG (re-review v2)

review-417-2026-03-26-v2 Review

review needs-refinement
Project

Verdict: NEEDS_REFINEMENT

Re-review after refinement v2. All 4 original issues were addressed. One new file target error found that could cause agent-directed data loss if followed literally.

Template Completeness

  • [x] Type — Bug
  • [x] Lineage — standalone, discovered during #184
  • [x] Repo — lists all 3 repos (pal-e-platform, pal-e-deployments, basketball-api)
  • [x] User Story — story:WS-S5, detailed with data counts
  • [x] What Broke — thorough risk assessment with current state
  • [x] Architecture — before/after ASCII diagrams
  • [x] Repro Steps — 4 kubectl commands, all verifiable
  • [x] Expected Behavior — clear target state referencing CNPG capabilities
  • [x] Environment — PG versions, DB size, namespace details
  • [x] File Targets — present (see issues below)
  • [x] Migration Steps — 9-step plan in refinement v2
  • [x] Test Expectations — 5 concrete checks
  • [x] Acceptance Criteria — 8 criteria in refinement v2
  • [x] Constraints — PG version gap, zero-downtime, PVC retention
  • [x] Related — links to #184, project page, restore SOP

Traceability

  • [x] story:WS-S5 label — superadmin backup coverage
  • [x] arch:postgres label — postgres architecture component
  • [x] Forgejo issue — forgejo_admin/pal-e-platform#187, open

File Targets

  • [x] terraform/network-policies.tf (lines 175-179) — verified: postgres namespace ingress allow list exists, basketball-api not yet present. Correct target.
  • [x] ~/pal-e-deployments/overlays/basketball-api/prod/kustomization.yaml — verified: references postgres.yaml resource, needs removal from resources list.
  • [x] ~/pal-e-deployments/overlays/basketball-api/prod/deployment-patch.yaml (line 30) — verified: BASKETBALL_DATABASE_URL points at postgres:5432 (standalone). Needs update to pal-e-postgres-rw.postgres.svc.cluster.local:5432.
  • [x] ~/pal-e-deployments/overlays/basketball-api/prod/postgres.yaml — verified: contains standalone PVC (postgres-data) + Deployment + Service. Correct removal target.
  • [ ] ~/pal-e-deployments/overlays/basketball-api/prod/pvc.yaml — ISSUE: WRONG FILE. Refinement v2 says "REMOVE after verification (standalone PVC)" but this file contains the photo-uploads PVC (1Gi, for player photo uploads), NOT the postgres PVC. The postgres PVC (postgres-data) is defined inside postgres.yaml. Removing pvc.yaml would delete the photo-uploads volume and cause data loss.
  • [x] ~/basketball-api/src/basketball_api/config.py — verified: database_url field with env_prefix = "BASKETBALL_". No code change needed; connection string comes from env var in deployment-patch.

Minor naming inaccuracy (advisory)

Refinement v2 says "update DATABASE_HOST env var" but the actual env var is BASKETBALL_DATABASE_URL (a full connection string, not a separate host var). The intent is clear but an agent reading literally might look for a nonexistent DATABASE_HOST env var and get confused. The correct change is updating the host portion within the BASKETBALL_DATABASE_URL value on deployment-patch.yaml line 30.

Repo Placement

OK. Issue correctly filed on pal-e-platform (network policy lives here). File targets correctly span 3 repos: pal-e-platform (network policy), pal-e-deployments (kustomize overlay), basketball-api (config verification only, no change needed).

Dependencies

  • [x] pal-e-services#33 (CNPG cluster manifest) — RESOLVED. Issue closed, PR #34 merged. Cluster manifest is back under source control.
  • [x] Board item #423 (Re-establish orphaned CNPG cluster manifest) — in done column.
  • No blocking items in in_progress that affect this ticket.

Acceptance Criteria

All 8 criteria in refinement v2 are testable by an agent:
  • [x] SQL database creation — verifiable via psql
  • [x] Network policy — verifiable via kubectl + connectivity test
  • [x] Connection string update — verifiable via kustomize build
  • [x] pg_dump/pg_restore + row counts — verifiable
  • [x] Health check — verifiable via curl
  • [x] Barman backup — verifiable via kubectl get backups
  • [x] Standalone removal — verifiable via kustomize build
  • [x] PVC retention 7 days — clear instruction

Blast Radius

Blast radius already addressed. mcd-tracker (#189) and pal-e-mail (#190) have identical standalone postgres:16-alpine patterns with zero backup coverage. Both issues were filed and are now closed (migrations likely completed). No additional blast radius concerns.

Previous Review Issues (v1) — Resolution Status

  • [x] CNPG creation mechanism — resolved: SQL approach documented in refinement v2
  • [x] Network policy missing from scope — resolved: added to file targets + AC
  • [x] File paths incorrect — partially resolved: overlay path corrected, but pvc.yaml target is wrong (see above)
  • [x] Backup AC misleading — resolved: reworded to match actual CronJob behavior

Recommendation

One fix required before READY:
  • Remove pvc.yaml from file target #5. The postgres PVC (postgres-data) is defined inside postgres.yaml, not in pvc.yaml. pvc.yaml is the photo-uploads PVC and must NOT be removed. Simply removing postgres.yaml from the kustomize resources list (already target #4) handles both the standalone Deployment AND the postgres PVC.
One advisory (non-blocking):
  • Clarify that the env var change is to BASKETBALL_DATABASE_URL (full connection string), not a separate DATABASE_HOST variable. Prevents agent confusion.