Add more shim layer tests for pf #2231

VenelinMartinov · 2024-07-23T09:13:58Z

Follow-up on #2187 with missing PF attribute/block types.

Looks like the shim layer does indeed lose information, unlike in sdkv2. Wondering how much of it is essential:

the distinction between block and attribute seems to be lost. Perhaps intentional but it is meaningful in PF (ex. blocks can't be Computed)
list-attribute-object-element and list-nested-attribute are the same. As far as I can tell, these are pretty much the same thing on the PF side, so fair.
object-attribute, single-nested-attribute and single-nested-block are identical in the shim layer.
These are also identical to the nested object in a map-nested-attribute, list-nested-attribute and - these are all roughly "Objects" so probably fair.
Interestingly these are all different from the nested object in a list-nested-block - my intuition is that these should be "the same" for the shim layer. Any ideas here?

VenelinMartinov · 2024-07-23T09:14:37Z

pf/tests/schemashim_test.go

+					},
+				}},
+			},
+			// TODO: Why is this different to list-nested-attribute? Should it be?


Is this expected? I wonder why the list-nested-attribute has 2 more layers of nesting in the shim layer.

Ah, this is probably elemSchemas

pulumi-terraform-bridge/pkg/tfbridge/schema.go

Line 268 in c9ce9b5

func elemSchemas(sch shim.Schema, ps *SchemaInfo) (shim.Schema, *SchemaInfo) {

Or not, elemSchemas is called after the shim schema is constructed.

Found it in

pulumi-terraform-bridge/pf/internal/schemashim/attr_schema.go

Line 85 in 18db7c5

func (s *attrSchema) Elem() interface{} {

vs blocks:

pulumi-terraform-bridge/pf/internal/schemashim/block_schema.go

Line 56 in 18db7c5

func (s *blockSchema) Elem() interface{} {

Trying to see what happens if attr shims match the block ones here: #2232

Explored further in #2232, seems plausible that these should actually match and the attribute one seems wrong here. Wondering how this affects schema.go

Did you reach a conclusion here?

codecov · 2024-07-23T09:22:01Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.66%. Comparing base (7ccdd03) to head (e46106b).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2231   +/-   ##
=======================================
  Coverage   60.66%   60.66%           
=======================================
  Files         356      356           
  Lines       46447    46447           
=======================================
  Hits        28176    28176           
  Misses      16711    16711           
  Partials     1560     1560

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VenelinMartinov · 2024-07-23T09:28:14Z

pf/tests/schemashim_test.go

+          }
+        },
+        "optional": true,
+        "type": 5


Note this is a list type - this means that the CastToTypeObject

pulumi-terraform-bridge/pkg/tfshim/util/types.go

Line 36 in 48fd41b

func CastToTypeObject(tfs shim.Schema) (shim.SchemaMap, bool) {

is wrong for both PF and SDKv2 - list types CAN have a resource elem in PF, like in SDKv2

This is the root cause of #2180 fixed in #2181

Is this out of date?

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 in `CastToTypeObject` was wrong for SDKV2 objects: https://github.com/pulumi/pulumi-terraform-bridge/blob/48fd41bd16e2f7f933bb9390ab7619d52faabb6d/pkg/tfshim/util/types.go#L36~ The `CastToTypeObject` object check is wrong for both SDKv2 and PF. For the SDKV2 only sets and lists can have a `Resource` `.Elem`. For PF it list-nested blocks are represented with a `Resource` `.Elem` in the shim layer, so the check did not catch these. PF details here: #2231 This 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

t0yv0 · 2024-08-08T15:34:39Z

I'm running a bit out of time to review this @VenelinMartinov this week but I think I can take some time next week to work through and get the gory details right here.

iwahbe

As long as the tests have comments explaining what is "correct" vs just locking in current behavior, I'm happy to merge.

iwahbe · 2024-08-08T17:00:03Z

pf/tests/schemashim_test.go

+          }
+        },
+        "optional": true,
+        "type": 5


Is this out of date?

iwahbe · 2024-08-08T17:00:06Z

pf/tests/schemashim_test.go

+					},
+				}},
+			},
+			// TODO: Why is this different to list-nested-attribute? Should it be?


