fix(wfctl): disambiguate iac_provider from impl-level provider in resource configs#620
Merged
Merged
Conversation
…ource configs
Plugin schemas like workflow-plugin-eventbus declare "provider" as the
implementation identifier (nats, kafka, ...). wfctl's plan/apply/dispatch
code paths conflated that with the iac.provider module reference, looking
up the impl name as a module name and failing with:
error: plan action for "bmw-eventbus" references provider "nats"
which is not declared as an iac.provider module
The infra.database resource ("provider: do-provider") works because there
the field happens to name the iac.provider module directly. Resources where
"provider" carries impl semantics (eventbus, etc.) need a separate field
for IaC routing.
This change: introduce `iac_provider` as the canonical field for selecting
the iac.provider module to dispatch to. Read it via a new
`resolveIaCProviderRef(map[string]any) string` helper that returns
iac_provider first and falls back to provider for backward compatibility.
Updated all 9 read sites:
cmd/wfctl/infra.go:1034 (resolveProviderForSpec)
cmd/wfctl/infra_provider_dispatch.go:118 (groupSpecsByProviderRef)
cmd/wfctl/infra_apply.go:348/356 (state/spec helpers)
cmd/wfctl/infra_apply.go:557/1340 (post-apply state record builds)
cmd/wfctl/infra_apply.go:1167/1173 (apply group dispatch)
cmd/wfctl/infra_apply_dryrun.go:156/226
cmd/wfctl/infra_destroy.go:69
cmd/wfctl/infra_status_drift.go:213
cloud.account / iac.provider module-internal `provider` reads (deploy.go,
deploy_providers.go, ci_run_dryrun.go) are intentionally left alone —
those refer to the impl-name field on iac.provider modules themselves
(provider: digitalocean), not resource-level routing.
Error messages updated to mention `iac_provider/provider` resolution path
so operators can quickly identify the field set.
Tests: TestResolveIaCProviderRef covers 8 cases including
iac_provider-wins-over-provider, type-mismatch fallback, nil/empty.
Existing Infra/Provider/Apply/Plan/Dispatch test suites green (no
regressions).
Unblocks BMW deploy: bmw-eventbus (provider="nats") now coexists with
iac_provider="do-provider" in infra.yaml without requiring the eventbus
plugin to drop its impl-name semantics.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a clear separation between “implementation provider” (resource-specific provider) and “IaC routing provider” (new canonical iac_provider) in wfctl’s infra plan/apply/destroy/diagnostics paths, preventing dispatch failures when a resource schema already uses provider for non-IaC semantics (e.g., eventbus provider nats/kafka).
Changes:
- Introduces
resolveIaCProviderRef(cfg)to preferiac_providerand fall back toproviderfor backward compatibility. - Updates infra routing/grouping logic across apply/dry-run/destroy/status drift/dispatch to use the new resolver.
- Adds unit tests covering precedence, fallbacks, nil config, and type-mismatch cases.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/wfctl/infra.go | Adds resolveIaCProviderRef and uses it for provider resolution; updates related error strings. |
| cmd/wfctl/infra_apply.go | Switches provider-ref extraction to resolveIaCProviderRef in multiple apply paths; updates error strings. |
| cmd/wfctl/infra_apply_dryrun.go | Uses resolveIaCProviderRef when rendering dry-run JSON and collecting provider groups. |
| cmd/wfctl/infra_destroy.go | Uses resolveIaCProviderRef when grouping destroy operations by provider. |
| cmd/wfctl/infra_provider_dispatch.go | Uses resolveIaCProviderRef in spec grouping and updates dispatch error strings. |
| cmd/wfctl/infra_status_drift.go | Uses resolveIaCProviderRef when inferring provider groups for drift/status reporting. |
| cmd/wfctl/infra_iac_provider_ref_test.go | Adds focused unit coverage for resolveIaCProviderRef behavior. |
| return providerType, modCfg, nil | ||
| } | ||
| return "", nil, fmt.Errorf("infra module %q references provider %q which is not declared as an iac.provider module", spec.Name, moduleRef) | ||
| return "", nil, fmt.Errorf("infra module %q references iac.provider module %q (resolved from iac_provider/provider) which is not declared in modules", spec.Name, moduleRef) |
| return nil, nil, fmt.Errorf("infra module %q references iac.provider %q which is disabled for environment %q", spec.Name, moduleRef, envName) | ||
| } | ||
| return nil, nil, fmt.Errorf("infra module %q references provider %q which is not declared as an iac.provider module", spec.Name, moduleRef) | ||
| return nil, nil, fmt.Errorf("infra module %q references iac.provider module %q (resolved from iac_provider/provider) which is not declared in modules", spec.Name, moduleRef) |
| def, ok := providerDefs[moduleRef] | ||
| if !ok { | ||
| return runHydrated, fmt.Errorf("plan action for %q references provider %q which is not declared as an iac.provider module", action.Resource.Name, moduleRef) | ||
| return runHydrated, fmt.Errorf("plan action for %q references iac.provider module %q (resolved from iac_provider/provider) which is not declared in modules", action.Resource.Name, moduleRef) |
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
…e" qualifier in error 3 inline findings, all the same nit at infra.go:1081, infra_apply.go:1182, infra_provider_dispatch.go:129. My initial change widened the wording "not declared as an iac.provider module" → "not declared in modules". The lookup map is filtered to type=iac.provider modules; if a same-named module of a different type exists, "not declared in modules" is inaccurate. Restored the precise qualifier and tightened the field reference to read "iac_provider/provider field" so the operator knows which fields the lookup key was resolved from. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Round 2 (5da12c9): restored |
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.
Why
BMW deploy run 25625555953 (attempt 2) failed with:
```
error: plan action for "bmw-eventbus" references provider "nats"
which is not declared as an iac.provider module
```
The `bmw-eventbus` resource declares `provider: nats` (the eventbus implementation, per workflow-plugin-eventbus's typed proto schema where `ClusterConfig.provider` ∈ {nats, kafka}) and `deploy_target: digitalocean.app_platform` (the IaC target hint).
wfctl's plan/apply code reads `Resource.Config["provider"]` for iac.provider routing and looks up "nats" in the iac.provider module set — there is no NATS iac.provider, so dispatch fails. `infra.database` works because there `provider: do-provider` happens to name the iac.provider module directly. Resources where `provider` carries impl semantics (eventbus today, possibly others in the future) need a separate field for IaC routing.
What
Introduce `iac_provider` as the canonical field for selecting the iac.provider module to dispatch to. New helper:
```go
func resolveIaCProviderRef(cfg map[string]any) string {
if v, ok := cfg["iac_provider"].(string); ok && v != "" { return v }
if v, ok := cfg["provider"].(string); ok { return v }
return ""
}
```
Reads `iac_provider` first, falls back to `provider` for backward compatibility. Updated all 9 read sites across infra.go, infra_apply.go, infra_apply_dryrun.go, infra_destroy.go, infra_provider_dispatch.go, infra_status_drift.go.
Module-internal `provider` reads (cloud.account, iac.provider's own `provider: digitalocean`) intentionally left alone — those name impl types on the module itself, not resource-level routing.
What this enables
bmw-eventbus can now declare both fields without conflict:
```yaml
type: infra.eventbus
config:
provider: nats # impl (read by eventbus plugin)
iac_provider: do-provider # IaC routing (read by wfctl)
deploy_target: digitalocean.app_platform
version: "2.10"
replicas: 2
```
infra.database keeps working unchanged via the fallback.
Tests
```
$ GOWORK=off go test ./cmd/wfctl/ -run 'TestResolveIaCProviderRef' -v
... 8 sub-tests PASS
$ GOWORK=off go test ./cmd/wfctl/ -run 'Infra|Provider|Apply|Plan|Dispatch' -count=1
ok github.com/GoCodeAlone/workflow/cmd/wfctl 4.897s
```
Test plan
🤖 Generated with Claude Code