diff --git a/internal/provider.go b/internal/provider.go index 026ac29..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" @@ -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'") @@ -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{ @@ -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 diff --git a/internal/provider_test.go b/internal/provider_test.go index 02051f1..cbf45fb 100644 --- a/internal/provider_test.go +++ b/internal/provider_test.go @@ -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) @@ -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) @@ -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 } @@ -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 @@ -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{ { @@ -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()) } }