Did you reach a conclusion here?

t0yv0 · 2024-08-09T22:15:10Z

As long as the tests have comments explaining what is "correct" vs just locking in current behavior, I'm happy to merge.

That's the rub, we need to do work to understand what's right, not to confuse the codebase any further.

iwahbe · 2024-09-10T11:12:43Z

@VenelinMartinov Do we have a path towards merging here?

VenelinMartinov · 2024-09-16T21:03:17Z

I believe we should merge this as is.

The shim layer is confusing and ambiguous and it is seems at least some of its behaviour is not intentional.

This PR does not fix that but it provides us with an easy to use reference for some of the shim layer behaviour. We should separately spend more time in cleaning up the shim layer, potentially explore #2232 and other ways of making the shim layer behaviour understandable.

VenelinMartinov · 2024-09-16T21:04:52Z

Moreover, I think anyone who changes the shim layer behaviour should do that very intentionally, so should be fine to re-record some tests which are noted to have questionable conclusions.

iwahbe · 2024-09-16T21:59:21Z

Moreover, I think anyone who changes the shim layer behaviour should do that very intentionally, so should be fine to re-record some tests which are noted to have questionable conclusions.

I agree. Let's merge.

t0yv0 · 2024-10-01T12:36:47Z

AS time permits, could we rebase on #2456 and have another peek at the interesting bits with the generated Pulumi Schema side-by-side? Could be a good learning experience. Thanks!

VenelinMartinov added 2 commits July 23, 2024 12:02

add more shim layer tests for pf

b816056

todo

e46106b

VenelinMartinov requested review from t0yv0, guineveresaenger and iwahbe July 23, 2024 09:14

VenelinMartinov commented Jul 23, 2024

View reviewed changes

VenelinMartinov mentioned this pull request Jul 23, 2024

Fix nested computed read #2181

Merged

VenelinMartinov requested a review from a team July 23, 2024 10:40

VenelinMartinov mentioned this pull request Jul 23, 2024

Cannot set .Elem on a singly nested object block error when upgrading bridge #2185

Closed

VenelinMartinov mentioned this pull request Jul 23, 2024

Incorrect assumptions about SDK-v2 TypeMap encoding #1828

Open

mjeffryes assigned VenelinMartinov Jul 26, 2024

iwahbe approved these changes Aug 8, 2024

View reviewed changes

mjeffryes added this to the 0.108 milestone Aug 16, 2024

mikhailshilkov removed this from the 0.108 milestone Aug 21, 2024

VenelinMartinov mentioned this pull request Oct 1, 2024

Add more shim representation tests #2456

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more shim layer tests for pf #2231

Add more shim layer tests for pf #2231

VenelinMartinov commented Jul 23, 2024

VenelinMartinov Jul 23, 2024

VenelinMartinov Jul 23, 2024

VenelinMartinov Jul 23, 2024

VenelinMartinov Jul 23, 2024

VenelinMartinov Jul 23, 2024 •

edited

Loading

iwahbe Aug 8, 2024

codecov bot commented Jul 23, 2024 •

edited

Loading

VenelinMartinov Jul 23, 2024

iwahbe Aug 8, 2024

t0yv0 commented Aug 8, 2024

iwahbe left a comment

iwahbe Aug 8, 2024

iwahbe Aug 8, 2024

t0yv0 commented Aug 9, 2024

iwahbe commented Sep 10, 2024

VenelinMartinov commented Sep 16, 2024

VenelinMartinov commented Sep 16, 2024

iwahbe commented Sep 16, 2024

t0yv0 commented Oct 1, 2024

Add more shim layer tests for pf #2231

Are you sure you want to change the base?

Add more shim layer tests for pf #2231

Conversation

VenelinMartinov commented Jul 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VenelinMartinov Jul 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Aug 8, 2024

iwahbe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 commented Aug 9, 2024

iwahbe commented Sep 10, 2024

VenelinMartinov commented Sep 16, 2024

VenelinMartinov commented Sep 16, 2024

iwahbe commented Sep 16, 2024

t0yv0 commented Oct 1, 2024

VenelinMartinov Jul 23, 2024 •

edited

Loading

codecov bot commented Jul 23, 2024 •

edited

Loading