Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify PF attr obj shim #2232

Draft
wants to merge 3 commits into
base: vvm/add_pf_shim_tests1
Choose a base branch
from
Draft

Simplify PF attr obj shim #2232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3a3d49c
WIP
VenelinMartinov Jul 23, 2024
3e4d5d7
remove todo, re-record shim tests
VenelinMartinov Jul 23, 2024
e316c27
fix test
VenelinMartinov Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion pf/internal/schemashim/attr_schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ func (*attrSchema) StateFunc() shim.SchemaStateFunc {

// Needs to return a shim.Schema, a shim.Resource, or nil.
func (s *attrSchema) Elem() interface{} {
asObjectType := func(typ any) (shim.Resource, bool) {
if tt, ok := typ.(basetypes.ObjectTypable); ok {
var res shim.Resource = newObjectPseudoResource(tt,
s.attr.Nested(),
nil)
return res, true
}
return nil, false
}
switch t := s.attr.GetType().(type) {
// The ObjectType can be triggered through tfsdk.SingleNestedAttributes. Logically it defines an attribute with
// a type that is an Object type. To encode the schema of the Object type in a way the shim layer understands,
Expand All @@ -96,7 +105,11 @@ func (s *attrSchema) Elem() interface{} {
var res shim.Resource = newTuplePseudoResource(t)
return res
case pfattr.TypeWithElementType:
return shim.Schema(newTypeSchema(t.ElementType(), s.attr.Nested()))
if r, ok := asObjectType(t.ElementType()); ok {
return r
} else {
return shim.Schema(newTypeSchema(t.ElementType(), s.attr.Nested()))
}

// t does not support any kind of element type.
default:
Expand Down
6 changes: 3 additions & 3 deletions pf/internal/schemashim/custom_type_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
"github.com/stretchr/testify/assert"

"github.com/pulumi/pulumi-terraform-bridge/pf/internal/pfutils"
"github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
shim "github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfshim"
)

// This example is taken from aws:resourceexplorer/index:Index "timeouts" property.
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assert on next line interestingly said it expects Resource, but the cast is casting to shim.Schema.

assert.Truef(t, isPseudoResource, "expected shim.Elem() to be of type shim.Resource, encoding an object type")

create := shimmed.Elem().(shim.Schema).Elem().(shim.Resource).Schema().Get("filter_string")
create := shimmed.Elem().(shim.Resource).Schema().Get("filter_string")
assert.Equal(t, shim.TypeString, create.Type())
}

Expand Down
43 changes: 14 additions & 29 deletions pf/tests/schemashim_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,15 +152,10 @@ func TestSchemaShimRepresentations(t *testing.T) {
"_": {
"list_attribute": {
"element": {
"schema": {
"element": {
"resource": {
"a1": {
"type": 4
}
}
},
"type": 6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the sdkv2 list nested attributes are also represented the same as list nested blocks: #2242

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"resource": {
"a1": {
"type": 4
}
}
},
"optional": true,
Expand Down Expand Up @@ -197,16 +192,11 @@ func TestSchemaShimRepresentations(t *testing.T) {
"_": {
"list_nested_attribute": {
"element": {
"schema": {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly a "single-nested-block-ish" encoding layer is stripped here.

"element": {
"resource": {
"a1": {
"optional": true,
"type": 4
}
}
},
"type": 6
"resource": {
"a1": {
"optional": true,
"type": 4
}
}
},
"optional": true,
Expand Down Expand Up @@ -240,16 +230,11 @@ func TestSchemaShimRepresentations(t *testing.T) {
"_": {
"map_nested_attribute": {
"element": {
"schema": {
"element": {
"resource": {
"a1": {
"optional": true,
"type": 4
}
}
},
"type": 6
"resource": {
"a1": {
"optional": true,
"type": 4
}
}
},
"type": 6
Expand Down