Skip to content

feat(provider): migrate to v2 IaC dispatch (W-7/W-8 conformance + ApplyPlan)#61

Merged
intel352 merged 11 commits intomainfrom
feat/pdo-v2-migration
May 5, 2026
Merged

feat(provider): migrate to v2 IaC dispatch (W-7/W-8 conformance + ApplyPlan)#61
intel352 merged 11 commits intomainfrom
feat/pdo-v2-migration

Conversation

@intel352
Copy link
Copy Markdown
Contributor

@intel352 intel352 commented May 5, 2026

Summary

PR P-DO of the IaC conformance plan (docs/plans/2026-05-03-iac-conformance-and-replace.md §P-DO). Migrates the DigitalOcean provider plugin onto the v2 helper-based dispatch pattern landed in workflow main W-3a → W-9.

Five tasks shipped in atomic commits for bisect-safety:

  • TP1 ci(plugin): codemod-report workflow uploads dry-run output as artifact + sticky PR comment summary (53fbb5a)
    Per-PR .github/workflows/codemod-report.yml runs iac-codemod refactor-apply -dry-run against the plugin source, uploads the full Markdown report as a 90-day-retention GitHub Actions artifact, and posts a sticky PR comment with the top-30-line summary.
  • TP2 refactor(provider): collapse Apply to wfctlhelpers.ApplyPlan (f9f0022)
    DOProvider.Apply now wraps wfctlhelpers.ApplyPlan(ctx, p, plan) instead of running its own per-action switch. The DO-specific deferred-update second pass (Database type=app trusted_sources) is preserved by wrapping the helper. Drivers AppPlatform / VPC / Firewall / Database structurally satisfy the canonical interfaces.UpsertSupporter.
  • TP3 feat(provider): ValidatePlan for App Platform region-VPC constraints (21a6d63)
    Implements interfaces.ProviderValidator (W-4): App Platform requires region GROUP slugs, zone-bound resources require zone slugs, App Platform vpc_ref must reference a VPC in a compatible region group. Closes the recurring "App Platform in nyc cannot reach VPC in sfo3" production bug class (root-cause issue D from the design).
  • TP4 feat(plugin): opt into computePlanVersion: v2 (6b4c552)
    Adds top-level iacProvider.computePlanVersion: v2 to plugin.json so wfctl's runtime dispatcher routes Apply through wfctlhelpers.ApplyPlan.
  • TP5 feat(provider): add conformance test + extend ValidatePlan for database vpc_ref + bump to v0.10.0 (c47a1c3)
    provider_conformance_test.go (build tag conformance) invokes iac/conformance.Run. ValidatePlan extended to flag dangling infra.database vpc_ref (closes Scenario_CrossResourceConstraintRejection). Version → v0.10.0; CHANGELOG updated.

Plus a chore(deps): bump workflow to e2c582b prep commit (234cd75).

T8 codemod reports

TP1 lint report (initial state, before TP2/TP3 — captured for traceability)
# iac-codemod lint report

Findings: 4
Skipped:  0
Errors:   0

## Findings

- internal/drivers/volume.go:137 [AssertDiffSetsNeedsReplaceForForceNew]
    VolumeDriver.Diff references ForceNew but never assigns NeedsReplace; W-3 force-new contract violated
- internal/provider.go:28 [AssertProviderImplementsValidatePlan]
    provider type DOProvider does not implement ValidatePlan; ProviderValidator (R-A10) cannot run on plans involving this provider
- internal/provider.go:158 [AssertPlanDelegatesToHelper]
    DOProvider.Plan does not delegate to platform.ComputePlan; non-canonical Plan() body
- internal/provider.go:241 [AssertApplyDelegatesToHelper]
    DOProvider.Apply does not delegate to wfctlhelpers.ApplyPlan; non-canonical Apply() body

After this PR (post TP2 + TP3 + skip-marker):

# iac-codemod lint report

Findings: 2
Skipped:  4
Errors:   0

## Findings

- internal/drivers/volume.go:137 [AssertDiffSetsNeedsReplaceForForceNew]
    VolumeDriver.Diff references ForceNew but never assigns NeedsReplace; W-3 force-new contract violated
