feat(deploy): wire real plugin loader into resolveIaCProvider#428
Merged
Conversation
- Replace the no-op stub with discoverAndLoadIaCProvider, which scans plugin.json files for capabilities.iacProvider.name, loads the matching plugin via ExternalPluginManager, and type-asserts the iac.provider module factory result to interfaces.IaCProvider. - Each failure mode (no plugin dir, no matching plugin, binary absent, no iac.provider module type, wrong type) returns an actionable error pointing at the fix command. - Refactor pluginDeployProvider to resolve lazily in ensureProvider(ctx) so the real request context is threaded through to Initialize. - Derive resourceType from the matched module's Type field instead of hardcoding "infra.container_service". - Add --plugin-dir flag to wfctl ci run (sets WFCTL_PLUGIN_DIR). Note: workflow-plugin-digitalocean v0.1.0 does not yet expose an iac.provider module type or ServiceInvoker handlers. Until it does, deploying via that plugin returns a clear "upgrade with wfctl plugin update" error rather than a silent no-op. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR wires wfctl deploy’s plugin-based IaC provider resolution to the real external plugin loader, enabling ci run deployments to discover/load an IaC provider from installed plugins (via WFCTL_PLUGIN_DIR / --plugin-dir) and initialize it with the calling context.
Changes:
- Implement plugin discovery + loading for IaC providers by scanning
plugin.jsonforcapabilities.iacProvider.nameand loading viaexternal.ExternalPluginManager. - Refactor
pluginDeployProviderto lazily resolve the IaC provider on firstDeploy/HealthCheck, threading through the realcontext.Context. - Add
--plugin-dirtowfctl ci run(also honoringWFCTL_PLUGIN_DIR) and add unit tests for loader behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/wfctl/deploy_providers.go | Replaces the IaC provider resolver stub with real plugin discovery/loading and adds lazy provider resolution; derives resourceType from the selected module. |
| cmd/wfctl/deploy_plugin_loader_test.go | Adds tests covering default IaC provider resolution error modes and lazy resolution behavior. |
| cmd/wfctl/ci_run.go | Adds --plugin-dir flag and applies it to WFCTL_PLUGIN_DIR during deploy phase. |
Comment on lines
+110
to
+111
| _, statErr := os.Stat(binaryPath) | ||
| return pluginName, statErr == nil, nil |
There was a problem hiding this comment.
findIaCPluginDir treats any os.Stat error on the plugin binary as “binary missing” (hasBinary=false). If Stat fails for reasons other than non-existence (e.g., permission denied, I/O error), the caller will surface an incorrect install hint. Consider distinguishing os.IsNotExist(statErr) from other errors and returning a wrapped error for the non-NotExist cases.
Suggested change
| _, statErr := os.Stat(binaryPath) | |
| return pluginName, statErr == nil, nil | |
| if _, statErr := os.Stat(binaryPath); statErr != nil { | |
| if os.IsNotExist(statErr) { | |
| return pluginName, false, nil | |
| } | |
| return "", false, fmt.Errorf("stat plugin binary %q: %w", binaryPath, statErr) | |
| } | |
| return pluginName, true, nil |
Comment on lines
+19
to
+25
| _, err := resolveIaCProvider(context.Background(), "any-provider", nil) | ||
| if err == nil { | ||
| t.Skip("resolveIaCProvider succeeded unexpectedly") | ||
| } | ||
| if strings.Contains(err.Error(), "no in-process provider loader") { | ||
| t.Errorf("default resolveIaCProvider still returns old placeholder message: %v", err) | ||
| } |
There was a problem hiding this comment.
TestDefaultResolveIaCProvider_IsNotPlaceholder uses t.Skip when resolveIaCProvider unexpectedly succeeds. A success here indicates a regression in the test setup/expectations and should fail the test (e.g., t.Fatalf) rather than being skipped, otherwise the test suite could hide real behavior changes.
| return fmt.Errorf("--env is required for deploy phase") | ||
| } | ||
| if *pluginDir != "" { | ||
| os.Setenv("WFCTL_PLUGIN_DIR", *pluginDir) //nolint:errcheck |
There was a problem hiding this comment.
runCIRun ignores the error return from os.Setenv when applying --plugin-dir. Since Setenv can fail on some platforms/environments, it’s better to check and return a wrapped error so the deploy phase doesn’t proceed with an unexpected plugin directory.
Suggested change
| os.Setenv("WFCTL_PLUGIN_DIR", *pluginDir) //nolint:errcheck | |
| if err := os.Setenv("WFCTL_PLUGIN_DIR", *pluginDir); err != nil { | |
| return fmt.Errorf("set WFCTL_PLUGIN_DIR: %w", err) | |
| } |
Comment on lines
+190
to
201
| // Find the first infra resource module referencing this provider. | ||
| var resourceName, resourceType string | ||
| var resourceCfg map[string]any | ||
| for _, m := range wfCfg.Modules { | ||
| if m.Type != "infra.container_service" { | ||
| if m.Type == "iac.provider" || m.Type == "" { | ||
| continue | ||
| } | ||
| if p, _ := m.Config["provider"].(string); p == providerModName { | ||
| resourceName = m.Name | ||
| resourceType = m.Type | ||
| resourceCfg = m.Config | ||
| break |
There was a problem hiding this comment.
newPluginDeployProvider now selects the first non-empty module whose config.provider matches the iac.provider module name. In configs with multiple infra.* resources (e.g., VPC/firewall/database + container_service), this can pick the wrong resource (like infra.vpc) and then attempt to deploy by injecting an "image" key into that resource’s config, leading to incorrect updates or failures. Consider restricting selection to the intended deployable resource type (likely infra.container_service), or introduce an explicit config option to choose which infra resource to deploy (e.g., by module name), rather than taking the first match.
… resolution - resolveIaCProvider now returns (IaCProvider, io.Closer, error); the Closer wraps mgr.Shutdown() via closerFunc so callers can reap the plugin subprocess when done. - discoverAndLoadIaCProvider calls mgr.Shutdown() on every early-exit error path so no subprocess is leaked when loading fails. - runDeployPhaseWithConfig defers Close() on any provider that implements io.Closer, ensuring the subprocess is reaped after deploy. - pluginDeployProvider.ensureProvider is guarded by sync.Once (safe for concurrent Deploy/HealthCheck) and short-circuits when provider is already set (preserves test injection pattern). - Update all resolveIaCProvider test overrides to the new 3-return signature with nil closer. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Option A vs Option B
Option A (engine build + module walk) was skipped because `iac.provider` has no registered factory in the engine — `workflow.NewEngineBuilder().BuildFromConfig(cfg)` would skip the module rather than instantiate it, so there is nothing to extract. Option B (direct `ExternalPluginManager` + raw JSON manifest scan) is the only path that can discover and load the plugin without requiring engine-side changes.
Known gap: workflow-plugin-digitalocean
`workflow-plugin-digitalocean` v0.1.0 declares `capabilities.iacProvider.name = "digitalocean"` in `plugin.json` but does not expose an `iac.provider` module type or any `ServiceInvoker` handlers. Deploying through it currently returns:
```
plugin "workflow-plugin-digitalocean" does not expose an iac.provider module type — upgrade with: wfctl plugin update workflow-plugin-digitalocean
```
The follow-up work in `workflow-plugin-digitalocean` is to:
Integration check (manual)
Steps run against `/Users/jon/workspace/buymywishlist`:
Step 1 — check plugin registry manifest exists:
```
wfctl plugin search workflow-plugin-digitalocean
```
Result: manifest exists in workflow-registry (added in PR #12).
Step 2 — install plugin binary:
```
wfctl plugin install workflow-plugin-digitalocean --plugin-dir ./data/plugins
```
Result: binary installed to `data/plugins/workflow-plugin-digitalocean/workflow-plugin-digitalocean`.
Step 3 — attempt deploy:
```
DIGITALOCEAN_TOKEN=... SPACES_access_key=... SPACES_secret_key=...
wfctl ci run --phase deploy --env staging --config infra.yaml
--plugin-dir ./data/plugins
```
Actual outcome (expected given known gap above):
```
plugin "workflow-plugin-digitalocean" does not expose an iac.provider module type — upgrade with: wfctl plugin update workflow-plugin-digitalocean
```
Plugin discovery and loading succeeds (ExternalPluginManager log lines appear on stderr); the error is returned at the `ModuleFactories()["iac.provider"]` assertion step — confirming the wiring is correct and the remaining work is entirely in the DO plugin.
Test plan
🤖 Generated with Claude Code