fix(wfctl): honor explicit resource adoption#626
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Updates wfctl infra apply adoption behavior so adopt_existing: true is treated as an explicit signal to adopt resources (including for typed IaC resource drivers), while preserving the special built-in adoption reference behavior for DNS resources.
Changes:
- Treat
adopt_existing: trueas an explicit adoption signal when resolving resource drivers and computing adoption refs. - Preserve built-in DNS adoption refs ahead of generic name-based adoption.
- Add regression tests covering config-based adoption, unsupported drivers, and DNS preservation; mark typed drivers as supporting config adoption.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| cmd/wfctl/infra_apply.go | Extends adoption resolution to honor adopt_existing and adds driver capability probing for config-based adoption. |
| cmd/wfctl/infra_apply_test.go | Adds coverage for explicit config adoption, unsupported driver rejection, and DNS built-in ref preservation. |
| cmd/wfctl/iac_typed_adapter.go | Declares typed resource drivers as supporting config-based adoption. |
Comments suppressed due to low confidence (1)
cmd/wfctl/infra_apply.go:652
- When
ResourceDriver()returns(nil, nil)(which is treated elsewhere as “no driver”), the current logic caches the nil driver indrivers[spec.Type]andadoptionRefForSpecwill silently skip adoption even ifadopt_existing: true(or a built-in adoptable type likeinfra.dns) was explicitly requested. This can result in the resource being created instead of adopted, and also prevents later specs of the same type from re-resolving a now-available driver because the nil is cached.
Suggested fix: treat driver == nil as a resolution failure when explicitAdoptable is true (return an error), and avoid caching nil drivers (only write to drivers when non-nil).
explicitAdoptable := hasBuiltInAdoptionRef(spec.Type) || boolFromAny(spec.Config["adopt_existing"])
driver, ok := drivers[spec.Type]
if !ok {
var err error
driver, err = provider.ResourceDriver(spec.Type)
if err != nil {
if !explicitAdoptable {
continue
}
return nil, fmt.Errorf("%s/%s: resolve resource driver: %w", spec.Type, spec.Name, err)
}
drivers[spec.Type] = driver
}
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
intel352
force-pushed
the
codex/adopt-existing-config
branch
from
1b2dbb6 to
b4f3714
Compare
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
adopt_existing: trueas an explicit adoption signal when resolving resource driversVerification
Review