refactor: extract shared iac.provider discovery + grouping helpers used by plan and apply#549
Merged
Merged
Conversation
Closed
4 tasks
Extract resolveProviderDefs and groupSpecsByProviderRef from the duplicated logic in computePlanForInfraSpecs (infra_plan_provider.go) and applyInfraModules (infra_apply.go) into a new shared file infra_provider_dispatch.go. - Add shared types: providerDef, specGroup (specGroup chosen to avoid conflict with providerGroup in infra_status_drift.go which groups ResourceRefs, not ResourceSpecs) - Add resolveProviderDefs: walks cfg.Modules, filters iac.provider, expands env vars, returns defs + typeCounts + disabled sets - Add groupSpecsByProviderRef: groups specs by provider module ref, returns stable first-reference-in-specs order - Remove duplicate local providerDef/planGroup types from plan path - Remove duplicate local providerDef/provGroup types from apply path - Add infra_provider_dispatch_test.go with unit tests covering both helpers and asserting plan-apply grouping equivalence No behavior change: error messages, ordering, and fallback heuristics are identical to the pre-refactor implementations. Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/07eff01d-aa38-48bf-ada6-5faf96c7de5e Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor shared iac.provider discovery and grouping helper
refactor: extract shared iac.provider discovery + grouping helpers used by plan and apply
May 5, 2026
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
intel352
marked this pull request as ready for review
May 5, 2026 13:35
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors wfctl infra planning and applying to share a single implementation for discovering iac.provider modules (including per-env resolution + env-var expansion) and for grouping infra resource specs by provider reference, reducing the risk of plan/apply drift for the same config.
Changes:
- Added shared helpers (
resolveProviderDefs,groupSpecsByProviderRef) and shared structs (providerDef,specGroup) in a new dispatch helper file. - Updated both the plan path (
computePlanForInfraSpecs) and apply path (applyInfraModules) to use the shared helpers instead of duplicated logic. - Added unit tests covering provider discovery, grouping behavior, and error paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| cmd/wfctl/infra_provider_dispatch.go | New shared provider discovery + spec grouping helpers used by both plan and apply. |
| cmd/wfctl/infra_provider_dispatch_test.go | New unit tests for the shared helper functions and grouping behavior. |
| cmd/wfctl/infra_plan_provider.go | Removes duplicated provider discovery/grouping and delegates to shared helpers. |
| cmd/wfctl/infra_apply.go | Removes duplicated provider discovery/grouping and delegates to shared helpers; updates grouping type usage. |
Contributor
|
@copilot apply changes based on the comments in this thread |
- Guard against nil spec.Config in groupSpecsByProviderRef to return the 'missing provider field' error instead of panicking - Add TestGroupSpecsByProviderRef_NilConfigTreatedAsMissingProvider - Fix comment in infra_apply.go: 'declaration order' → 'first-reference order' - Fix test name typo: EmptyProvTyp → EmptyProvType - Replace TestPlanApplyGroupingEquivalence with TestPlanApplyGroupingEquivalence_ViaRealEntryPoints, which exercises both computePlanForInfraSpecs and applyInfraModules via the real package-level computeInfraPlan and resolveIaCProvider seams rather than calling the shared helper twice Agent-Logs-Url: https://github.com/GoCodeAlone/workflow/sessions/f6d6cba5-fc43-43f4-bd3f-6624e7669eab Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
Contributor
Author
Applied all four review changes in e466520:
|
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.
computePlanForInfraSpecsandapplyInfraModulesindependently duplicated the same ~50 lines ofiac.providerdiscovery, env-var resolution, disabled-provider tracking, type-count heuristic, and spec-grouping logic — creating silent drift risk between plan and apply outputs for the same input config.What changed
New
cmd/wfctl/infra_provider_dispatch.goproviderDef— shared type replacing identical local structs in both filesspecGroup— shared struct replacingplanGroup(plan) /provGroup(apply); namedspecGroupto avoid collision with the existingproviderGroupininfra_status_drift.go(which groupsResourceRefs, not specs)resolveProviderDefs(cfg, envName)— single source of truth for walkingcfg.Modules, filteringiac.provider, expanding env vars, and returningdefs + typeCounts + disabledgroupSpecsByProviderRef(specs, defs, disabled, envName)— groups specs byproviderfield in stable first-reference order, with consistent error messages for missing/disabled/unconfigured providersinfra_plan_provider.go/infra_apply.goproviderTypeCountsfallback heuristic are byte-identical to the pre-refactor implementationsNew
cmd/wfctl/infra_provider_dispatch_test.goresolveProviderDefs(basic, env-disabled, duplicate type counts) andgroupSpecsByProviderRef(ordering, all error paths)TestPlanApplyGroupingEquivalence— regression gate asserting both paths produce identical group order and spec membership for the same input config