- internal/provider.go:159 [AssertPlanDelegatesToHelper]
    DOProvider.Plan does not delegate to platform.ComputePlan; non-canonical Plan() body

The two remaining findings are out of scope for P-DO:

  • volume.go:137 ForceNew/NeedsReplace gap is an existing volume-driver bug separate from the v2 migration; filed as a follow-up.
  • provider.go:159 DOProvider.Plan non-canonical body is the next migration step (a future refactor(provider): collapse Plan to platform.ComputePlan PR).
TP1 refactor-apply (dry-run) report
# iac-codemod refactor-apply report

Mode:    dry-run
Sites:   1
Errors:  0

## Upsert recovery — DO-style ErrResourceAlreadyExists path

- internal/provider.go:241 DOProvider.Apply upsert-recovery (offender at internal/provider.go:255)
  - suggestion: preserve via wfctlhelpers.ApplyPlan's upsertSupporter hook:
    drivers that support name-based discovery should implement
    SupportsUpsert() bool returning true; the helper handles
    ErrResourceAlreadyExists -> Read+Update internally. Sample patch:
    keep the existing upsertSupporter interface declaration on the
    driver type, then delete the manual upsert branch from Apply.

This is the canonical idiom the codemod identifies. TP2 implemented exactly this — the local upsertSupporter interface is removed (drivers' SupportsUpsert() method satisfies the canonical interfaces.UpsertSupporter directly) and the manual upsert branch is gone (handled inside wfctlhelpers.ApplyPlan::doCreate).

Scope deviations / incidental findings

  • Plan deviation (TP2): the plan said "replace DOProvider.Apply body with return wfctlhelpers.ApplyPlan(ctx, p, plan)". Strict literal application would break the deferred-update regression (provider_deferred_test.go — DatabaseDriver type=app trusted_sources referencing apps created later in the same plan). The implementation wraps the helper and runs the second-pass deferred-flush after, with // wfctl:skip-iac-codemod documenting the intentional shape deviation. When wfctlhelpers grows a deferred-update lifecycle hook, the wrapper can collapse and the marker can drop.
  • Plan deviation (TP3 → TP5): the plan listed App Platform region-VPC constraints only. The conformance suite's Scenario_CrossResourceConstraintRejection exercises an infra.database dangling vpc_ref, which my initial TP3 ValidatePlan didn't cover. TP5 extended ValidatePlan with appendDatabaseDiagnostics so the conformance scenario passes. The design's "(other constraints discoverable from DO docs as additions)" line covers this.
  • Plan deviation (TP2 test update): the TestDOProvider_Apply_DeleteAction_MissingCurrent regression test asserted v1 pre-flight defense (synthesize an error when action.Current == nil). Under v2 dispatch the contract is "driver is the authority on what an empty ProviderID means". Test rewritten to lock the new contract; commit message documents.
  • Incidental finding (volume.go:137): the codemod's AssertDiffSetsNeedsReplaceForForceNew analyzer flags VolumeDriver.Diff referencing ForceNew without assigning NeedsReplace. This is a pre-existing W-3 contract violation in the volume driver, not introduced by this PR. Suggest filing as a follow-up.
  • Incidental finding (provider.go:159 Plan): DOProvider.Plan is still the legacy in-provider implementation. This PR only migrates Apply (per the plan's TP-scope); migrating Plan is a separate follow-up that would invoke iac-codemod refactor-plan.
  • SDK manifest additionalProperties: false observation: during TP4 verification I noticed that the plugin/sdk/manifest_schema.json has additionalProperties: false on the iacProvider block, but the schema-validation step accepts a plugin.json with extra iacProvider keys (name, resourceTypes, configSchema). Strictly enforced via small standalone repros, this should have rejected those keys. Filing this as an upstream workflow follow-up — possible jsonschema-library or schema-draft interpretation gap. Not a blocker for this PR.

Test plan

  • GOWORK=off go build ./... — clean
  • GOWORK=off go vet ./... — clean
  • GOWORK=off go test ./... -count=1 -race — all packages PASS
  • GOWORK=off go test -tags=conformance ./internal/ -run TestConformance -count=1 — 6/6 non-cloud scenarios PASS (UpsertOnAlreadyExists + DiffSurvivesGRPCRoundTrip skip on absent opt-in driver types; remaining six all PASS)
  • GOWORK=off go test -tags=integration ./... -count=1 — all packages PASS
  • wfctl plugin validate --file plugin.json --strict-contracts — OK
  • iac-codemod lint . — only out-of-scope findings remain (volume ForceNew + Plan non-canonical body)
  • SDK manifest EffectiveComputePlanVersion() returns "v2" for the new plugin.json

Backward compat

  • wfctl < v0.21.0 ignores iacProvider.computePlanVersion; the legacy v1 dispatch (provider.Apply switch, now wrapping wfctlhelpers.ApplyPlan) continues to work for all existing callers.
  • No public-API breaking changes. The local upsertSupporter interface in internal/provider.go was unexported, so its removal is invisible to consumers; drivers implementing SupportsUpsert() continue to satisfy both the (gone) local interface and the canonical interfaces.UpsertSupporter.

🤖 Generated with Claude Code

intel352 and others added 6 commits May 4, 2026 21:20
Pseudo-version v0.20.6-0.20260505011403-e2c582bece90, the workflow main HEAD
that includes:
  - W-7: iac/conformance scenario suite (12 scenarios) and DO smoke gate
  - W-8: cmd/iac-codemod 4-mode AST tool

Required for TP1-TP5 of PR P-DO (IaC conformance plan §P-DO).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t + sticky PR comment summary

PR P-DO TP1: per pull_request, runs the iac-codemod refactor-apply mode
in -dry-run against the plugin source, uploads the full Markdown report as
a 90-day retention GitHub Actions artifact, and posts/updates a sticky PR
comment with the top-30 lines of the report so drive-by reviewers see the
key findings without downloading the artifact.

Supply-chain note: actions/github-script SHA-pinned per workflow security
policy (Renovate tracks upstream releases via .github/renovate.json).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR P-DO TP2: replace the in-Apply per-action switch (create/update/replace/
delete + upsert recovery + nil-out diagnostic) with a single dispatch to
wfctlhelpers.ApplyPlan. The helper handles:

  - ErrResourceAlreadyExists upsert recovery via interfaces.UpsertSupporter
    (DO drivers AppPlatform, VPC, Firewall, Database already implement
    SupportsUpsert() bool, so they satisfy the canonical interface without
    code change — the local upsertSupporter declaration is now removed).
  - JIT ${MODULE.id} / ${VAR} substitution (W-5).
  - Replace decomposition + ReplaceIDMap propagation (W-3a/W-3b).
  - Input-drift postcondition (W-3a).
  - Per-action context cancellation between iterations.

The DO-plugin-specific deferred-update flush (DatabaseDriver type=app
trusted_sources referencing apps created later in the plan; regression
gated by provider_deferred_test.go and CHANGELOG entry for staging-deploy-
blockers Blocker 2) is preserved by wrapping ApplyPlan with the second-
pass loop that calls FlushDeferredUpdates on any deferredUpdater driver.

The wrapper deviates from the codemod's canonical
'return wfctlhelpers.ApplyPlan(ctx, p, plan)' single-statement shape; the
deviation is documented and marked with // wfctl:skip-iac-codemod so
AssertApplyDelegatesToHelper recognizes the intentional shape. When
wfctlhelpers grows a deferred-update lifecycle hook, the wrapper can
collapse and the marker can drop.

The provider_apply_test.go DeleteAction_MissingCurrent regression test
was a v1 pre-flight defense (synthesize an error when action.Current is
nil before calling Delete). Under v2 dispatch the contract is 'driver is
the authority on what an empty ProviderID means' (per wfctlhelpers/
apply.go::doUpdate's analogous comment). The test was rewritten to lock
the new contract: dispatch IS made with an empty-ProviderID ref; real
drivers like FirewallDriver surface the diagnostic via their typed
validation.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR P-DO TP3: implement interfaces.ProviderValidator (W-4) on DOProvider
to surface DO region constraints at `wfctl infra align` time before
any cloud API call. Per the W-4 design, R-A10 invokes ValidatePlan via
type-assertion; providers that do not implement it continue to work
unchanged.

The first pass covers three constraint families:

  1. App Platform infra.container_service requires a region GROUP slug
     (nyc, ams, fra, sfo, sgp, syd, tor, blr, lon). Zone slugs (nyc1,
     sfo3 …) are rejected with PlanDiagnosticError. This is the
     "copy-pasted nyc3 from a Droplet config" defense.

  2. Zone-bound resources (infra.vpc, infra.droplet, infra.volume) MUST
     use a zone slug. Bare group slugs are rejected with
     PlanDiagnosticError. Inverse of (1).

  3. Cross-resource: an App Platform with vpc_ref pointing to a VPC in
     the same plan must have a region group whose zones include the
     referenced VPC's region. This locks the recurring 'App Platform in
     nyc cannot reach VPC in sfo3' production bug class (root-cause
     issue D from the conformance design). Cross-resource resolution
     looks at desired spec first, falls back to action.Current's
     Outputs["region"] for unchanged-VPC scenarios. vpc_ref pointing
     to a name not in the plan emits a Severity=Warning so non-strict
     align tolerates external VPCs while --strict escalates.

ValidatePlan is read-only and makes no remote calls per the W-4
contract. Compile-time interface assertion lives at the bottom of
validate_plan.go.

11 TDD tests in validate_plan_test.go cover: nil/empty plan,
group-slug accepted, zone-slug rejected for AP, zone-slug accepted for
VPC/Droplet/Volume, group-slug rejected for VPC, mismatch error
(flagship), happy-path match, unknown-vpc_ref Warning, current-state
fallback, delete-action skipped, compile-time assertion.

Future extensions (deferred follow-ups): database/cache zone slugs,
load balancer zone matching against attached droplets, registry
regional restrictions.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
PR P-DO TP4: declare iacProvider.computePlanVersion=v2 at the TOP LEVEL
of plugin.json so wfctl's runtime dispatcher (cmd/wfctl/deploy_providers.go
::iacPluginManifest) routes Apply through wfctlhelpers.ApplyPlan instead
of the legacy in-provider switch.

