Skip to content

fix/refactor(provider): Initialize ctx + Plan canonicalization (#62, #63) #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
intel352 merged 3 commits into from
May 5, 2026
Merged

fix/refactor(provider): Initialize ctx + Plan canonicalization (#62, #63) #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e407325
fix(provider): thread ctx to godo client in Initialize (#62)
intel352 May 5, 2026
b155b7e
refactor(provider): collapse Plan to platform.ComputePlan (#63)
intel352 May 5, 2026
48b380e
test(provider): return valid JSON body from capturing transport
intel352 May 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
93 changes: 31 additions & 62 deletions internal/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -50,7 +51,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'")
Expand All @@ -64,7 +77,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{
Expand Down Expand Up @@ -155,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
Expand Down
140 changes: 115 additions & 25 deletions internal/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,32 @@ package internal
import (
"context"
"fmt"
"io"
"net/http"
"os"
"strings"
"sync"
"testing"

"github.com/GoCodeAlone/workflow-plugin-digitalocean/internal/drivers"
"github.com/GoCodeAlone/workflow/interfaces"
"github.com/digitalocean/godo"
"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)

Expand Down Expand Up @@ -68,6 +85,73 @@ 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 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: io.NopCloser(strings.NewReader("{}")),
Header: 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)
Expand Down Expand Up @@ -157,9 +241,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
}
Expand All @@ -175,6 +268,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
Expand Down Expand Up @@ -293,18 +388,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{
{
Expand All @@ -313,27 +412,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())
}
}

Expand Down
Loading