Simplify PF attr obj shim #2232
Conversation
|
The failing tests even expects this: It says it asserts on type |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## vvm/add_pf_shim_tests1 #2232 +/- ##
=======================================================
Coverage 60.66% 60.66%
=======================================================
Files 356 356
Lines 46447 46459 +12
=======================================================
+ Hits 28176 28184 +8
- Misses 16711 16714 +3
- Partials 1560 1561 +1 ☔ View full report in Codecov by Sentry. |
|
I believe we must have been already handling both as both are present. I'll sit on this for a bit |
| } | ||
| } | ||
| }, | ||
| "type": 6 |
There was a problem hiding this comment.
type=6 is TypeMap I think. This looks like the test was expecting what Elem() describes as "single-nested block encoding" - "In this case s.Type() == TypeMap and s.Elem() is a Resource value."
IN the new encoding, this is encoded as TypeList with Elem()=Resource. Elem() doc comment describes that this is an encoding for List-nested block but with this change it also encodes a List attribute.
There was a problem hiding this comment.
It the old encoding it is List(Elem(Map(Elem(Resource))))
In the new one it is List(Elem(Resource))
Given that pulumi doesn't care about attribute vs block, we might want to encode list nested blocks and list nested objects the same. Both represent an object in a list as far as pulumi is concerned.
There was a problem hiding this comment.
Also in the old encoding list_nested_attribute and list with object elem are encoded the same. I'm not sure I understand why these would be different to list_nested_block in the shim layer.
There was a problem hiding this comment.
In the sdkv2 list nested attributes are also represented the same as list nested blocks: #2242
There was a problem hiding this comment.
Given that pulumi doesn't care about attribute vs block
Are we 100% sure. I was trying to remember if that's the case, can't find anything just now searching now. There is normalizeBlockCollections but it's scoped to sdkv2 "under" the Shim abstraction.
| @@ -197,16 +192,11 @@ func TestSchemaShimRepresentations(t *testing.T) { | |||
| "_": { | |||
| "list_nested_attribute": { | |||
| "element": { | |||
| "schema": { | |||
There was a problem hiding this comment.
Similarly a "single-nested-block-ish" encoding layer is stripped here.
| @@ -109,10 +109,10 @@ func TestCustomListAttribute(t *testing.T) { | |||
| shimmed := &attrSchema{"key", pfutils.FromAttrLike(raw)} | |||
| assert.Equal(t, shim.TypeList, shimmed.Type()) | |||
| assert.NotNil(t, shimmed.Elem()) | |||
| _, isPseudoResource := shimmed.Elem().(shim.Schema) | |||
| _, isPseudoResource := shimmed.Elem().(shim.Resource) | |||
There was a problem hiding this comment.
Assert on next line interestingly said it expects Resource, but the cast is casting to shim.Schema.
|
This may be a better encoding, but this is pretty error-prone, I wonder if we could complement this with some tests taking this for a spin end to end. Specifically, for tfgen layer and runtime handling:
|
|
Also sounds like an update on the Elem() doc comment is needed here for this case. |
|
Yeah, some end-to-end tests are certainly needed - thanks for the suggestions on what to focus on! I'll also revisit the Elem docs. |
|
I expect there is a big encoding bug here somewhere. Map-nested attributes https://developer.hashicorp.com/terraform/plugin/framework/handling-data/attributes/map-nested But they might clobber each other in the shim.Schema representation with Type=TypeMap, Elem()=Resource. Also in SDKv2 TypeMap, Elem()=Resource means a degenerate case of |
This is invalid in SDKv2 - only lists and sets can have an Elem Resource |
|
IT is admitted though, and valid, per doc in Elem() and https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/core_schema_test.go#L220 |
|
Wondering if any providers actually use this - I doubt it works well |
|
We have to make it an error though, or else to make the schema Map<string,string>, since it's a possibility. |
|
Yeah, absolutely, great point about Map nested Resource elems in SDKv2 |
|
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! |
We seem to represent list-nested objects in the shim layer differently depending on if it is an attr or block.
The attribute one seems overly complicated:
list: {element: schema: element: resource}vslist: element: resourcefor blocks.This is an attempt at simplifying it - not fully tested yet.