Schema note: the SDK manifest schema (plugin/sdk/manifest_schema.json)
expects iacProvider.computePlanVersion at the top level of plugin.json.
The existing capabilities.iacProvider sub-block (name, resourceTypes,
configSchema) is a DIFFERENT consumer (plugin discovery + capability
declaration); the two structures coexist in the same file. The runtime
loader unmarshals both into one struct (one Capabilities.IaCProvider.Name
field plus one top-level IaCProvider.ComputePlanVersion field) so a
single plugin.json read serves both code paths.

Validated via three checkers:
  - go run ./cmd/wfctl plugin validate --file plugin.json --strict-contracts (OK)
  - JSON-schema validation against plugin/sdk/manifest_schema.json (OK)
  - sdk.ParseManifest decode confirms EffectiveComputePlanVersion()==v2

Backward compat: wfctl < v0.21.0 ignores the new field; the legacy v1
dispatch (provider.Apply switch, now wrapping wfctlhelpers.ApplyPlan)
continues to work for all existing callers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…se vpc_ref + bump to v0.10.0

PR P-DO TP5: ship the conformance test entry point, extend ValidatePlan
to satisfy the conformance suite's cross-resource constraint scenario,
and bump plugin.json + CHANGELOG to v0.10.0.

provider_conformance_test.go (new, behind 'conformance' build tag):
  - Invokes iac/conformance.Run against a freshly-constructed and
    initialized DOProvider. Initialize is always called with a stub
    token so the driver registry is populated for the non-cloud
    scenarios that probe ResourceDriver lookups (structpb-roundtrip,
    cross-module resolution, etc.) — these exercise read-only or
    pure-data paths that don't hit DO's API.
  - LiveCloud (CONFORMANCE_LIVE_CLOUD=1 + DIGITALOCEAN_ACCESS_TOKEN)
    swaps the stub for the real token before driver instantiation.
  - SmokeOnly = testing.Short() limits to Smoke=true scenarios for
    the per-PR smoke gate's narrow contract.

