-
Notifications
You must be signed in to change notification settings - Fork 95
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.
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
internal/fwserver: Ensure Attribute and Block Plan Modification Returns Custom Value Type Implementations When Using Custom Type NestedAttributeObject/NestedBlockObject #823
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🎸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! One minor testing thing I noticed, otherwise looks good to me 🚀
basetypes.ObjectType{ | ||
AttrTypes: map[string]attr.Type{ | ||
"nested_attr": types.StringType, | ||
}, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I might be being dense, should this be c.ObjectValue.Type(ctx)
so it matches up with how the type was created?
Similarly with the other custom types below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps I've misunderstood, but I don't think we can use the return value of c.ObjectValue.Type(ctx)
as the type is attr.Type
and NestedObjectCustomType
expects the embedded type to be basetypes.ObjectType
:
func (c NestedObjectCustomValue) Type(ctx context.Context) attr.Type {
return NestedObjectCustomType{
c.ObjectValue.Type(ctx), // wrong type, expects `basetypes.ObjectType`, gets `attr.Type`
}
}
However, we could use a call to obtain the attributes for the embedded basetypes.ObjectType
, for instance:
func (c NestedObjectCustomValue) Type(ctx context.Context) attr.Type {
return NestedObjectCustomType{
basetypes.ObjectType{
AttrTypes: c.AttributeTypes(ctx),
},
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, you are correct -- I was just surprised by the hardcoded attribute names/types in there.
If this works:
func (c NestedObjectCustomValue) Type(ctx context.Context) attr.Type {
return NestedObjectCustomType{
basetypes.ObjectType{
AttrTypes: c.AttributeTypes(ctx),
},
}
}
I'd highly suggest it, so anyone can implement test schemas without hardcoded schema implementation details. 👍
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Closes: #767
Closes: #821
Reference: #768
This PR addresses diagnostic errors arising from using a
CustomType
associated with aNestedObject
within aListNestedAttribute
,ListNestedBlock
,MapNestedAttribute
,SetNestedAttribute
orSetNestedBlock
. The diagnostic error generated isInvalid ... Element Type
. Further details can be found in #821 and #767.Previously, before logic updates: