fix(reverse-import): wire closure-capable discoverer into the job (#195)#196
Merged
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
1cf1b0a to
2bc93c1
Compare
The reverse-import job built reverseimport.Options with neither Discoverer nor ClosureDiscoverer set, so the presets engine emitted the selection_closure_unavailable diagnostic and silently skipped both dependency-closure expansion (the 'auto-included N dependencies' behavior the fast-discovery initiative relies on) and dep-chase. Hand the engine a closure-capable cloud discoverer built from the shared presets constructor (reversedisco.New, added in insideout-terraform-presets#738). The concrete adapter implements both reverseimport.Discoverer (DiscoverByID) and reverseimport.ClosureDiscoverer (DiscoverClosure), and the engine resolves the closure surface from Options.Discoverer when it is closure-capable, so one field wires both. Details: - Construction is lazy (lazyDiscoverer): the real cloud client is built on first use by the engine's closure/dep-chase phases, which run after the engine's cheap selection validation. Eagerly dialing the cloud would let a credential/API gap (e.g. GCP ADC missing) mask the structured validation result the engine produces for an invalid selection. - Cloud/region/GCP-project are derived from the request resources when the flags are omitted, mirroring how reverseimport.Run derives the cloud, so the discoverer targets the correct provider regardless of dispatch flags. - DiscoverRegions carries every distinct region across the selected resources (or the single --region override) so closure discovery scans all selected regions for a multi-region request rather than only the first. - newDiscoverer is a package var defaulting to reversedisco.New; tests swap in a fake to assert the Options wiring, region scope, and validation-first ordering without standing up real cloud clients. Bumps insideout-terraform-presets to the commit that exports reversedisco.New.
2bc93c1 to
00fb610
Compare
presets#738 merged as 4669c80c; move the go.mod require off the PR-branch commit (ef92b38) onto the merge commit so the dependency stays reachable after the presets branch is deleted.
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.
Closes #195
Problem
The Mars reverse-import job built
reverseimport.Options{…}with neitherDiscoverernorClosureDiscovererset. Both stayed nil, so the presets reverse-import engine emitted theselection_closure_unavailablediagnostic and silently skipped:opts.Discoverer != nil).Found during reliable's #2040 fast-discovery e2e on staging: the diagnostic appeared in the plan artifact for reverse run
ri-ea0b6f37-vvwqf. The presets CLI wires this correctly; the omission was squarely in Mars.Fix
reversedisco.New(added in feat(reverse-import): export shared closure-capable discoverer constructor insideout-terraform-presets#738). The concrete adapter implements bothreverseimport.Discoverer(DiscoverByID) andreverseimport.ClosureDiscoverer(DiscoverClosure); the engine resolves the closure surface fromOptions.Discovererwhen it is closure-capable, so one field wires both.lazyDiscoverer): the real cloud client is built on first use by the engine's closure/dep-chase phases, which run after the engine's cheap selection validation. Building it eagerly would let a credential/API gap (e.g. GCP ADC missing) mask the structured validation result the engine produces for an invalid selection.reverseimport.Runderives the cloud, so the discoverer targets the correct provider regardless of dispatch flags.DiscoverRegionscarries every distinct region across the selected resources (or the single--regionoverride) so closure discovery scans all selected regions for a multi-region request, not just the first.newDiscovereris a package var defaulting toreversedisco.New; tests swap in a fake to assert the wiring, region scope, and validation-first ordering without real cloud clients.insideout-terraform-presetsto the commit that exportsreversedisco.New.Rather than duplicate the discoverer wiring in Mars, the constructor was extracted into an exported presets package so the CLI and Mars share one wiring. See luthersystems/insideout-terraform-presets#738.
Tests
TestMainWiresClosureDiscovererForRegisteredParents— selects a registered parent type (aws_kms_key) and assertsMainpasses the engine a non-nilOptions.Discovererimplementingreverseimport.ClosureDiscoverer. Verified failing before the fix.TestMainValidatesSelectionBeforeBuildingCloudClient— a failing discoverer factory must not mask the engine's selection validation; asserts the cloud client is never constructed before validation rejects anInsideOutImportedselection. Verified failing under eager construction.TestMainScansAllSelectedRegionsForClosure/TestMainRegionOverrideWinsForClosureScope— multi-region closure scope and--regionoverride.TestMainDecodesRequestAndPassesOptionsalso asserts the discoverer and region scope are wired, using the real productionnewDiscoverer.Full mars suite green (15 tests in the touched package, 17 packages overall),
go vetclean, gofmt-clean; theinsideout-reverse-importbinary cross-compiles.