ValidatePlan extension (validate_plan.go):
  - Added appendDatabaseDiagnostics: infra.database vpc_ref pointing to
    a name not in the plan emits a Severity=Error diagnostic. Closes
    the conformance Scenario_CrossResourceConstraintRejection assertion
    that 'at least one Severity=Error diagnostic' fires for a dangling
    cross-resource reference. The assertion was failing before this
    change (database vpc_ref was previously unhandled by ValidatePlan).
  - Two new TDD tests: dangling-vpc_ref → Error (mirrors the
    conformance contract in-tree); in-plan vpc_ref → no diagnostic
    (happy path).

plugin.json:
  - version 0.9.0 → 0.10.0
  - download URL paths v0.9.0 → v0.10.0 (per-OS/arch)

CHANGELOG.md:
  - New v0.10.0 section catalogues TP1-TP5 changes (ValidatePlan,
    computePlanVersion: v2 opt-in, Apply collapse, conformance test,
    codemod-report workflow), the Apply-delete v2-contract change,
    and the workflow dep bump.
  - Migrates the previously-Unreleased infra.vpc id-output fix into
    the v0.10.0 Fixed section (it ships in this release).

Test results:
  - go test -tags=conformance ./internal/ -run TestConformance: 6/6
    non-cloud scenarios PASS (CrossResourceConstraintRejection,
    DiffSurvivesGRPCRoundTrip, InfraOutputCrossModuleResolution,
    PlanStaleDiagnostic, ProtectedReplaceWithOverride,
    ProtectedReplaceWithoutOverride). Upsert-on-already-exists +
    grpc-roundtrip skip when their opt-in driver types are absent
    (DO does not expose infra.compute).
  - go test ./... -count=1 -race: ALL packages PASS.
  - go run ./cmd/wfctl plugin validate --strict-contracts: OK.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 5, 2026 01:40
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

