Fix nested computed read #2181
Fix nested computed read #2181
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2181 +/- ##
==========================================
+ Coverage 62.49% 62.50% +0.01%
==========================================
Files 355 355
Lines 38625 38625
==========================================
+ Hits 24137 24141 +4
+ Misses 12928 12925 -3
+ Partials 1560 1559 -1 ☔ View full report in Codecov by Sentry. |
VenelinMartinov
requested review from
iwahbe,
guineveresaenger and
t0yv0
and removed request for
iwahbe
|
GH rate limits... |
| @@ -494,8 +493,6 @@ func (ctx *conversionContext) makeTerraformInput( | |||
|
|
|||
| var tfflds shim.SchemaMap | |||
|
|
|||
| // We cannot use [shimutil.CastToTypeObject] because we have machinery that constructs invalid | |||
There was a problem hiding this comment.
This is exactly the root cause, I find it unfortunate to remove this comment. elemSchemas produces
(&schema.Schema{Elem: e}).Shim()This is not a valid schema as it does not have Type() defined. This is an artificial schema that you will not find coming from the actual provider, it's a recursion artifact.
There was a problem hiding this comment.
But CastToTypeObject is wrong in the first place for SDKv2 - it checks for .Resource under a Map type which is illegal in SDKV2.
I don't understand why we'd want to use it here anyway - tfs.Elem().(shim.Resource) is the correct way to check for an object type
There was a problem hiding this comment.
I'm not going to block on this change but the Elem() representation is continuously confusing. I don't think "object type" is defined enough for me.
How about #2187 to try to clarify some of the issues here?
There was a problem hiding this comment.
My understanding was that tfs.Elem().(shim.Resource) was insufficient to identify an object type: tfs.Type() == TypeList && tfs.Elem().(shim.Resource) implied a List[Object] where tfs.Type() == TypeMap && tfs.Elem().(shim.Resource) implied an Object type.
There was a problem hiding this comment.
When recurring over a List[Object] type which is indeed tfs.Type() == TypeList && tfs.Elem().(shim.Resource) the code will call elemSchemas to compute a shim.Schema for the Object, which will result in a shim.Schema value that's not recognized by CastToTypeObject. One can argue that it should, in fact, be recognized, and then we could bring that back here and work this out that way. That's a bit more changes though.
This adds test cases for how TF types are handled by `ExtractInputsFromOutputs` in combination with `Default` and `Computed` and `MaxItems: 1` `ExtractInputsFromOutputs` is the function in `schema.go` responsible for generating the inputs from resource outputs when a resource is imported. Its output is what the engine then uses for generating the code for the import. Notably, nested computed values are wrong for both max items one and non-max items one. In #2180 we found a regression there, so this fills in some missing coverage. Extracting the tests to make the changes more apparent and to make sure we don't regress in #2181
VenelinMartinov
changed the base branch from
master
to
vvm/pf_extract_inputs_from_outputs
|
After adding the PF tests it looks like this affects PF too. I'll add that to the issue and PR description. |
This adds adds missing test coverage for `ExtractInputsFromOutputs` for various PF schemas. `ExtractInputsFromOutputs` is a shared function so we should have coverage on both sides to make sure we don't cause regressions when fixing one. Similar to #2224 but on the PF side. This is in preparation for #2181 for #2180. Seeing some more evidence of #2218 here.
|
This PR has been shipped in release v3.88.0. |
Fixes an issue with the SDKv2 and PF input extraction. During import we wrongly return values for properties which are fully computed which causes invalid programs to be generated.
The object check inCastToTypeObjectwas wrong for SDKV2 objects:pulumi-terraform-bridge/pkg/tfshim/util/types.go
Line 36 in 48fd41b
The
CastToTypeObjectobject check is wrong for both SDKv2 and PF. For the SDKV2 only sets and lists can have aResource.Elem. For PF it list-nested blocks are represented with aResource.Elemin the shim layer, so the check did not catch these. PF details here: #2231This fixes the check and adds regression tests for the broken imports which resulted from 2180.
Test coverage added in #2224 for sdkv2 and #2225 for pf.
fixes #2180
depends on pulumi/providertest#91
stacked on #2225