From e407325fdac51f2e63210f0b1e8da519845dec90 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 5 May 2026 06:37:46 -0400 Subject: [PATCH 1/3] fix(provider): thread ctx to godo client in Initialize (#62) Initialize previously discarded its caller-supplied ctx and constructed the oauth2 client with context.Background(), silently dropping any oauth2.HTTPClient injected via the provided ctx (tests, custom transports, proxy configurations). Now the ctx is forwarded to oauth2.NewClient, and a nil-ctx guard returns a typed error instead of panicking inside the oauth2 stack. Tests: nil-ctx guard + a capturing-transport regression test that verifies a custom http.Client injected via oauth2.HTTPClient observes a real outbound godo request. Refs: workflow-plugin-digitalocean#62 --- internal/provider.go | 16 ++++++++-- internal/provider_test.go | 62 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 2 deletions(-) diff --git a/internal/provider.go b/internal/provider.go index 026ac29..91af91e 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -50,7 +50,19 @@ func (p *DOProvider) Version() string { return Version } // Initialize configures the godo client using the provided config map. // Required: "token". // Optional: "region" (default "nyc3"), "spaces_access_key", "spaces_secret_key". -func (p *DOProvider) Initialize(_ context.Context, config map[string]any) error { +// +// The provided ctx is threaded into oauth2.NewClient so callers that inject a +// custom *http.Client via oauth2.HTTPClient (tests, custom transports, proxy +// configurations) flow through to the godo client. Per-request cancellation +// remains controlled by the ctx passed to each subsequent driver call — +// godo wraps each request in ctx via http.Request.WithContext. +// +// addresses workflow-plugin-digitalocean#62: prior implementation hardcoded +// context.Background(), silently dropping any HTTPClient injection. +func (p *DOProvider) Initialize(ctx context.Context, config map[string]any) error { + if ctx == nil { + return fmt.Errorf("digitalocean: Initialize requires non-nil ctx") + } token, _ := config["token"].(string) if token == "" { return fmt.Errorf("digitalocean: missing required config key 'token'") @@ -64,7 +76,7 @@ func (p *DOProvider) Initialize(_ context.Context, config map[string]any) error spacesAccessKey, _ := config["spaces_access_key"].(string) spacesSecretKey, _ := config["spaces_secret_key"].(string) - oauthClient := oauth2.NewClient(context.Background(), &tokenSource{token: token}) + oauthClient := oauth2.NewClient(ctx, &tokenSource{token: token}) p.client = godo.NewClient(oauthClient) p.drivers = map[string]interfaces.ResourceDriver{ diff --git a/internal/provider_test.go b/internal/provider_test.go index 02051f1..59ffc50 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -10,6 +10,7 @@ import ( "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" "github.com/GoCodeAlone/workflow/interfaces" "github.com/digitalocean/godo" + "golang.org/x/oauth2" ) // compile-time interface check @@ -68,6 +69,67 @@ func TestDOProvider_Initialize_MissingToken(t *testing.T) { } } +// TestDOProvider_Initialize_NilCtxRejected pins the nil-ctx guard added in +// workflow-plugin-digitalocean#62. Initialize must reject nil ctx rather than +// silently passing it down to oauth2.NewClient. +func TestDOProvider_Initialize_NilCtxRejected(t *testing.T) { + p := NewDOProvider() + //nolint:staticcheck // intentional nil ctx for the guard test + err := p.Initialize(nil, map[string]any{"token": "fake-token"}) + if err == nil { + t.Fatal("expected error for nil ctx; got nil") + } + if !strings.Contains(err.Error(), "non-nil ctx") { + t.Errorf("error %q should mention non-nil ctx", err.Error()) + } +} + +// httpClientCapturingTransport records the http.Client that issued a request. +// Used to verify Initialize threaded ctx-injected oauth2.HTTPClient into the +// godo client's transport chain. +type httpClientCapturingTransport struct { + called bool + resp *http.Response +} + +func (t *httpClientCapturingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + t.called = true + if t.resp != nil { + return t.resp, nil + } + // Return a minimal 200 with empty JSON body so godo doesn't choke. + return &http.Response{ + StatusCode: http.StatusOK, + Body: http.NoBody, + Header: make(http.Header), + Request: req, + }, nil +} + +// TestDOProvider_Initialize_ThreadsCtxToGodoClient pins workflow-plugin-digitalocean#62. +// Prior to the fix, Initialize discarded the caller's ctx and constructed the +// oauth2 client with context.Background(), so any oauth2.HTTPClient injected via +// ctx (tests, custom transports, proxy configs) was silently dropped. This test +// injects a capturing transport via oauth2.HTTPClient and verifies it observes a +// real outbound request after Initialize wires the godo client. +func TestDOProvider_Initialize_ThreadsCtxToGodoClient(t *testing.T) { + transport := &httpClientCapturingTransport{} + customClient := &http.Client{Transport: transport} + ctx := context.WithValue(t.Context(), oauth2.HTTPClient, customClient) + + p := NewDOProvider() + if err := p.Initialize(ctx, map[string]any{"token": "fake-token"}); err != nil { + t.Fatalf("Initialize: %v", err) + } + + // Issue any godo call that hits the transport. Account.Get is the cheapest. + _, _, _ = p.client.Account.Get(t.Context()) + + if !transport.called { + t.Fatal("custom transport injected via oauth2.HTTPClient was never called; ctx was not threaded into godo client") + } +} + func TestDOProvider_ResolveSizing(t *testing.T) { p := NewDOProvider() result, err := p.ResolveSizing("infra.database", interfaces.SizeM, nil) From b155b7e36c6527e5cc1b8762cb35583fa1088173 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 5 May 2026 06:48:55 -0400 Subject: [PATCH 2/3] refactor(provider): collapse Plan to platform.ComputePlan (#63) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hand-rolled Plan body duplicated logic that already lives in platform.ComputePlan and silently dropped two correctness paths the canonical helper enforces: 1. ForceNew → replace promotion: ComputePlan upgrades update→replace when any FieldChange has ForceNew=true; the prior body only honored DiffResult.NeedsReplace, so any driver that returned ForceNew=true on a per-field basis (without setting NeedsReplace) emitted update actions that would later fail at Apply time. 2. DependsOn-ordered dispatch: ComputePlan parallelises Diff with an errgroup worker pool and topologically sorts the resulting actions so the Apply stage sees creates-before-replaces-before-deletes order. The previous body iterated desired in input order with no ordering guarantee. The 2-statement (call + pointer-bridge return) shape matches the W-Refactor / iac-codemod canonical target asserted by cmd/iac-codemod AssertPlanDelegatesToHelper. Lint reports 0 findings on internal/provider.go after this change. Test impact: - planDiffFakeDriver gains a sync.Mutex around its observable state (Diff is dispatched in parallel by ComputePlan). - TestMain sets WFCTL_DIFFCACHE=disabled so per-test Diff dispatches are reproducible (the filesystem cache is global per UID and would otherwise let one test poison another). - TestDOProvider_Plan_KeepsDistinctCurrentStatePerConfigHashAction retired and replaced by TestDOProvider_Plan_RejectsUnregisteredDriverType — the configHash-fallback semantic it pinned no longer applies, since ComputePlan surfaces the missing-driver error rather than masking it. Refs: workflow-plugin-digitalocean#63 --- internal/provider.go | 77 +++++++++------------------------------ internal/provider_test.go | 71 +++++++++++++++++++++++------------- 2 files changed, 63 insertions(+), 85 deletions(-) diff --git a/internal/provider.go b/internal/provider.go index 91af91e..4206706 100644 --- a/internal/provider.go +++ b/internal/provider.go @@ -12,6 +12,7 @@ import ( "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" "github.com/GoCodeAlone/workflow/iac/wfctlhelpers" "github.com/GoCodeAlone/workflow/interfaces" + "github.com/GoCodeAlone/workflow/platform" "github.com/digitalocean/godo" "golang.org/x/oauth2" "google.golang.org/grpc/codes" @@ -167,67 +168,23 @@ func (p *DOProvider) ResolveSizing(resourceType string, size interfaces.Size, hi return resolveSizing(resourceType, size, hints) } -// Plan computes the set of actions needed to reach the desired state. +// Plan computes the set of actions needed to reach the desired state by +// delegating to the canonical platform.ComputePlan helper. The helper +// dispatches per-resource Diff in parallel, classifies replace vs update +// (including the ForceNew → replace promotion), emits creates/deletes in +// dependency-correct order, and consults the diff cache. +// +// The 2-statement form (call + pointer-bridge return) is mandated by the +// W-Refactor / iac-codemod analyzer (cmd/iac-codemod AssertPlanDelegatesToHelper); +// see workflow CHANGELOG entry referencing platform.ComputePlan as the +// canonical target for v2 IaC providers. +// +// addresses workflow-plugin-digitalocean#63: the prior hand-rolled body +// duplicated ComputePlan's classification logic and silently dropped the +// ForceNew → replace upgrade path (only NeedsReplace was honored). func (p *DOProvider) Plan(ctx context.Context, desired []interfaces.ResourceSpec, current []interfaces.ResourceState) (*interfaces.IaCPlan, error) { - currentByName := make(map[string]interfaces.ResourceState, len(current)) - for _, r := range current { - currentByName[r.Name] = r - } - - plan := &interfaces.IaCPlan{ - ID: fmt.Sprintf("plan-%d", time.Now().UnixNano()), - CreatedAt: time.Now(), - } - - for _, spec := range desired { - cur, exists := currentByName[spec.Name] - if !exists { - plan.Actions = append(plan.Actions, interfaces.PlanAction{ - Action: "create", - Resource: spec, - }) - continue - } - if driver, err := p.ResourceDriver(spec.Type); err == nil { - diff, err := driver.Diff(ctx, spec, resourceOutputFromState(cur)) - if err != nil { - return nil, fmt.Errorf("plan diff %s/%s: %w", spec.Type, spec.Name, err) - } - if diff != nil && (diff.NeedsUpdate || diff.NeedsReplace) { - action := "update" - if diff.NeedsReplace { - action = "replace" - } - plan.Actions = append(plan.Actions, interfaces.PlanAction{ - Action: action, - Resource: spec, - Current: &cur, - Changes: diff.Changes, - }) - continue - } - if diff != nil { - continue - } - } - if configHash(cur.AppliedConfig) != configHash(spec.Config) { - plan.Actions = append(plan.Actions, interfaces.PlanAction{ - Action: "update", - Resource: spec, - Current: &cur, - }) - } - } - return plan, nil -} - -func resourceOutputFromState(state interfaces.ResourceState) *interfaces.ResourceOutput { - return &interfaces.ResourceOutput{ - Name: state.Name, - Type: state.Type, - ProviderID: state.ProviderID, - Outputs: state.Outputs, - } + plan, err := platform.ComputePlan(ctx, p, desired, current) + return &plan, err } // deferredUpdater is an optional interface for ResourceDrivers that accumulate diff --git a/internal/provider_test.go b/internal/provider_test.go index 59ffc50..a1bc653 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -4,7 +4,9 @@ import ( "context" "fmt" "net/http" + "os" "strings" + "sync" "testing" "github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers" @@ -13,6 +15,19 @@ import ( "golang.org/x/oauth2" ) +// TestMain disables the platform/diffcache filesystem backend so per-test Diff +// dispatch is reproducible. ComputePlan caches Diff results under +// ~/.cache/wfctl/diff/ keyed on (PluginVersion, Type, ProviderID, SHAConfig, +// SHAOutputs); without disabling, prior test runs poison subsequent runs and +// fakes that record Diff invocations observe zero calls. WFCTL_DIFFCACHE is +// resolved by getDiffCache via sync.Once on first cache-eligible Diff call. +func TestMain(m *testing.M) { + if err := os.Setenv("WFCTL_DIFFCACHE", "disabled"); err != nil { + panic(err) + } + os.Exit(m.Run()) +} + // compile-time interface check var _ interfaces.IaCProvider = (*DOProvider)(nil) @@ -219,9 +234,18 @@ func TestConfigHash_Empty(t *testing.T) { } } +// planDiffFakeDriver is a test double whose Diff records each invocation. +// platform.ComputePlan dispatches Diff in parallel via errgroup, so all +// observable state mutations are guarded by mu. type planDiffFakeDriver struct { - diffResult *interfaces.DiffResult - diffCalls int + diffResult *interfaces.DiffResult + mu sync.Mutex + diffCalls int + // receivedSpec / receivedCurrent capture the LAST Diff invocation; in + // multi-resource tests they reflect non-deterministic ordering since + // dispatch is parallel — assertions that depend on a specific resource + // should pin the spec by Name in the assertion (or use a multi-shot + // recorder per name). receivedSpec interfaces.ResourceSpec receivedCurrent *interfaces.ResourceOutput } @@ -237,6 +261,8 @@ func (f *planDiffFakeDriver) Update(_ context.Context, _ interfaces.ResourceRef, } func (f *planDiffFakeDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { return nil } func (f *planDiffFakeDriver) Diff(_ context.Context, spec interfaces.ResourceSpec, current *interfaces.ResourceOutput) (*interfaces.DiffResult, error) { + f.mu.Lock() + defer f.mu.Unlock() f.diffCalls++ f.receivedSpec = spec f.receivedCurrent = current @@ -355,18 +381,22 @@ func TestDOProvider_Plan_KeepsDistinctCurrentStatePerAction(t *testing.T) { } } -func TestDOProvider_Plan_KeepsDistinctCurrentStatePerConfigHashAction(t *testing.T) { +// TestDOProvider_Plan_RejectsUnregisteredDriverType pins the new contract +// after refactoring DOProvider.Plan to platform.ComputePlan +// (workflow-plugin-digitalocean#63). Previously the hand-rolled Plan body +// silently fell back to a configHash compare when ResourceDriver returned an +// error; the canonical helper instead surfaces the missing-driver error so +// operators see a clear failure rather than a stale-shape plan. +// +// Replaces TestDOProvider_Plan_KeepsDistinctCurrentStatePerConfigHashAction — +// the legacy-fallback semantic that test pinned no longer applies. +func TestDOProvider_Plan_RejectsUnregisteredDriverType(t *testing.T) { desired := []interfaces.ResourceSpec{ { Name: "one-dns", Type: "infra.dns", Config: map[string]any{"domain": "one.example.com"}, }, - { - Name: "two-dns", - Type: "infra.dns", - Config: map[string]any{"domain": "two.example.com"}, - }, } current := []interfaces.ResourceState{ { @@ -375,27 +405,18 @@ func TestDOProvider_Plan_KeepsDistinctCurrentStatePerConfigHashAction(t *testing ProviderID: "one.example.com", AppliedConfig: map[string]any{"domain": "old-one.example.com"}, }, - { - Name: "two-dns", - Type: "infra.dns", - ProviderID: "two.example.com", - AppliedConfig: map[string]any{"domain": "old-two.example.com"}, - }, } + // DOProvider with no drivers registered — ResourceDriver returns an error + // for every type. ComputePlan must propagate that error rather than + // silently emit a configHash-based update. p := &DOProvider{} - plan, err := p.Plan(t.Context(), desired, current) - if err != nil { - t.Fatalf("Plan: %v", err) - } - if len(plan.Actions) != 2 { - t.Fatalf("plan actions = %d, want 2", len(plan.Actions)) - } - if plan.Actions[0].Current == nil || plan.Actions[0].Current.ProviderID != "one.example.com" { - t.Fatalf("first action current = %+v, want one.example.com", plan.Actions[0].Current) + _, err := p.Plan(t.Context(), desired, current) + if err == nil { + t.Fatal("expected error from Plan when driver is not registered; got nil") } - if plan.Actions[1].Current == nil || plan.Actions[1].Current.ProviderID != "two.example.com" { - t.Fatalf("second action current = %+v, want two.example.com", plan.Actions[1].Current) + if !strings.Contains(err.Error(), "infra.dns") { + t.Errorf("error %q should mention the missing resource type", err.Error()) } } From 48b380e79f4283e5e97e5c70a1cd40466d5e818a Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 5 May 2026 06:53:32 -0400 Subject: [PATCH 3/3] test(provider): return valid JSON body from capturing transport MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot review #68: the capturing transport returned http.NoBody, which godo's response decoder can error on (EOF / invalid JSON), making the test more brittle than necessary and rendering the inline comment about "empty JSON body" misleading. Return a minimal valid JSON object body ("{}") with a Content-Type header instead — the test still only cares that the transport observed the request, but the path is robust against godo's decode-on-success behavior on pointer-to-struct destinations. --- internal/provider_test.go | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/internal/provider_test.go b/internal/provider_test.go index a1bc653..cbf45fb 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -3,6 +3,7 @@ package internal import ( "context" "fmt" + "io" "net/http" "os" "strings" @@ -112,11 +113,17 @@ func (t *httpClientCapturingTransport) RoundTrip(req *http.Request) (*http.Respo if t.resp != nil { return t.resp, nil } - // Return a minimal 200 with empty JSON body so godo doesn't choke. + // Return a minimal 200 with a valid JSON object body so godo's response + // decoder doesn't error on EOF / invalid JSON when the caller passes a + // pointer-to-struct destination. We don't care what the test caller does + // with the response — only that the transport observed the request, which + // proves the ctx-injected http.Client made it through Initialize. + header := make(http.Header) + header.Set("Content-Type", "application/json") return &http.Response{ StatusCode: http.StatusOK, - Body: http.NoBody, - Header: make(http.Header), + Body: io.NopCloser(strings.NewReader("{}")), + Header: header, Request: req, }, nil }