From c94469ef9dd41d104f67aea4e8ce055500a41509 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 5 May 2026 11:16:43 -0400 Subject: [PATCH 1/2] fix(diff): VPC/Droplet/AppPlatform Diff now compares region (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously only VolumeDriver.Diff compared `region` against current.Outputs["region"]; the other three regional-resource drivers (VPCDriver, DropletDriver, AppPlatformDriver) silently no-op'd on region changes despite DO having no in-place region update API for those resources. ## Bug surfaced by Core-dump TC2 staging cutover (https://github.com/GoCodeAlone/core-dump/actions/runs/25383166075): plan against an infra.yaml whose VPC/Volume/Droplet/App regions moved nyc3 → nyc1. Pre-cutover DO state confirmed all 5 resources in nyc3. Plan output: only Volume got a `replace` action; VPC and Droplet were absent from the plan entirely; App got `update` (should be replace). The plan-shape assertion in core-dump's tc2-cutover.yml caught the discrepancy before any cloud mutation. Without the assertion gate, apply would have updated the firewall + App in nyc1 and replaced the Volume to nyc1, while leaving VPC + Droplet in nyc3 — producing a half-migrated state requiring manual cleanup (App's vpc_ref pointing at nyc3 VPC, Volume in nyc1 unable to mount on a nyc3 Droplet, etc.). ## Fix Add the canonical region-comparison block (mirroring VolumeDriver.Diff line 165-173) to the three affected drivers: - internal/drivers/vpc.go::VPCDriver.Diff - internal/drivers/droplet.go::DropletDriver.Diff - internal/drivers/app_platform.go::AppPlatformDriver.Diff Each emits `FieldChange{Path: "region", ForceNew: true}` and sets `NeedsReplace=true` when desired.region differs from current.region. ## Upgrade-safe guard The pattern guards on `curRegion != ""` so resources in state from earlier plugin versions (when their respective output functions didn't include region) do not false-positive on the first plan after upgrade. They will Read on next apply to populate the field naturally. dropletOutput + vpcOutput already populate `region`. appOutput did NOT — this PR also adds `region: app.Spec.Region` to the appOutput map so the Diff comparison has a value to compare against. ## Tests Added regression tests for each driver: - TestVPCDriver_Diff_RegionChangeForcesReplace + RegionEmptyCurrentSkipped - TestDropletDriver_Diff_RegionChangeForcesReplace + RegionEmptyCurrentSkipped - TestAppPlatformDriver_Diff_RegionChangeForcesReplace + RegionEmptyCurrentSkipped Each pair covers (a) the change-detection path (current=nyc3, desired=nyc1, asserts ForceNew + NeedsReplace) and (b) the upgrade-safe skip path (current.Outputs missing "region", asserts no false-positive drift). ## Validation - `go build ./...` (GOWORK=off): clean - `go test -count=1 ./internal/drivers/`: all 6.1s pass - `go test -count=1 ./...`: all packages pass ## Scope Only the 3 Diff fixes + their regression tests + appOutput addition + CHANGELOG entry. No other Diff changes; no public API changes; no .github/workflows touch. Per upstream issue #70 scope. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 4 ++ internal/drivers/app_platform.go | 30 ++++++++++- internal/drivers/app_platform_test.go | 71 ++++++++++++++++++++++++++ internal/drivers/droplet.go | 15 ++++++ internal/drivers/droplet_test.go | 72 +++++++++++++++++++++++++++ internal/drivers/vpc.go | 16 ++++++ internal/drivers/vpc_test.go | 59 ++++++++++++++++++++++ 7 files changed, 266 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb2a132..6113f47 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to workflow-plugin-digitalocean are documented here. ## [Unreleased] +### Fixed + +- **VPCDriver / DropletDriver / AppPlatformDriver `Diff` now compares `region`** (#70) — previously only `VolumeDriver.Diff` checked region drift; the other three drivers silently no-op'd on region changes despite DO having no in-place region update on those resources. Region change now correctly emits `FieldChange{ForceNew: true}` and sets `NeedsReplace=true`. Surfaced by core-dump's TC2 cutover plan-shape assertion which expected a 5-resource cascade replace but got 1 (Volume only); without the assertion gate the partial cutover would have left VPC + Droplet in nyc3 while the App + Volume moved to nyc1, producing a half-migrated state requiring manual cleanup. Includes upgrade-safe guard: `appOutput`, `vpcOutput`, `dropletOutput` already populate `region`, but state from earlier plugin versions may not — the new check skips when `current.Outputs["region"]` is empty so the next Read populates it without false-positive drift. AppPlatformDriver also adds `region` to `appOutput.Outputs` for the same reason. Regression tests cover both the change-detection and empty-current-skip paths for all three drivers. + ## [v0.10.0] ### Added diff --git a/internal/drivers/app_platform.go b/internal/drivers/app_platform.go index 7643f00..a4d4dd9 100644 --- a/internal/drivers/app_platform.go +++ b/internal/drivers/app_platform.go @@ -209,7 +209,30 @@ func (d *AppPlatformDriver) Diff(_ context.Context, desired interfaces.ResourceS if desiredExpose != curExpose { changes = append(changes, interfaces.FieldChange{Path: "expose", Old: curExpose, New: desiredExpose}) } - return &interfaces.DiffResult{NeedsUpdate: len(changes) > 0, Changes: changes}, nil + + // region: App Platform's UpdateApp does not accept region changes — + // region is a Create-only field on godo.AppSpec, so any drift forces + // replace. Mirror VolumeDriver.Diff's region pattern. Guard + // curRegion != "" so apps in state from earlier plugin versions + // (when appOutput didn't include region) don't false-positive on + // the first plan after upgrade — they'll Read on next apply to + // populate the field. + var needsReplace bool + if region := strFromConfig(desired.Config, "region", ""); region != "" { + curRegion, _ := current.Outputs["region"].(string) + if curRegion != "" && curRegion != region { + changes = append(changes, interfaces.FieldChange{ + Path: "region", Old: curRegion, New: region, ForceNew: true, + }) + needsReplace = true + } + } + + return &interfaces.DiffResult{ + NeedsUpdate: len(changes) > 0, + NeedsReplace: needsReplace, + Changes: changes, + }, nil } // canonicalExpose returns the canonical `expose` value for a desired-spec @@ -768,6 +791,11 @@ func appOutput(app *godo.App) *interfaces.ResourceOutput { // desired `cfg["image"]` and avoid emitting spurious image // FieldChanges on no-op reconciles — F4 round-3 Finding A. "image": deriveImageFromAppSpec(app.Spec), + // `region` is the App Platform region from AppSpec.Region. + // Stored on Outputs so Diff can detect region drift and + // surface it as ForceNew (App Platform's UpdateApp does not + // accept region changes — region is a Create-only field). + "region": app.Spec.Region, }, Status: "running", } diff --git a/internal/drivers/app_platform_test.go b/internal/drivers/app_platform_test.go index 499ffb8..64fed6b 100644 --- a/internal/drivers/app_platform_test.go +++ b/internal/drivers/app_platform_test.go @@ -371,6 +371,77 @@ func TestAppPlatformDriver_Diff_NoChanges(t *testing.T) { } } +// TestAppPlatformDriver_Diff_RegionChangeForcesReplace covers issue #70: +// App Platform region is a Create-only field on godo.AppSpec — UpdateApp +// does not accept region changes. Region drift must surface as ForceNew so +// dependents (vpc_ref to a region-locked VPC) get correctly recreated too. +func TestAppPlatformDriver_Diff_RegionChangeForcesReplace(t *testing.T) { + mock := &mockAppClient{} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "region": "nyc3", + }, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsReplace { + t.Fatal("region change must force replace; NeedsReplace=false") + } + var found bool + for _, c := range result.Changes { + if c.Path == "region" && c.Old == "nyc3" && c.New == "nyc1" && c.ForceNew { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange{Path:region, Old:nyc3, New:nyc1, ForceNew:true}; got %+v", result.Changes) + } +} + +// TestAppPlatformDriver_Diff_RegionEmptyCurrentSkipped covers the +// upgrade-safe guard: Apps in state from earlier plugin versions (when +// appOutput didn't include region) must not false-positive on the first +// plan after upgrade — they'll Read on next apply to populate the field. +func TestAppPlatformDriver_Diff_RegionEmptyCurrentSkipped(t *testing.T) { + mock := &mockAppClient{} + d := drivers.NewAppPlatformDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + // no region — represents pre-region-output state + }, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "image": "registry.digitalocean.com/myrepo/myapp:v1", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if result.NeedsReplace { + t.Error("empty curRegion should not force replace; got NeedsReplace=true") + } + for _, c := range result.Changes { + if c.Path == "region" { + t.Errorf("empty curRegion should not emit a region change; got %+v", c) + } + } +} + // TestAppPlatformDriver_Diff_DetectsExposeChange covers quality-review Finding // 1: changing `expose` (a security-relevant toggle) on an existing service // must produce a Plan action — Diff cannot silently no-op the way the diff --git a/internal/drivers/droplet.go b/internal/drivers/droplet.go index 88cad46..7cfe9fc 100644 --- a/internal/drivers/droplet.go +++ b/internal/drivers/droplet.go @@ -223,6 +223,21 @@ func (d *DropletDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, _ = boolFromConfig(desired.Config, "ipv6", false) // see TODO above // ssh_keys: parsed in Create; not re-parsed here to avoid wasted work. + // region: Droplets are regional and DO does not support region change + // in Update. Mirror VolumeDriver.Diff's region pattern: any region + // drift forces replace. Guard curRegion != "" so Droplets in state + // from earlier plugin versions (when dropletOutput didn't include + // region) don't false-positive on the first plan after upgrade — + // they'll Read on next apply to populate the field. + if region := strFromConfig(desired.Config, "region", ""); region != "" { + curRegion, _ := current.Outputs["region"].(string) + if curRegion != "" && curRegion != region { + changes = append(changes, interfaces.FieldChange{ + Path: "region", Old: curRegion, New: region, ForceNew: true, + }) + } + } + return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0, NeedsReplace: len(changes) > 0, diff --git a/internal/drivers/droplet_test.go b/internal/drivers/droplet_test.go index 5ac4ddf..89dfc3c 100644 --- a/internal/drivers/droplet_test.go +++ b/internal/drivers/droplet_test.go @@ -965,6 +965,78 @@ func TestDropletDriver_Create_Volumes_EmptyStringRejected(t *testing.T) { } } +// TestDropletDriver_Diff_RegionChangeForcesReplace covers issue #70: +// Droplets are regional and DO does not support region change in Update +// (godo Droplet PUT only resizes). Region drift must surface as ForceNew +// so cascade dependents (Volume mount, Firewall tags) get correctly +// re-coordinated against the replaced Droplet. +func TestDropletDriver_Diff_RegionChangeForcesReplace(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + "region": "nyc3", + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !r.NeedsReplace { + t.Fatal("region change must force replace; NeedsReplace=false") + } + var found bool + for _, c := range r.Changes { + if c.Path == "region" && c.Old == "nyc3" && c.New == "nyc1" && c.ForceNew { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange{Path:region, Old:nyc3, New:nyc1, ForceNew:true}; got %+v", r.Changes) + } +} + +// TestDropletDriver_Diff_RegionEmptyCurrentSkipped covers the upgrade-safe +// guard: Droplets in state from earlier plugin versions (when dropletOutput +// didn't include region) must not false-positive on the first plan after +// upgrade — they'll Read on next apply to populate the field. +func TestDropletDriver_Diff_RegionEmptyCurrentSkipped(t *testing.T) { + mock := &mockDropletClient{} + d := drivers.NewDropletDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{ + "size": "s-1vcpu-2gb", + // no region — represents pre-region-output state + }, + } + r, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{ + "size": "s-1vcpu-2gb", + "region": "nyc1", + }, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if r.NeedsReplace { + t.Error("empty curRegion should not force replace; got NeedsReplace=true") + } + for _, c := range r.Changes { + if c.Path == "region" { + t.Errorf("empty curRegion should not emit a region change; got %+v", c) + } + } +} + func contains(s, sub string) bool { if len(sub) == 0 { return true diff --git a/internal/drivers/vpc.go b/internal/drivers/vpc.go index 53fb031..d20ecd7 100644 --- a/internal/drivers/vpc.go +++ b/internal/drivers/vpc.go @@ -151,6 +151,22 @@ func (d *VPCDriver) Diff(_ context.Context, desired interfaces.ResourceSpec, cur }) } } + + // region: VPCs are regional and DO does not support region change in + // Update. Mirror VolumeDriver.Diff's region pattern: any region drift + // forces replace. Guard curRegion != "" so VPCs in state from earlier + // plugin versions (when vpcOutput didn't include region) don't false- + // positive on the first plan after upgrade — they'll Read on next + // apply to populate the field. + if region := strFromConfig(desired.Config, "region", ""); region != "" { + curRegion, _ := current.Outputs["region"].(string) + if curRegion != "" && curRegion != region { + changes = append(changes, interfaces.FieldChange{ + Path: "region", Old: curRegion, New: region, ForceNew: true, + }) + } + } + return &interfaces.DiffResult{ NeedsUpdate: len(changes) > 0, NeedsReplace: len(changes) > 0, diff --git a/internal/drivers/vpc_test.go b/internal/drivers/vpc_test.go index b974896..2548ba3 100644 --- a/internal/drivers/vpc_test.go +++ b/internal/drivers/vpc_test.go @@ -217,6 +217,65 @@ func TestVPCDriver_Diff_IPRangeChange(t *testing.T) { } } +// TestVPCDriver_Diff_RegionChangeForcesReplace covers issue #70: VPCs are +// regional and DO does not support region change in Update. Region drift +// must surface as ForceNew so cascade dependents (Droplet vpc_uuid, App +// vpc_ref) are correctly replaced too. +func TestVPCDriver_Diff_RegionChangeForcesReplace(t *testing.T) { + mock := &mockVPCClient{vpc: testVPC()} + d := drivers.NewVPCDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{"ip_range": "10.20.0.0/16", "region": "nyc3"}, + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"ip_range": "10.20.0.0/16", "region": "nyc1"}, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if !result.NeedsReplace { + t.Fatal("expected NeedsReplace for region change") + } + var found bool + for _, c := range result.Changes { + if c.Path == "region" && c.Old == "nyc3" && c.New == "nyc1" && c.ForceNew { + found = true + break + } + } + if !found { + t.Errorf("expected FieldChange{Path:region, Old:nyc3, New:nyc1, ForceNew:true}; got %+v", result.Changes) + } +} + +// TestVPCDriver_Diff_RegionEmptyCurrentSkipped covers the upgrade-safe +// guard: VPCs in state from earlier plugin versions (when vpcOutput +// didn't include region) must not false-positive on the first plan +// after upgrade — they'll Read on next apply to populate the field. +func TestVPCDriver_Diff_RegionEmptyCurrentSkipped(t *testing.T) { + mock := &mockVPCClient{vpc: testVPC()} + d := drivers.NewVPCDriverWithClient(mock, "nyc3") + + current := &interfaces.ResourceOutput{ + Outputs: map[string]any{"ip_range": "10.20.0.0/16"}, // no region + } + result, err := d.Diff(context.Background(), interfaces.ResourceSpec{ + Config: map[string]any{"ip_range": "10.20.0.0/16", "region": "nyc1"}, + }, current) + if err != nil { + t.Fatalf("Diff: %v", err) + } + if result.NeedsReplace { + t.Error("empty curRegion should not force replace; got NeedsReplace=true") + } + for _, c := range result.Changes { + if c.Path == "region" { + t.Errorf("empty curRegion should not emit a region change; got %+v", c) + } + } +} + func TestVPCDriver_SupportsUpsert(t *testing.T) { d := drivers.NewVPCDriverWithClient(&mockVPCClient{}, "nyc3") if !d.SupportsUpsert() { From f6dcd8170033e4bb0e6e1a00dab354279e5e5752 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 5 May 2026 11:42:21 -0400 Subject: [PATCH 2/2] docs(changelog): clarify appOutput region addition (PR #71 Copilot review) Copilot caught a contradiction in the CHANGELOG entry: the original wording said "appOutput, vpcOutput, dropletOutput already populate region" but this PR is also ADDING region to appOutput.Outputs. Reworded to: vpcOutput + dropletOutput already had it; appOutput gets it in this PR. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6113f47..44b29d9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to workflow-plugin-digitalocean are documented here. ### Fixed -- **VPCDriver / DropletDriver / AppPlatformDriver `Diff` now compares `region`** (#70) — previously only `VolumeDriver.Diff` checked region drift; the other three drivers silently no-op'd on region changes despite DO having no in-place region update on those resources. Region change now correctly emits `FieldChange{ForceNew: true}` and sets `NeedsReplace=true`. Surfaced by core-dump's TC2 cutover plan-shape assertion which expected a 5-resource cascade replace but got 1 (Volume only); without the assertion gate the partial cutover would have left VPC + Droplet in nyc3 while the App + Volume moved to nyc1, producing a half-migrated state requiring manual cleanup. Includes upgrade-safe guard: `appOutput`, `vpcOutput`, `dropletOutput` already populate `region`, but state from earlier plugin versions may not — the new check skips when `current.Outputs["region"]` is empty so the next Read populates it without false-positive drift. AppPlatformDriver also adds `region` to `appOutput.Outputs` for the same reason. Regression tests cover both the change-detection and empty-current-skip paths for all three drivers. +- **VPCDriver / DropletDriver / AppPlatformDriver `Diff` now compares `region`** (#70) — previously only `VolumeDriver.Diff` checked region drift; the other three drivers silently no-op'd on region changes despite DO having no in-place region update on those resources. Region change now correctly emits `FieldChange{ForceNew: true}` and sets `NeedsReplace=true`. Surfaced by core-dump's TC2 cutover plan-shape assertion which expected a 5-resource cascade replace but got 1 (Volume only); without the assertion gate the partial cutover would have left VPC + Droplet in nyc3 while the App + Volume moved to nyc1, producing a half-migrated state requiring manual cleanup. Includes upgrade-safe guard: `vpcOutput` and `dropletOutput` already populated `region`, and this PR adds `region` to `appOutput.Outputs` so AppPlatformDriver.Diff has a current-side value to compare; the new check then skips when `current.Outputs["region"]` is empty so state from earlier plugin versions doesn't false-positive — the next Read populates it without spurious drift. Regression tests cover both the change-detection and empty-current-skip paths for all three drivers. ## [v0.10.0]