iac-codemod refactor-apply report

Mode: dry-run
Sites: 1
Errors: 0

Skipped (// wfctl:skip-iac-codemod)

  • internal/provider.go:267 DOProvider.Apply skipped

Full report (90-day retention) attached as workflow artifact.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Migrates the DigitalOcean IaC provider plugin to the workflow “v2” helper-based Apply dispatch, adds plan-time cross-resource validation (ValidatePlan), and introduces a tagged conformance test suite runner; also bumps plugin/versioning metadata to v0.10.0 and updates CI to publish codemod reports.

Changes:

  • Opt into v2 IaC dispatch by adding iacProvider.computePlanVersion: v2 and refactoring DOProvider.Apply to wrap wfctlhelpers.ApplyPlan while preserving the DO-specific deferred-update flush.
  • Add DOProvider.ValidatePlan with region/vpc_ref cross-resource constraints (App Platform group vs zone slugs; VPC/Droplet/Volume zone slugs; database dangling vpc_ref).
  • Add -tags=conformance conformance runner test and update unit tests around v2 delete-dispatch behavior; bump workflow dependency + CHANGELOG.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
plugin.json Bumps version/URLs and opts into v2 compute plan dispatch.
internal/provider.go Routes legacy v1 Apply through wfctlhelpers.ApplyPlan and keeps the deferred-update second pass.
internal/validate_plan.go Introduces provider-side ValidatePlan cross-resource diagnostics.
internal/validate_plan_test.go Adds unit coverage for ValidatePlan constraints and edge cases.
internal/provider_conformance_test.go Adds a conformance build-tag test harness for upstream conformance scenarios.
internal/provider_apply_test.go Updates delete-with-missing-current expectations to match v2 dispatch behavior.
go.mod / go.sum Bumps github.com/GoCodeAlone/workflow dependency to a newer pseudo-version.
CHANGELOG.md Documents v0.10.0 changes (v2 dispatch, ValidatePlan, conformance, CI codemod report).
.github/workflows/codemod-report.yml Adds a PR workflow to run iac-codemod refactor-apply -dry-run, upload artifact, and post a sticky PR comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/validate_plan.go
Comment thread internal/validate_plan.go Outdated
Comment thread internal/validate_plan.go Outdated
Comment thread internal/provider_conformance_test.go Outdated
Comment thread .github/workflows/codemod-report.yml
5/5 findings addressed:

#1 (validate_plan.go:52) — byName index must exclude delete-action
   resources. Previously a vpc_ref pointing to a VPC scheduled for
   deletion in the same plan would silently 'resolve' as if live; now
   delete-targets are excluded from the index so cross-resource
   references to them surface as Severity=Error dangling references.
   New regression test:
   TestDOProvider_ValidatePlan_DatabaseVPCRefToDeleteTargetIsDangling.

#2 (validate_plan.go:91) — appendDatabaseDiagnostics docstring
   incorrectly said 'Warning when missing'; the implementation has
   always emitted PlanDiagnosticError, and the conformance scenario +
   TDD tests both REQUIRE Error severity. Doc rewritten to match the
   implemented contract.

#3 (validate_plan.go:225) — zonesInGroup docstring promised sorted
   output but returned the raw underlying slice unsorted. Now copies +
   sorts (lexicographic) so diagnostic messages are deterministic and
   the returned slice is owned by the caller (safe to mutate). nil
   for unknown groups; the caller's strings.Join still works.

#4 (provider_conformance_test.go:70) — comment described a
   'stub-then-real swap' with two Initialize calls; the implementation
   has always made one call with the right token chosen up-front. Doc
   rewritten to match the actual single-call flow.

#5 (codemod-report.yml:73) — fork PRs run with a read-only
   GITHUB_TOKEN per GitHub's pull_request workflow security model, so
   issues:create-comment + issues:update-comment fail 403 and would
   block CI. Gate the comment step on
   github.event.pull_request.head.repo.fork == false. The artifact
   upload step still runs unconditionally so the report remains
   reachable from the Actions tab.

Verified locally: go test ./... -count=1 -race PASS, go test
-tags=conformance ./internal/ -run TestConformance PASS, go vet
clean, codemod-report.yml YAML valid.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/validate_plan.go Outdated
Comment thread internal/validate_plan.go
2/2 findings addressed (both legitimate type-safety improvements):

#6 (validate_plan.go:123) — appendDatabaseDiagnostics validated that
   vpc_ref name resolved in the plan but did NOT validate that the
   target type is actually infra.vpc. A vpc_ref pointing to a Droplet
   or App Platform resource silently passed prior validation. Now
   surfaces a typed Severity=Error diagnostic when target.spec.Type
   != "infra.vpc". New regression test:
   TestDOProvider_ValidatePlan_DatabaseVPCRefToNonVPCType.

#7 (validate_plan.go:189) — same bug in appendAppPlatformDiagnostics:
   vpc_ref pointing to a non-VPC resource would silently bypass the
   region-match check (target.spec.Config["region"] would be a
   region GROUP for an App Platform target, not a zone for a VPC) and
   the operator would never see a clear diagnostic. Same fix: typed
   Error when target.spec.Type != "infra.vpc". New regression test:
   TestDOProvider_ValidatePlan_AppPlatformVPCRefToNonVPCType.

Both diagnostics carry the offending type in the message body so the
operator immediately knows whether they typo'd a name (resolves to
nothing) vs. typo'd a TYPE in a name (resolves to wrong resource).

