feat(deploy): add remoteIaCProvider adapter for gRPC plugin IaC dispatch#429
Merged
feat(deploy): add remoteIaCProvider adapter for gRPC plugin IaC dispatch#429
Conversation
RemoteModule (the result of ModuleFactories()["iac.provider"]) implements service invocation via InvokeService but does NOT directly satisfy interfaces.IaCProvider. Replace the failing type assertion with a remoteIaCProvider wrapper that routes each IaCProvider method call through InvokeService, and a remoteResourceDriver that routes Update/HealthCheck. - remoteIaCProvider: Name, Version, Initialize, ResourceDriver; all other methods (Plan, Apply, etc.) return a clear error pointing at wfctl infra apply - remoteResourceDriver: Update and HealthCheck fully wired; Create/Read/Delete return actionable errors - Tests cover happy-path dispatch, error propagation, and the compile-time guarantee that remoteIaCProvider satisfies interfaces.IaCProvider Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a remoteIaCProvider/remoteResourceDriver adapter layer so wfctl deploy can use external (gRPC) IaC provider plugins whose iac.provider module factory returns a *external.RemoteModule (service-invocation based) rather than an object directly implementing interfaces.IaCProvider.
Changes:
- Replace the failing
interfaces.IaCProvidertype assertion indiscoverAndLoadIaCProviderwith aremoteServiceInvoker-based adapter. - Implement
remoteIaCProviderandremoteResourceDriverto route deploy-relevant calls viaInvokeService. - Add unit tests for the new adapters.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/wfctl/deploy_providers.go | Wrap external plugin iac.provider modules with a service-invocation adapter implementing interfaces.IaCProvider / interfaces.ResourceDriver. |
| cmd/wfctl/deploy_remote_provider_test.go | Adds tests for adapter routing/error propagation and a compile-time interface assertion. |
| } | ||
|
|
||
| func (r *remoteIaCProvider) Destroy(_ context.Context, _ []interfaces.ResourceRef) (*interfaces.DestroyResult, error) { | ||
| return nil, fmt.Errorf("IaCProvider.Destroy not supported via remote deploy — use wfctl infra apply") |
There was a problem hiding this comment.
IaCProvider.Destroy returns an error that tells users to run wfctl infra apply, but there is a dedicated wfctl infra destroy command. This message is likely to misdirect users trying to tear down infra; update it to point at the destroy flow (or otherwise clarify the correct command).
Suggested change
| return nil, fmt.Errorf("IaCProvider.Destroy not supported via remote deploy — use wfctl infra apply") | |
| return nil, fmt.Errorf("IaCProvider.Destroy not supported via remote deploy — use wfctl infra destroy") |
| return nil, fmt.Errorf("ResourceDriver.Read not yet supported via remote deploy — use wfctl infra apply") | ||
| } | ||
| func (d *remoteResourceDriver) Delete(_ context.Context, _ interfaces.ResourceRef) error { | ||
| return fmt.Errorf("ResourceDriver.Delete not yet supported via remote deploy — use wfctl infra apply") |
There was a problem hiding this comment.
ResourceDriver.Delete errors with "use wfctl infra apply", but deletions/teardown are typically handled by wfctl infra destroy in this CLI. Consider adjusting the guidance (or making it explicit why apply is the right next step) so users aren’t sent to the wrong command.
Suggested change
| return fmt.Errorf("ResourceDriver.Delete not yet supported via remote deploy — use wfctl infra apply") | |
| return fmt.Errorf("ResourceDriver.Delete not yet supported via remote deploy — use wfctl infra destroy") |
Comment on lines
+18
to
+26
| func (f *fakeRemoteInvoker) InvokeService(method string, _ map[string]any) (map[string]any, error) { | ||
| if errStr, ok := f.errors[method]; ok { | ||
| return nil, errString(errStr) | ||
| } | ||
| if res, ok := f.methods[method]; ok { | ||
| return res, nil | ||
| } | ||
| return map[string]any{}, nil | ||
| } |
There was a problem hiding this comment.
The fake invoker ignores the args passed to InvokeService, so the tests don’t actually verify the argument contract (resource_type, ref_name, spec_config, etc.) that the adapter is responsible for. Capture the incoming args per method (e.g., store lastArgs[method]) and assert the expected keys/values in the Update/HealthCheck/Initialize tests.
Comment on lines
+165
to
+173
| // TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider verifies that | ||
| // when a plugin's iac.provider module does NOT directly implement IaCProvider | ||
| // (the normal case for gRPC plugins), discoverAndLoadIaCProvider wraps it in | ||
| // remoteIaCProvider instead of failing the type assertion. | ||
| func TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider(t *testing.T) { | ||
| // This is covered end-to-end by the plugin tests; here we just confirm that | ||
| // remoteIaCProvider satisfies interfaces.IaCProvider at compile time. | ||
| var _ interfaces.IaCProvider = (*remoteIaCProvider)(nil) | ||
| } |
There was a problem hiding this comment.
TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider claims to verify wrapping behavior, but it only contains a compile-time interface assertion, so it will pass regardless of discoverAndLoadIaCProvider logic. Either rename it to reflect what it actually checks (e.g., interface satisfaction) and/or move the compile-time assertion to package scope, or add a real behavioral test that exercises discoverAndLoadIaCProvider with a fake module implementing remoteServiceInvoker.
Suggested change
| // TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider verifies that | |
| // when a plugin's iac.provider module does NOT directly implement IaCProvider | |
| // (the normal case for gRPC plugins), discoverAndLoadIaCProvider wraps it in | |
| // remoteIaCProvider instead of failing the type assertion. | |
| func TestDiscoverAndLoadIaCProvider_WrapsModuleAsRemoteIaCProvider(t *testing.T) { | |
| // This is covered end-to-end by the plugin tests; here we just confirm that | |
| // remoteIaCProvider satisfies interfaces.IaCProvider at compile time. | |
| var _ interfaces.IaCProvider = (*remoteIaCProvider)(nil) | |
| } | |
| // Ensure remoteIaCProvider continues to satisfy interfaces.IaCProvider. | |
| // Wrapping behavior for discoverAndLoadIaCProvider is covered by higher-level | |
| // plugin tests; this is only a compile-time interface assertion. | |
| var _ interfaces.IaCProvider = (*remoteIaCProvider)(nil) |
11 tasks
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
Companion to workflow-plugin-digitalocean PR #1.
RemoteModule(the object returned byModuleFactories()["iac.provider"]) implementsInvokeServicefor cross-process dispatch but does not directly satisfyinterfaces.IaCProvider. The type assertion added in PR #428 (mod.(interfaces.IaCProvider)) always fails with a*RemoteModule.This PR replaces that assertion with two adapter types:
remoteIaCProvider— implementsinterfaces.IaCProviderby routingName,Version,Initialize, andResourceDriver(type)throughInvokeService. Methods not needed by the deploy path (Plan,Apply,Destroy, etc.) return a clear error pointing atwfctl infra apply.remoteResourceDriver— implementsinterfaces.ResourceDriverby routingUpdateandHealthCheckthroughInvokeServiceusing the arg convention the DO plugin expects (resource_type,ref_name,ref_type,spec_name,spec_type,spec_config).Create/Read/Deletereturn actionable errors.remoteServiceInvokerlocal interface — satisfied by*external.RemoteModule, avoiding a direct import of the concrete type.Test plan
TestRemoteIaCProvider_Name/Initialize_RoutesViaInvoker/Initialize_PropagatesErrorTestRemoteIaCProvider_ResourceDriver_ReturnsRemoteDriverTestRemoteResourceDriver_Update_RoutesViaInvoker/_PropagatesErrorTestRemoteResourceDriver_HealthCheck_Healthy/_Unhealthyvar _ interfaces.IaCProvider = (*remoteIaCProvider)(nil)assertion./cmd/wfctl/...suite passes;go vetclean🤖 Generated with Claude Code