Skip to content

feat(opdb) --dry-run mode for OPDB sync via OpdbSyncMode enum#71

Merged
jkeeley2073 merged 1 commit into
mainfrom
Dev-OpdbDryRun
May 4, 2026
Merged

feat(opdb) --dry-run mode for OPDB sync via OpdbSyncMode enum#71
jkeeley2073 merged 1 commit into
mainfrom
Dev-OpdbDryRun

Conversation

@jkeeley2073
Copy link
Copy Markdown
Contributor

Summary

docs/build-spec.md Phase 2 § Scope item 4 — code half (Wave 2). The operational hand-off half (actual run against deployed Cosmos with env-var setup, dry-run pass, real run, Cosmos state verification) closes item 4 fully after this PR merges.

IOpdbSyncService.SyncAsync now takes an OpdbSyncMode enum (Apply / DryRun) in place of the previous parameterless signature. In DryRun mode the service:

  • Fetches the full OPDB catalog (same as Apply mode)
  • Reads existing machines via IMachineRepository.GetByOpdbIdAsync (required to distinguish projected-insert from projected-update counts)
  • Runs OpdbMachineMapper.MergeOpdbFieldsInto on existing machines (in-memory; the mutated existing is discarded by the GC, but performing the merge confirms the mapping itself doesn't throw on real OPDB data)
  • Skips _machines.UpsertAsync calls — no Cosmos writes
  • Returns the same OpdbSyncResult shape; counters represent projected writes in dry-run mode

The CLI handler for --source opdb threads the existing --dry-run flag through and emits a distinct output line in dry-run mode ("OPDB sync (DRY RUN — no writes): fetched X, would-insert Y, would-update Z, skipped W, duration N.Ns").

Why an enum, not a bool

/local-review flagged a bool dryRun first draft as a "boolean trap" — opaque at call sites (sync.SyncAsync(true, ct) is mystery-flag), and leaves no room for a future Validate-only mode without another breaking change. The interface change is breaking this PR regardless, so switching to an enum now is the cheapest moment.

Test Plan

  • dotnet test PinballWizard.slnx --nologo518 / 518 passing (was 516; +2 new dry-run tests). The 3 existing OpdbSyncService tests were updated to pass OpdbSyncMode.Apply explicitly.
  • dotnet build PinballWizard.slnx --nologo → clean, zero warnings.
  • The new test SyncAsync_DryRunExistingMachine_ProjectsUpdateWithoutUpserting asserts both _repository.DidNotReceive().UpsertAsync(...) AND existing.Title == "Stranger Things" (after the call) — the second assertion pins the documented contract that merge runs in dry-run mode, so a future refactor wrapping the merge in if (mode != DryRun) will break the test and force a re-read.

Local review

/local-review outcome: 0 🔴 / 2 ⚠️ / 8 categories ✅.

  • ⚠️ Fixedbool dryRun first draft replaced with OpdbSyncMode enum (boolean-trap finding).
  • ⚠️ Fixed — added the merge-runs-in-dry-run behavioral assertion to SyncAsync_DryRunExistingMachine_* (test-quality finding).

The reviewer's caller-search verification confirmed only Program.cs:192 and OpdbSyncServiceTests.cs (5 sites) call SyncAsync — all updated. No missed call sites.

Operational hand-off (closes Phase 2 § Scope item 4)

After this PR merges:

  1. Set the env vars (PowerShell, not Git-Bash, per the friendly-error guard in ArmCosmosProvisioner):
    $env:Cosmos__AccountEndpoint   = az cosmosdb show -n pinwiz-cosmos-dev-hlpz4 -g rg-pinwiz-shared-dev --query documentEndpoint -o tsv
    $env:Cosmos__AccountResourceId = az cosmosdb show -n pinwiz-cosmos-dev-hlpz4 -g rg-pinwiz-shared-dev --query id              -o tsv
    $env:Opdb__BaseUrl             = "https://opdb.org/api/"
    $env:Opdb__ApiToken            = "<your-OPDB-token>"
  2. Dry-run pass: dotnet run --project src/PinballWizard.Cli -- --source opdb --dry-run
    Confirm the OPDB sync (DRY RUN — no writes) summary line shows non-zero fetched and reasonable would-insert / would-update counts.
  3. Real run: dotnet run --project src/PinballWizard.Cli -- --source opdb
    Confirm the OPDB sync summary line shows actual writes. Spot-check 5 random machines in the Cosmos machines container for full provenance.
  4. Capture dry-run + real-run output in the Phase 2 retrospective.

7-item self-audit

  1. Every option field is read — N/A (no new options).
  2. Sibling-diff — diffed against IngestionSourceSeeder (no shared sibling for "discover-then-conditionally-write" except the scraper --dry-run flag, which has the same semantic). Confirmed by /local-review reviewer's caller search.
  3. No bare catch { } — no new catches added.
  4. CLI / orchestrator wiring--source opdb --dry-run exercised by manual trace; existing tests cover the orchestrator dispatch path.
  5. Tests assert behavior — both new dry-run tests assert the no-write invariant + the merge-still-runs invariant; the latter pins the load-bearing contract.
  6. Build is zero-warning — verified.
  7. Identity checkgit log -1 --format='%an <%ae>'Jim Keeley <94459922+jkeeley2073@users.noreply.github.com>

Out of Scope

  • Item 4 operational hand-off — happens after merge, captured against Phase 2 § Exit criteria.
  • Items 6 (Playwright bump) and 7 round 2 (clean Dependabot bumps) — separate PRs.

🤖 Generated with Claude Code

Phase 2 § Scope item 4 code half (Wave 2). The operational hand-off
half (actual run against deployed Cosmos with env-var setup, dry-run
pass, real run, Cosmos state verification) closes item 4 fully after
this PR merges.

What ships:

- IOpdbSyncService.SyncAsync now takes an OpdbSyncMode enum
  { Apply, DryRun } in place of the previous parameterless signature.
  /local-review flagged a `bool dryRun` first draft as a "boolean
  trap" — opaque at call sites, leaves no room for a future
  Validate/Schema-only mode without another breaking change. Enum
  is self-documenting at every call site (sync.SyncAsync(
  OpdbSyncMode.DryRun, ct) reads as English).
- OpdbSyncService skips _machines.UpsertAsync calls in DryRun mode
  but still runs reads (required to distinguish projected-insert
  from projected-update counts) and the in-place merge (which
  exercises the mapping against real OPDB data and would surface
  any DTO-shape regression even before any write).
- CLI handler for --source opdb threads --dry-run through to the
  new mode parameter and emits a distinct output line in dry-run
  mode: "OPDB sync (DRY RUN — no writes): fetched X, would-insert
  Y, would-update Z, skipped W, duration N.Ns".

Local review summary: 0 🔴 / 2 ⚠️ / 8 categories ✅.

⚠️ — Fixed: bool dryRun replaced with OpdbSyncMode enum (per the
review's "boolean trap" finding; the interface change is breaking
this PR anyway, so making it an enum now costs nothing extra and
prevents another breaking change for a future Validate mode).

⚠️ — Fixed: added Assert.Equal("Stranger Things", existing.Title) +
Assert.Equal(NowFixed, existing.LastSeenAt) to the dry-run-existing
test, asserting the merge actually ran in dry-run mode. The XML doc
on OpdbSyncMode.DryRun documents that reads + merge happen even in
dry-run; the test now pins that contract. If a future refactor wraps
the merge in `if (mode != DryRun)`, this test fails — forcing a
re-read of the contract.

Tests: 518 / 518 passing (2 new dry-run tests added; 3 existing
tests updated to pass OpdbSyncMode.Apply explicitly). Build clean,
zero warnings.

After merge — operational hand-off (closes Phase 2 § Scope item 4):
1. Set Cosmos__AccountEndpoint + Cosmos__AccountResourceId +
   Opdb__BaseUrl + Opdb__ApiToken env vars (PowerShell, not
   Git-Bash, per the friendly-error guard in ArmCosmosProvisioner)
2. Run: dotnet run --project src/PinballWizard.Cli --
   --source opdb --dry-run
   Confirm the "OPDB sync (DRY RUN — no writes)" summary line
   includes a non-zero fetched count and reasonable inserted/updated
   projection.
3. Run: dotnet run --project src/PinballWizard.Cli -- --source opdb
   Confirm the "OPDB sync" summary line shows actual writes; spot-
   check 5 random machines in the Cosmos `machines` container for
   full provenance.
4. Capture the dry-run + real-run output in the Phase 2 retrospective.
@jkeeley2073 jkeeley2073 added the claude-code Generated with Claude Code label May 4, 2026
MachineJson("GRBN-MQR4P", manufacturer: "Stern Pinball, Inc.", name: "Stranger Things (Pro)", commonName: "Stranger Things"),
MachineJson("XYZ", manufacturer: "Jersey Jack Pinball", name: "Wonka", commonName: "Wonka")));

_repository.GetByOpdbIdAsync("GRBN-MQR4P", "stern", Arg.Any<CancellationToken>()).Returns((Machine?)null);
MachineJson("XYZ", manufacturer: "Jersey Jack Pinball", name: "Wonka", commonName: "Wonka")));

_repository.GetByOpdbIdAsync("GRBN-MQR4P", "stern", Arg.Any<CancellationToken>()).Returns((Machine?)null);
_repository.GetByOpdbIdAsync("XYZ", "jjp", Arg.Any<CancellationToken>()).Returns((Machine?)null);
@jkeeley2073 jkeeley2073 merged commit c452b53 into main May 4, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

claude-code Generated with Claude Code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants