From b86cf21b61ce693558624257f6106010e7c243e5 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 26 May 2026 21:32:51 -0400 Subject: [PATCH 1/4] feat(wfctl): add infra import-all subcommand + dispatch + helpers --- cmd/wfctl/infra.go | 3 + cmd/wfctl/infra_import_all.go | 317 +++++++++++++++++++++++++++++ cmd/wfctl/infra_import_all_test.go | 38 ++++ 3 files changed, 358 insertions(+) create mode 100644 cmd/wfctl/infra_import_all.go create mode 100644 cmd/wfctl/infra_import_all_test.go diff --git a/cmd/wfctl/infra.go b/cmd/wfctl/infra.go index 878d8581..3814de9a 100644 --- a/cmd/wfctl/infra.go +++ b/cmd/wfctl/infra.go @@ -74,6 +74,8 @@ func runInfra(args []string) error { return runInfraDestroy(args[1:]) case "import": return runInfraImport(args[1:]) + case "import-all": + return runInfraImportAll(args[1:]) case "state": return runInfraState(args[1:]) case "logs": @@ -126,6 +128,7 @@ Actions: drift Detect configuration drift destroy Tear down infrastructure import Import an existing cloud resource into state + import-all Bulk-import every resource of --type from a provider via EnumerateAll state Manage IaC state (list, export, import) logs Capture provider logs for an infrastructure resource outputs Print captured resource outputs from state diff --git a/cmd/wfctl/infra_import_all.go b/cmd/wfctl/infra_import_all.go new file mode 100644 index 00000000..7c61e5ac --- /dev/null +++ b/cmd/wfctl/infra_import_all.go @@ -0,0 +1,317 @@ +package main + +import ( + "context" + "encoding/json" + "flag" + "fmt" + "os" + "strings" + + "github.com/GoCodeAlone/workflow/config" + "github.com/GoCodeAlone/workflow/interfaces" +) + +// runInfraImportAll implements `wfctl infra import-all` — a bulk wrapper that +// resolves a single iac.provider module from the config, runs the provider's +// IaCProviderEnumerator.EnumerateAll(resourceType), and then iterates +// IaCProvider.Import for each enumerated cloud ID, persisting each +// synthesized ResourceState into the configured iac.state backend. +// +// Per-zone failure isolation: a single Import failure does NOT abort the +// loop; failures are accumulated and surfaced as a single error at the end. +// The caller observes a non-zero exit when any zone failed, with the +// failure list in the error message — matching the design's Phase 2 +// "non-zero exit if any zone import fails" contract. +// +// `--provider` is the iac.provider MODULE NAME from the config file (e.g. +// "do-prod"), NOT the plugin type discriminator (e.g. "digitalocean"). The +// helper resolveProviderModuleByName walks cfg.Modules to extract the plugin +// type from modCfg["provider"] — same pattern as resolveProviderForSpec for +// the single-resource import path. +// +// `--type` is the resource-type string the EnumerateAll method accepts — +// initially "infra.dns"; the EnumeratorAll contract is type-agnostic so this +// command works for any resource type a provider plugin implements +// (spaces_key, dns, etc.). +// +// `--dry-run` probes each enumerated cloud ID via provider.Import to surface +// auth + reachability failures without persisting any state. Useful for +// dispatch-readiness checks before running the actual import. +// +// `--output` / `-o` dumps the post-import state-store contents to the given +// file as a JSON array, for scenario test harnesses that need to diff state +// across runs without re-reading the live state backend. +func runInfraImportAll(args []string) error { + fs := flag.NewFlagSet("infra import-all", flag.ContinueOnError) + var configFile, envName, providerName, resourceType, pluginDirFlag, outputPath string + var dryRun bool + fs.StringVar(&configFile, "config", "", "Config file") + fs.StringVar(&configFile, "c", "", "Config file (short for --config)") + fs.StringVar(&envName, "env", "", "Environment name") + fs.StringVar(&providerName, "provider", "", "Provider module name from config (required)") + fs.StringVar(&resourceType, "type", "", "Resource type to enumerate, e.g. infra.dns (required)") + fs.BoolVar(&dryRun, "dry-run", false, "List enumerated resources without persisting state") + fs.StringVar(&pluginDirFlag, "plugin-dir", "", "Plugin directory (overrides WFCTL_PLUGIN_DIR and default data/plugins)") + fs.StringVar(&outputPath, "output", "", "Optional: dump state-store contents to this file (in addition to the state backend)") + fs.StringVar(&outputPath, "o", "", "Output path (short for --output)") + if err := fs.Parse(args); err != nil { + return err + } + if providerName == "" { + return fmt.Errorf("import-all requires --provider (the iac.provider module name from the config)") + } + if resourceType == "" { + return fmt.Errorf("import-all requires --type (e.g. infra.dns)") + } + + // Plugin-dir flag follows the same scoped-override pattern used by + // runInfraImport: temporarily set the package-level + // currentInfraPluginDir so downstream provider resolution honors the + // flag, then restore on exit. Empty flag → use existing default. + prevInfraPluginDir := currentInfraPluginDir + currentInfraPluginDir = pluginDirFlag + defer func() { currentInfraPluginDir = prevInfraPluginDir }() + + cfgFile, err := resolveInfraConfig(fs, configFile) + if err != nil { + return err + } + + ctx := context.Background() + providerType, providerCfg, err := resolveProviderModuleByName(cfgFile, envName, providerName) + if err != nil { + return err + } + provider, closer, err := resolveIaCProvider(ctx, providerType, providerCfg) + if err != nil { + return fmt.Errorf("load provider %q: %w", providerType, err) + } + if closer != nil { + defer func() { + if cerr := closer.Close(); cerr != nil { + fmt.Fprintf(os.Stderr, "warning: provider %q shutdown: %v\n", providerType, cerr) + } + }() + } + + store, err := resolveStateStore(cfgFile, envName) + if err != nil { + return fmt.Errorf("resolve state store: %w", err) + } + // Note: unlike runInfraImport, dry-run does NOT require a writable + // state backend — the dry-run path probes via provider.Import without + // calling store.SaveResource. The noopStateStore is acceptable for + // dry-run. For the real import path, require a writable backend. + if !dryRun && isNoopStateStore(store) { + return fmt.Errorf("infra import-all requires a writable iac.state backend; add an iac.state module before importing") + } + + n, dispatchErr := runInfraImportAllWithDeps(ctx, provider, providerType, store, resourceType, dryRun) + if outputPath != "" { + if werr := dumpStateToFile(ctx, store, outputPath); werr != nil { + // Output dump is auxiliary; surface as a warning rather than + // overwriting the dispatch error. Operators care about the + // import result first; the dump is a debug-trail bonus. + fmt.Fprintf(os.Stderr, "warning: --output dump to %q failed: %v\n", outputPath, werr) + } + } + if dryRun { + fmt.Printf("dry-run: %d %s zones would be imported via provider %q\n", n, resourceType, providerName) + } else { + fmt.Printf("imported %d %s zones via provider %q\n", n, resourceType, providerName) + } + return dispatchErr +} + +// runInfraImportAllWithDeps is the testable dispatch core. Split from +// runInfraImportAll so unit tests can drive it with stubbed +// IaCProvider + infraStateStore implementations without touching plugin +// discovery, env resolution, or the filesystem. +// +// Contract: +// - provider MUST implement interfaces.EnumeratorAll (the +// IaCProviderEnumerator strict-contract sub-interface). If not, returns +// (0, error) immediately — operators see a clear "provider does not +// support EnumerateAll" message rather than a panic or empty result. +// - Per-zone failures are isolated. Each enumerated cloud ID is imported +// independently; a failure on zone N does not block zone N+1. The total +// count of *successful* imports is returned alongside an error +// summarizing all failures (or nil if zero failures). +// - dryRun=true probes each cloud ID via provider.Import but does NOT +// call store.SaveResource. The import-state result is discarded; only +// the success/failure tier of the call matters. This validates auth + +// reachability without persisting state. +func runInfraImportAllWithDeps(ctx context.Context, provider interfaces.IaCProvider, providerType string, store infraStateStore, resourceType string, dryRun bool) (int, error) { + enumerator, ok := provider.(interfaces.EnumeratorAll) + if !ok { + return 0, fmt.Errorf("provider %q does not implement EnumerateAll (interfaces.EnumeratorAll); cannot bulk-import %s", providerType, resourceType) + } + outputs, err := enumerator.EnumerateAll(ctx, resourceType) + if err != nil { + return 0, fmt.Errorf("enumerate %s via %s: %w", resourceType, providerType, err) + } + imported := 0 + var failures []string + for _, o := range outputs { + if o == nil { + continue + } + zoneName, _ := o.Outputs["zone"].(string) + if zoneName == "" { + zoneName = o.ProviderID + } + if zoneName == "" { + // Skip entries with neither Outputs.zone nor ProviderID — a + // well-formed EnumerateAll output always has at least one; + // surface as a soft failure rather than a hard abort so a + // single malformed provider entry doesn't tank the run. + failures = append(failures, fmt.Sprintf("(unnamed): EnumerateAll output has empty zone + empty ProviderID; skipped")) + continue + } + if dryRun { + if _, perr := provider.Import(ctx, o.ProviderID, resourceType); perr != nil { + failures = append(failures, fmt.Sprintf("%s: dry-run probe failed: %v", zoneName, perr)) + continue + } + fmt.Printf("would import: zone=%s id=%s\n", zoneName, o.ProviderID) + imported++ + continue + } + imported_state, ierr := provider.Import(ctx, o.ProviderID, resourceType) + if ierr != nil { + failures = append(failures, fmt.Sprintf("%s: import: %v", zoneName, ierr)) + continue + } + synth, serr := buildResourceStateFromImport(zoneName, o.ProviderID, resourceType, providerType, imported_state) + if serr != nil { + failures = append(failures, fmt.Sprintf("%s: %v", zoneName, serr)) + continue + } + if perr := store.SaveResource(ctx, synth); perr != nil { + failures = append(failures, fmt.Sprintf("%s: save: %v", zoneName, perr)) + continue + } + imported++ + } + if len(failures) > 0 { + return imported, fmt.Errorf("import-all completed with %d failure(s):\n %s", len(failures), strings.Join(failures, "\n ")) + } + return imported, nil +} + +// buildResourceStateFromImport synthesizes an interfaces.ResourceState from +// an EnumerateAll output paired with the provider's Import result. The +// ResourceSpec is fabricated from the zone name (the user-facing identifier) +// because import-all has no per-zone spec in the config — the operator is +// importing zones that may not yet be declared. +// +// Reuses resourceStateFromImportedState (workflow/cmd/wfctl/infra.go:1198) +// for ProviderID resolution + AppliedConfig hashing + timestamp normalization +// so the synthesized state matches the single-resource import path exactly. +func buildResourceStateFromImport(zoneName, cloudID, resourceType, providerType string, imported *interfaces.ResourceState) (interfaces.ResourceState, error) { + spec := interfaces.ResourceSpec{ + Name: sanitizeImportedZoneName(zoneName), + Type: resourceType, + } + return resourceStateFromImportedState(spec, providerType, imported, cloudID) +} + +// sanitizeImportedZoneName converts a zone identifier (typically a FQDN like +// "example.test") into a name suitable for use as ResourceState.ID. Dots and +// other characters that are valid in a domain but problematic in YAML keys +// + filesystem-state paths are replaced with hyphens. Idempotent: an already +// sanitized input passes through unchanged. +func sanitizeImportedZoneName(zone string) string { + if zone == "" { + return zone + } + // Mirror existing slug conventions in the codebase (alphanumerics, + // hyphens, underscores). Replace anything else with hyphen so the + // result is filesystem + YAML safe across all state backends. + out := make([]byte, 0, len(zone)) + for i := 0; i < len(zone); i++ { + c := zone[i] + switch { + case c >= 'a' && c <= 'z', + c >= 'A' && c <= 'Z', + c >= '0' && c <= '9', + c == '-', c == '_': + out = append(out, c) + default: + out = append(out, '-') + } + } + return string(out) +} + +// resolveProviderModuleByName resolves an iac.provider module by name from +// the config file, returning (a) the plugin discriminator (from +// modCfg["provider"]) used to dispatch to a concrete provider plugin and +// (b) the fully env-resolved module config. +// +// Mirrors resolveProviderForSpec (workflow/cmd/wfctl/infra.go:1157) but +// indexes the modules slice by NAME rather than by an iac_provider/provider +// reference on a ResourceSpec. The bulk-import wrapper has no spec; the +// operator picks the provider module directly via --provider. +// +// Implementation pinned to mirror resolveProviderForSpec exactly (cycle-4 +// adversarial finding I2): +// - Range over INDEX (`for i := range cfg.Modules` + `m := &cfg.Modules[i]`) +// because m.ResolveForEnv has a pointer receiver. +// - m.ResolveForEnv returns (*config.ResolvedModule, bool) — guard via +// `if !ok` rather than `err != nil`. +// - config.ExpandEnvInMapPreservingKeys returns a single +// map[string]any — not (map, error). Single-value assignment. +func resolveProviderModuleByName(cfgFile, envName, name string) (string, map[string]any, error) { + cfg, err := config.LoadFromFile(cfgFile) + if err != nil { + return "", nil, fmt.Errorf("load %s: %w", cfgFile, err) + } + for i := range cfg.Modules { + m := &cfg.Modules[i] + if m.Type != "iac.provider" || m.Name != name { + continue + } + var modCfg map[string]any + if envName != "" { + resolved, ok := m.ResolveForEnv(envName) + if !ok { + return "", nil, fmt.Errorf("provider module %q is disabled for environment %q", name, envName) + } + modCfg = config.ExpandEnvInMapPreservingKeys(resolved.Config, infraPreserveKeys) + } else { + modCfg = config.ExpandEnvInMapPreservingKeys(m.Config, infraPreserveKeys) + } + providerType, _ := modCfg["provider"].(string) + if providerType == "" { + return "", nil, fmt.Errorf("provider module %q has no 'provider' type configured", name) + } + return providerType, modCfg, nil + } + return "", nil, fmt.Errorf("no iac.provider module named %q in config", name) +} + +// dumpStateToFile snapshots the current state-store contents to outputPath +// as a JSON object with a "resources" array. Intended for scenario test +// harnesses that diff state across runs without re-reading the live state +// backend (which may be remote or expensive to query). +// +// The 0o600 file mode mirrors the sister state-emit paths (configHashMap, +// fsWfctlStateStore) so secrets in AppliedConfig/Outputs are not +// world-readable. Operators wiring this into CI should treat the output as +// sensitive. +func dumpStateToFile(ctx context.Context, store infraStateStore, path string) error { + resources, err := store.ListResources(ctx) + if err != nil { + return fmt.Errorf("list resources: %w", err) + } + data, err := json.MarshalIndent(map[string]any{"resources": resources}, "", " ") + if err != nil { + return fmt.Errorf("marshal: %w", err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + return fmt.Errorf("write %s: %w", path, err) + } + return nil +} diff --git a/cmd/wfctl/infra_import_all_test.go b/cmd/wfctl/infra_import_all_test.go new file mode 100644 index 00000000..29de9f59 --- /dev/null +++ b/cmd/wfctl/infra_import_all_test.go @@ -0,0 +1,38 @@ +package main + +import ( + "strings" + "testing" +) + +// TestRunInfraImportAll_requiresProvider pins the contract: import-all without +// --provider fails fast with a clear error pointing at the missing flag. +// Mirrors the sister guard in runInfraImport which requires --name. Catches +// the regression where the dispatch core silently defaults to the empty +// provider name and falls through to a generic "no module named \"\"" error +// that doesn't help operators. +func TestRunInfraImportAll_requiresProvider(t *testing.T) { + err := runInfraImportAll([]string{}) + if err == nil { + t.Fatal("expected error from runInfraImportAll with no flags; got nil") + } + if !strings.Contains(err.Error(), "--provider") { + t.Fatalf("error %q should mention --provider; got %v", err.Error(), err) + } +} + +// TestRunInfraImportAll_requiresType pins the second-required-flag contract: +// after --provider passes, --type must also be set. Catches the regression +// where the implementation only validates --provider and lets a missing --type +// fall through to enumerator.EnumerateAll("") which surfaces as a generic +// "resource type not supported" error from the provider plugin instead of a +// clear CLI-level error. +func TestRunInfraImportAll_requiresType(t *testing.T) { + err := runInfraImportAll([]string{"--provider", "digitalocean"}) + if err == nil { + t.Fatal("expected error from runInfraImportAll with --provider but no --type; got nil") + } + if !strings.Contains(err.Error(), "--type") { + t.Fatalf("error %q should mention --type; got %v", err.Error(), err) + } +} From 10dd0e250aa4058afeded084facd0fa23c0cb531 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 26 May 2026 21:41:16 -0400 Subject: [PATCH 2/4] test(wfctl): cover import-all dispatch (happy, dry-run, isolation, missing enumerator, dump) --- cmd/wfctl/infra_import_all_test.go | 364 +++++++++++++++++++++++++++++ 1 file changed, 364 insertions(+) diff --git a/cmd/wfctl/infra_import_all_test.go b/cmd/wfctl/infra_import_all_test.go index 29de9f59..8317ed86 100644 --- a/cmd/wfctl/infra_import_all_test.go +++ b/cmd/wfctl/infra_import_all_test.go @@ -1,8 +1,16 @@ package main import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" "strings" + "sync" "testing" + + "github.com/GoCodeAlone/workflow/interfaces" ) // TestRunInfraImportAll_requiresProvider pins the contract: import-all without @@ -36,3 +44,359 @@ func TestRunInfraImportAll_requiresType(t *testing.T) { t.Fatalf("error %q should mention --type; got %v", err.Error(), err) } } + +// ── stubImportAllProvider ───────────────────────────────────────────────────── + +// stubImportAllProvider implements interfaces.IaCProvider + the optional +// interfaces.EnumeratorAll sub-interface so the dispatch core's runtime +// type-assertion succeeds. Test-local: only Import + EnumerateAll receive +// meaningful behavior; everything else returns zero-value to satisfy the +// full IaCProvider surface. +type stubImportAllProvider struct { + mu sync.Mutex + enumerateAll func(ctx context.Context, resourceType string) ([]*interfaces.ResourceOutput, error) + importFn func(ctx context.Context, cloudID, resourceType string) (*interfaces.ResourceState, error) + importCalls []importCall // captured for assertions + enumerateCalls []string // resourceType values from each EnumerateAll call + enumerateAllErr error // when non-nil, EnumerateAll returns this regardless of enumerateAll fn +} + +type importCall struct { + CloudID string + ResourceType string +} + +func (s *stubImportAllProvider) Name() string { return "stub" } +func (s *stubImportAllProvider) Version() string { return "0.0.0" } +func (s *stubImportAllProvider) Initialize(_ context.Context, _ map[string]any) error { return nil } +func (s *stubImportAllProvider) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } +func (s *stubImportAllProvider) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return nil, nil +} +func (s *stubImportAllProvider) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return nil, nil +} +func (s *stubImportAllProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + return nil, nil +} +func (s *stubImportAllProvider) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + return nil, nil +} +func (s *stubImportAllProvider) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + return nil, nil +} +func (s *stubImportAllProvider) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + return nil, nil +} +func (s *stubImportAllProvider) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return nil, fmt.Errorf("stubImportAllProvider: ResourceDriver not implemented") +} +func (s *stubImportAllProvider) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { + return nil, nil +} +func (s *stubImportAllProvider) Close() error { return nil } +func (s *stubImportAllProvider) SupportedCanonicalKeys() []string { return nil } + +func (s *stubImportAllProvider) Import(ctx context.Context, cloudID, resourceType string) (*interfaces.ResourceState, error) { + s.mu.Lock() + s.importCalls = append(s.importCalls, importCall{CloudID: cloudID, ResourceType: resourceType}) + s.mu.Unlock() + if s.importFn == nil { + return nil, fmt.Errorf("stub: importFn not set") + } + return s.importFn(ctx, cloudID, resourceType) +} + +// EnumerateAll satisfies interfaces.EnumeratorAll. The stub MUST implement +// it on the value-receiver path that matches the IaCProvider's pointer +// receiver, so the type assertion in runInfraImportAllWithDeps succeeds. +func (s *stubImportAllProvider) EnumerateAll(ctx context.Context, resourceType string) ([]*interfaces.ResourceOutput, error) { + s.mu.Lock() + s.enumerateCalls = append(s.enumerateCalls, resourceType) + s.mu.Unlock() + if s.enumerateAllErr != nil { + return nil, s.enumerateAllErr + } + if s.enumerateAll == nil { + return nil, nil + } + return s.enumerateAll(ctx, resourceType) +} + +// Compile-time assertion: the stub really does satisfy both interfaces. +// If a future workflow SDK revision adds a method, this fails fast at +// compile time rather than at test-time via a nil-pointer panic. +var ( + _ interfaces.IaCProvider = (*stubImportAllProvider)(nil) + _ interfaces.EnumeratorAll = (*stubImportAllProvider)(nil) + _ interfaces.IaCProvider = (*providerImportOnly)(nil) +) + +// providerImportOnly implements IaCProvider but NOT EnumeratorAll. The +// dispatch core's runtime type assertion against EnumeratorAll must fail +// cleanly on this type, returning (0, error) — not panic, not proceed with +// a nil enumerator. Drives TestRunInfraImportAllWithDeps_requiresEnumerator. +type providerImportOnly struct { + importFn func(ctx context.Context, cloudID, resourceType string) (*interfaces.ResourceState, error) +} + +func (p *providerImportOnly) Name() string { return "import-only" } +func (p *providerImportOnly) Version() string { return "0.0.0" } +func (p *providerImportOnly) Initialize(_ context.Context, _ map[string]any) error { return nil } +func (p *providerImportOnly) Capabilities() []interfaces.IaCCapabilityDeclaration { return nil } +func (p *providerImportOnly) Plan(_ context.Context, _ []interfaces.ResourceSpec, _ []interfaces.ResourceState) (*interfaces.IaCPlan, error) { + return nil, nil +} +func (p *providerImportOnly) Apply(_ context.Context, _ *interfaces.IaCPlan) (*interfaces.ApplyResult, error) { + return nil, nil +} +func (p *providerImportOnly) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { + return nil, nil +} +func (p *providerImportOnly) Status(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.ResourceStatus, error) { + return nil, nil +} +func (p *providerImportOnly) DetectDrift(_ context.Context, _ []interfaces.ResourceRef) ([]interfaces.DriftResult, error) { + return nil, nil +} +func (p *providerImportOnly) Import(ctx context.Context, cloudID, resourceType string) (*interfaces.ResourceState, error) { + if p.importFn == nil { + return nil, nil + } + return p.importFn(ctx, cloudID, resourceType) +} +func (p *providerImportOnly) ResolveSizing(_ string, _ interfaces.Size, _ *interfaces.ResourceHints) (*interfaces.ProviderSizing, error) { + return nil, nil +} +func (p *providerImportOnly) ResourceDriver(_ string) (interfaces.ResourceDriver, error) { + return nil, fmt.Errorf("not implemented") +} +func (p *providerImportOnly) BootstrapStateBackend(_ context.Context, _ map[string]any) (*interfaces.BootstrapResult, error) { + return nil, nil +} +func (p *providerImportOnly) Close() error { return nil } +func (p *providerImportOnly) SupportedCanonicalKeys() []string { return nil } + +// ── dispatch-core tests ─────────────────────────────────────────────────────── + +// TestRunInfraImportAllWithDeps_dispatch is the happy-path contract: given +// a provider that implements EnumeratorAll and returns 2 zones, the +// dispatch core calls Import for each and persists 2 ResourceState rows via +// the state store. Mirrors plan §Task 16 Step 1. +func TestRunInfraImportAllWithDeps_dispatch(t *testing.T) { + stub := &stubImportAllProvider{ + enumerateAll: func(_ context.Context, rt string) ([]*interfaces.ResourceOutput, error) { + if rt != "infra.dns" { + return nil, fmt.Errorf("unexpected resourceType %q", rt) + } + return []*interfaces.ResourceOutput{ + {ProviderID: "alpha.test", Type: "infra.dns", Outputs: map[string]any{"zone": "alpha.test", "ttl": 1800}}, + {ProviderID: "beta.test", Type: "infra.dns", Outputs: map[string]any{"zone": "beta.test", "ttl": 3600}}, + }, nil + }, + importFn: func(_ context.Context, cloudID, rt string) (*interfaces.ResourceState, error) { + // Mirror what a real provider returns: ProviderID populated, Name + // reflects the cloud-side identifier so resourceStateFromImportedState + // can hash + persist without re-reading. + return &interfaces.ResourceState{ + ID: cloudID, + Name: cloudID, + Type: rt, + ProviderID: cloudID, + Outputs: map[string]any{"zone": cloudID}, + }, nil + }, + } + store := &fakeStateStore{} + n, err := runInfraImportAllWithDeps(context.Background(), stub, "stub", store, "infra.dns", false) + if err != nil { + t.Fatalf("dispatch: %v", err) + } + if n != 2 { + t.Errorf("imported = %d; want 2", n) + } + saved, _ := store.ListResources(context.Background()) + if len(saved) != 2 { + t.Fatalf("store saved %d resources; want 2: %+v", len(saved), saved) + } + // Each ResourceState must carry the spec name (sanitized zone name) + + // the provider type so the state-store row is identifiable by operators + // later via `wfctl infra state list`. + for _, s := range saved { + if s.Provider != "stub" { + t.Errorf("state.Provider = %q; want %q", s.Provider, "stub") + } + if s.Type != "infra.dns" { + t.Errorf("state.Type = %q; want %q", s.Type, "infra.dns") + } + if s.ProviderID == "" { + t.Errorf("state.ProviderID empty for %+v", s) + } + } + if len(stub.importCalls) != 2 { + t.Errorf("Import called %d times; want 2", len(stub.importCalls)) + } + if len(stub.enumerateCalls) != 1 || stub.enumerateCalls[0] != "infra.dns" { + t.Errorf("EnumerateAll calls = %+v; want [infra.dns]", stub.enumerateCalls) + } +} + +// TestRunInfraImportAllWithDeps_dryRun pins that --dry-run probes via +// Import but does NOT persist any state. Catches the regression where the +// dry-run branch silently calls SaveResource and writes through to the +// configured state backend. +func TestRunInfraImportAllWithDeps_dryRun(t *testing.T) { + stub := &stubImportAllProvider{ + enumerateAll: func(_ context.Context, _ string) ([]*interfaces.ResourceOutput, error) { + return []*interfaces.ResourceOutput{ + {ProviderID: "x.test", Outputs: map[string]any{"zone": "x.test"}}, + }, nil + }, + importFn: func(_ context.Context, _, _ string) (*interfaces.ResourceState, error) { + return &interfaces.ResourceState{ProviderID: "x.test", Type: "infra.dns", Name: "x-test"}, nil + }, + } + store := &fakeStateStore{} + n, err := runInfraImportAllWithDeps(context.Background(), stub, "stub", store, "infra.dns", true) + if err != nil { + t.Fatalf("dry-run: %v", err) + } + if n != 1 { + t.Errorf("dry-run reported %d would-import; want 1", n) + } + saved, _ := store.ListResources(context.Background()) + if len(saved) != 0 { + t.Errorf("dry-run wrote %d states to store; want 0 (dry-run must not persist)", len(saved)) + } + if len(stub.importCalls) != 1 { + t.Errorf("dry-run Import calls = %d; want 1 (probe for each zone)", len(stub.importCalls)) + } +} + +// TestRunInfraImportAllWithDeps_perZoneFailureIsolation pins that a single +// zone's import failure does NOT abort subsequent zones. The successful +// imports are persisted; the failure surfaces in the returned error. Mirrors +// the design's "per-zone failure isolated" contract. +func TestRunInfraImportAllWithDeps_perZoneFailureIsolation(t *testing.T) { + stub := &stubImportAllProvider{ + enumerateAll: func(_ context.Context, _ string) ([]*interfaces.ResourceOutput, error) { + return []*interfaces.ResourceOutput{ + {ProviderID: "ok-1.test", Outputs: map[string]any{"zone": "ok-1.test"}}, + {ProviderID: "fail.test", Outputs: map[string]any{"zone": "fail.test"}}, + {ProviderID: "ok-2.test", Outputs: map[string]any{"zone": "ok-2.test"}}, + }, nil + }, + importFn: func(_ context.Context, cloudID, rt string) (*interfaces.ResourceState, error) { + if cloudID == "fail.test" { + return nil, fmt.Errorf("simulated transient: %s", cloudID) + } + return &interfaces.ResourceState{ProviderID: cloudID, Type: rt, Name: cloudID}, nil + }, + } + store := &fakeStateStore{} + n, err := runInfraImportAllWithDeps(context.Background(), stub, "stub", store, "infra.dns", false) + if err == nil { + t.Fatal("want error summarizing per-zone failures; got nil") + } + if !strings.Contains(err.Error(), "fail.test") { + t.Errorf("error must mention failing zone; got %v", err) + } + if n != 2 { + t.Errorf("successful imports = %d; want 2 (the two non-failing zones)", n) + } + saved, _ := store.ListResources(context.Background()) + if len(saved) != 2 { + t.Errorf("store saved %d states; want 2 (the two non-failing zones)", len(saved)) + } +} + +// TestRunInfraImportAllWithDeps_requiresEnumerator pins that providers +// which don't implement IaCProviderEnumerator surface a clear error rather +// than panicking. Operators see "provider does not support EnumerateAll" +// instead of a runtime crash when targeting a provider that hasn't shipped +// the optional sub-contract yet. +func TestRunInfraImportAllWithDeps_requiresEnumerator(t *testing.T) { + p := &providerImportOnly{} + store := &fakeStateStore{} + n, err := runInfraImportAllWithDeps(context.Background(), p, "import-only", store, "infra.dns", false) + if err == nil { + t.Fatal("want error for provider missing EnumerateAll; got nil") + } + if !strings.Contains(err.Error(), "EnumerateAll") { + t.Errorf("error must mention EnumerateAll; got %v", err) + } + if n != 0 { + t.Errorf("n = %d; want 0 (no work attempted)", n) + } +} + +// TestRunInfraImportAllWithDeps_enumerateError pins the early-abort path: +// when EnumerateAll itself errors (auth failure, network, etc.), the +// dispatch core returns (0, error) without calling Import or SaveResource. +func TestRunInfraImportAllWithDeps_enumerateError(t *testing.T) { + sentinel := errors.New("auth: unauthorized") + stub := &stubImportAllProvider{enumerateAllErr: sentinel} + store := &fakeStateStore{} + n, err := runInfraImportAllWithDeps(context.Background(), stub, "stub", store, "infra.dns", false) + if err == nil || !errors.Is(err, sentinel) { + t.Fatalf("want wrapped sentinel error; got %v", err) + } + if n != 0 { + t.Errorf("n = %d; want 0 on enumerate failure", n) + } + if len(stub.importCalls) != 0 { + t.Errorf("Import should not be called when enumerate fails; got %d calls", len(stub.importCalls)) + } +} + +// TestDumpStateToFile pins the --output dump contract: state-store +// snapshot is written as JSON with a top-level "resources" array, file mode +// is 0o600 (sensitive — AppliedConfig + Outputs may contain creds). +func TestDumpStateToFile(t *testing.T) { + store := &fakeStateStore{} + _ = store.SaveResource(context.Background(), interfaces.ResourceState{ + Name: "alpha", Type: "infra.dns", ProviderID: "alpha.test", + }) + dir := t.TempDir() + out := filepath.Join(dir, "snapshot.json") + if err := dumpStateToFile(context.Background(), store, out); err != nil { + t.Fatalf("dump: %v", err) + } + info, err := os.Stat(out) + if err != nil { + t.Fatalf("stat: %v", err) + } + if info.Mode().Perm() != 0o600 { + t.Errorf("file mode = %v; want 0o600 (state may contain secrets)", info.Mode().Perm()) + } + data, err := os.ReadFile(out) + if err != nil { + t.Fatalf("read: %v", err) + } + if !strings.Contains(string(data), `"resources"`) { + t.Errorf("dump missing 'resources' key: %s", data) + } + if !strings.Contains(string(data), `"alpha.test"`) { + t.Errorf("dump missing alpha.test ProviderID: %s", data) + } +} + +// TestSanitizeImportedZoneName pins the zone-name → state-key mapping: +// dots in FQDNs become hyphens so the resulting name is safe for YAML keys +// and filesystem-backed state paths. Idempotent on already-sanitized input. +func TestSanitizeImportedZoneName(t *testing.T) { + cases := []struct{ in, want string }{ + {"alpha.test", "alpha-test"}, + {"a.b.example.com", "a-b-example-com"}, + {"already-clean", "already-clean"}, + {"under_score", "under_score"}, + {"123-digits", "123-digits"}, + {"", ""}, + } + for _, c := range cases { + got := sanitizeImportedZoneName(c.in) + if got != c.want { + t.Errorf("sanitizeImportedZoneName(%q) = %q; want %q", c.in, got, c.want) + } + } +} From 629e859827bad0e0294395789f9336585275ea9a Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 26 May 2026 21:42:17 -0400 Subject: [PATCH 3/4] test(wfctl): env-gated e2e for infra import-all against real DO plugin --- cmd/wfctl/infra_import_all_e2e_test.go | 121 +++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 cmd/wfctl/infra_import_all_e2e_test.go diff --git a/cmd/wfctl/infra_import_all_e2e_test.go b/cmd/wfctl/infra_import_all_e2e_test.go new file mode 100644 index 00000000..b6aa92b3 --- /dev/null +++ b/cmd/wfctl/infra_import_all_e2e_test.go @@ -0,0 +1,121 @@ +//go:build e2e_dns_import + +// End-to-end smoke test for `wfctl infra import-all` against a real +// workflow-plugin-digitalocean plugin loaded from disk. Gated by: +// +// - build tag `e2e_dns_import` +// - WFCTL_E2E_DNS_IMPORT=1 env var +// - DIGITALOCEAN_TOKEN env var (read access to /v2/domains) +// +// Optional: +// - WFCTL_E2E_DO_PLUGIN_DIR — directory containing the +// workflow-plugin-digitalocean binary; defaults to ../../data/plugins. +// +// Run locally: +// +// WFCTL_E2E_DNS_IMPORT=1 DIGITALOCEAN_TOKEN=$TOKEN \ +// GOWORK=off go test -tags e2e_dns_import \ +// -run TestInfraImportAll_e2e_DO ./cmd/wfctl/... + +package main + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +func TestInfraImportAll_e2e_DO(t *testing.T) { + if os.Getenv("WFCTL_E2E_DNS_IMPORT") != "1" { + t.Skip("set WFCTL_E2E_DNS_IMPORT=1 + DIGITALOCEAN_TOKEN to run") + } + token := os.Getenv("DIGITALOCEAN_TOKEN") + if token == "" { + t.Skip("DIGITALOCEAN_TOKEN not set; cannot run live e2e") + } + + dir := t.TempDir() + stateDir := filepath.Join(dir, "state") + if err := os.MkdirAll(stateDir, 0o700); err != nil { + t.Fatalf("mkdir state: %v", err) + } + cfgPath := filepath.Join(dir, "infra.yaml") + cfgContent := ` +modules: + - name: do-prod + type: iac.provider + config: + provider: digitalocean + token: ${DIGITALOCEAN_TOKEN} + region: nyc3 + - name: local-state + type: iac.state + config: + backend: filesystem + directory: ` + stateDir + ` +` + if err := os.WriteFile(cfgPath, []byte(cfgContent), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + pluginDir := os.Getenv("WFCTL_E2E_DO_PLUGIN_DIR") + args := []string{"--config", cfgPath, "--provider", "do-prod", "--type", "infra.dns", "--dry-run"} + if pluginDir != "" { + args = append(args, "--plugin-dir", pluginDir) + } + if err := runInfraImportAll(args); err != nil { + t.Fatalf("e2e import-all dry-run: %v", err) + } + + // Second pass: real import. Skip if dry-run already reported zero + // zones (account has no DNS zones, can't validate). Tightest contract + // the e2e can assert is: dry-run succeeded; reading the state directory + // after a real import shows non-zero files. + args[len(args)-1] = "--type" + args = append(args, "infra.dns") + // Drop the --dry-run flag for the real pass. + args = dropFlag(args, "--dry-run") + if err := runInfraImportAll(args); err != nil { + t.Fatalf("e2e import-all real: %v", err) + } + // Snapshot the state-store contents by listing the filesystem backend. + store, err := resolveStateStore(cfgPath, "") + if err != nil { + t.Fatalf("resolve state: %v", err) + } + resources, err := store.ListResources(context.Background()) + if err != nil { + t.Fatalf("list state: %v", err) + } + if len(resources) == 0 { + t.Log("note: account has zero DNS zones; e2e validated dispatch + flag plumbing only") + return + } + for _, r := range resources { + if r.Type != "infra.dns" { + t.Errorf("state resource %q has Type=%q; want infra.dns", r.Name, r.Type) + } + if r.Provider != "digitalocean" { + t.Errorf("state resource %q has Provider=%q; want digitalocean", r.Name, r.Provider) + } + if r.ProviderID == "" { + t.Errorf("state resource %q has empty ProviderID", r.Name) + } + } + t.Logf("e2e: imported %d DNS zones into local state store", len(resources)) +} + +// dropFlag removes the first occurrence of name from args. Helper for the +// dry-run → real-run transition above; standard library does not expose a +// slice-remove primitive. +func dropFlag(args []string, name string) []string { + out := make([]string, 0, len(args)) + for _, a := range args { + if a == name { + continue + } + out = append(out, a) + } + return out +} From 0b1b3f131e35dc59c99a76c5942db4582ad37a80 Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Tue, 26 May 2026 21:53:02 -0400 Subject: [PATCH 4/4] fix(wfctl): lint S1039 + e2e args mutation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up fixes after PR 786's first CI pass: 1. cmd/wfctl/infra_import_all.go:169 — staticcheck S1039 flagged fmt.Sprintf with no format directives. Replaced with the plain string literal. 2. cmd/wfctl/infra_import_all_e2e_test.go — the dry-run → real-run transition mutated args in-place (args[len(args)-1] = "--type"; append; dropFlag) which assumed --dry-run was always the last element. When WFCTL_E2E_DO_PLUGIN_DIR was set the plugin-dir path took the last slot and the rewrite corrupted the flag list. Rebuild each pass cleanly off a baseArgs slice so the transition is order-independent. dropFlag helper deleted (no longer needed). --- cmd/wfctl/infra_import_all.go | 2 +- cmd/wfctl/infra_import_all_e2e_test.go | 44 +++++++++++--------------- 2 files changed, 19 insertions(+), 27 deletions(-) diff --git a/cmd/wfctl/infra_import_all.go b/cmd/wfctl/infra_import_all.go index 7c61e5ac..ad7ffd5a 100644 --- a/cmd/wfctl/infra_import_all.go +++ b/cmd/wfctl/infra_import_all.go @@ -166,7 +166,7 @@ func runInfraImportAllWithDeps(ctx context.Context, provider interfaces.IaCProvi // well-formed EnumerateAll output always has at least one; // surface as a soft failure rather than a hard abort so a // single malformed provider entry doesn't tank the run. - failures = append(failures, fmt.Sprintf("(unnamed): EnumerateAll output has empty zone + empty ProviderID; skipped")) + failures = append(failures, "(unnamed): EnumerateAll output has empty zone + empty ProviderID; skipped") continue } if dryRun { diff --git a/cmd/wfctl/infra_import_all_e2e_test.go b/cmd/wfctl/infra_import_all_e2e_test.go index b6aa92b3..a6f21d1f 100644 --- a/cmd/wfctl/infra_import_all_e2e_test.go +++ b/cmd/wfctl/infra_import_all_e2e_test.go @@ -60,23 +60,29 @@ modules: } pluginDir := os.Getenv("WFCTL_E2E_DO_PLUGIN_DIR") - args := []string{"--config", cfgPath, "--provider", "do-prod", "--type", "infra.dns", "--dry-run"} + // baseArgs is the canonical flag set without --dry-run; each pass + // builds its own slice off this base so the dry-run vs. real-run + // transition does NOT depend on flag ordering inside `args`. Earlier + // revisions mutated the last element + ran dropFlag, which broke when + // WFCTL_E2E_DO_PLUGIN_DIR was set (the last element became the plugin + // directory path, not --dry-run, and the in-place rewrite corrupted + // the args list). + baseArgs := []string{"--config", cfgPath, "--provider", "do-prod", "--type", "infra.dns"} if pluginDir != "" { - args = append(args, "--plugin-dir", pluginDir) + baseArgs = append(baseArgs, "--plugin-dir", pluginDir) } - if err := runInfraImportAll(args); err != nil { + + dryRunArgs := append([]string(nil), baseArgs...) + dryRunArgs = append(dryRunArgs, "--dry-run") + if err := runInfraImportAll(dryRunArgs); err != nil { t.Fatalf("e2e import-all dry-run: %v", err) } - // Second pass: real import. Skip if dry-run already reported zero - // zones (account has no DNS zones, can't validate). Tightest contract - // the e2e can assert is: dry-run succeeded; reading the state directory - // after a real import shows non-zero files. - args[len(args)-1] = "--type" - args = append(args, "infra.dns") - // Drop the --dry-run flag for the real pass. - args = dropFlag(args, "--dry-run") - if err := runInfraImportAll(args); err != nil { + // Second pass: real import. Tightest contract the e2e can assert is: + // dry-run succeeded; reading the state directory after a real import + // shows non-zero rows (or zero if the account has no DNS zones). + realArgs := append([]string(nil), baseArgs...) + if err := runInfraImportAll(realArgs); err != nil { t.Fatalf("e2e import-all real: %v", err) } // Snapshot the state-store contents by listing the filesystem backend. @@ -105,17 +111,3 @@ modules: } t.Logf("e2e: imported %d DNS zones into local state store", len(resources)) } - -// dropFlag removes the first occurrence of name from args. Helper for the -// dry-run → real-run transition above; standard library does not expose a -// slice-remove primitive. -func dropFlag(args []string, name string) []string { - out := make([]string, 0, len(args)) - for _, a := range args { - if a == name { - continue - } - out = append(out, a) - } - return out -}