Verified locally: go test ./... -count=1 -race PASS, go test
-tags=conformance ./internal/ -run TestConformance PASS, go vet
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/validate_plan.go
Comment thread internal/validate_plan.go
Comment thread internal/validate_plan.go Outdated
Comment thread internal/provider_apply_test.go Outdated
…e + classifyRegion + delete-test comment)

4/4 findings addressed (deeper architectural correctness):

#8/#9 (validate_plan.go:137,205) — vpc_ref's accepted DO API shapes
   are: (a) a VPC UUID literal, OR (b) a wfctl JIT template like
   ${vpc.id} that resolves to a UUID at apply time via
   wfctlhelpers.ApplyPlan's jitsubst.ResolveSpec. The prior validator
   unconditionally treated vpc_ref as an in-plan resource Name and
   would have rejected production configs that use the canonical UUID
   shape (godo.AppVpcSpec{ID: vpcID} consumes it directly per
   internal/drivers/app_platform_buildspec.go:674). New
   looksLikeResourceName() helper detects UUID literals (RFC-4122
   pattern) and JIT templates (${...} / $(...)), and the in-plan-
   name lookup is skipped for both. Only plain-name vpc_ref values
   trigger the dangling-reference + non-VPC-type checks. Diagnostic
   messages updated to call out the 'plain resource name' branch
   explicitly so an operator who hit the diagnostic understands it
   does not apply to UUID/template forms. Two new TDD tests cover the
   UUID-literal + JIT-template paths for both database and App
   Platform.

#10 (validate_plan.go:307) — classifyRegion emitted 'a zone slug in
   group ""' for legacy zones (nyc2, ams2) that intentionally map
   to an empty group. Now special-cases the empty-group case to emit
   'a zone slug not in any App Platform region group'. New TDD test
   exercises the path via an App Platform action with region=nyc2.

#11 (provider_apply_test.go:94) — the test comment claimed real
   drivers reject empty-ProviderID deletes via typed validation;
   FirewallDriver.Delete actually resolves by name when ProviderID
   is empty. Comment rewritten to reflect that v2 dispatch contract
   is 'driver knows what an empty ProviderID means for its resource
   shape', not 'all drivers reject empty ProviderID'.

Net new test count: 3 (UUID-deferred, JIT-template-deferred,
classify-empty-group). All existing tests still pass.

Verified locally: go test ./... -count=1 -race PASS, go test
-tags=conformance ./internal/ -run TestConformance PASS, go vet
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/validate_plan.go Outdated
Comment thread internal/provider_conformance_test.go Outdated
Comment thread .github/workflows/codemod-report.yml Outdated
…t + workflow perms)

3/3 findings addressed:

#12 (validate_plan.go:16) — uuidPattern was lowercase-only; UUIDs are
   case-insensitive in practice (operator clipboards, templating
   engines, mixed-case API responses). Added (?i) flag so upper-case
   and mixed-case VPC UUIDs also classify as deferred-to-apply, not
   as plain resource names that would trigger false dangling-reference
   diagnostics. New test:
   TestDOProvider_ValidatePlan_VPCRefAsUpperCaseUUIDIsDeferred.

#13 (provider_conformance_test.go:86) — switched
   p.Initialize(context.Background(), ...) to p.Initialize(t.Context(), ...)
   so live-cloud Initialize is interrupted promptly when the test
   is canceled or hits its deadline. Removed the now-unused
   "context" import.

#14 (codemod-report.yml:12) — dropped the unused pull-requests:write
   permission. The workflow only creates/updates an issue comment
   (PR comments are issues at the GitHub API layer) so the surviving
   issues:write is sufficient. Inline doc-comment captures the
   reasoning so future maintainers don't restore the broader grant.
   Aligns with ci.yml's contents:read-only baseline.

Verified locally: go test ./... -count=1 -race PASS, go test
-tags=conformance ./internal/ -run TestConformance PASS, go vet
clean, codemod-report.yml YAML valid.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/provider_conformance_test.go Outdated
Comment thread internal/validate_plan.go
…known-region forward-compat)

2/2 findings addressed:

#15 (provider_conformance_test.go:83) — the prior comment claimed
   t.Context() makes Initialize cancelable, but DOProvider.Initialize
   today constructs its godo client with its own context.Background()
   and ignores the passed-in ctx. Comment rewritten to reflect that
   the change is forward-prep (so any future rev of Initialize that
   honors ctx picks up the test-scoped cancellation/deadline) rather
   than an immediate behavior fix. Tracked as a follow-up to thread
   ctx through the godo client construction.

#16 (validate_plan.go:334) — the hardcoded region/zone allowlists
   would Severity=Error any brand-new DO region the plugin hasn't
   caught up to, blocking apply until the plugin is bumped. Severity
   is now two-bucket:
     - Documented misconfig (group-where-zone-required, zone-where-
       group-required) → Error (the original anti-pattern stays loud).
     - Unknown slug (neither known group nor known zone, e.g. a
       hypothetical 'atl' or 'atl1') → Warning so non-strict align
       lets operators on bleeding-edge DO regions proceed; --strict
       still escalates for cautious operators.
   New regression test:
   TestDOProvider_ValidatePlan_UnknownRegionSlugWarnsNotErrors covers
   both VPC zone and AP group unknown-slug paths.

Net new test count: 1. All existing tests still PASS (the documented
misconfig branches use known specific slugs that hit the Error path).

Verified locally: go test ./... -count=1 -race PASS, go test
-tags=conformance ./internal/ -run TestConformance PASS, go vet
clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@intel352 intel352 merged commit 6797d9e into main May 5, 2026
5 checks passed
@intel352 intel352 deleted the feat/pdo-v2-migration branch May 5, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants