chore: Cleanup non-custom type usage in autogen #3834
Conversation
There was a problem hiding this comment.
Moved common functions used by custom types here.
| "github.com/hashicorp/terraform-plugin-framework/types/basetypes" | ||
| ) | ||
|
|
||
| func getValueElementType[T attr.Value](ctx context.Context) attr.Type { |
There was a problem hiding this comment.
qq: what is the difference between getValueElementType and getElementType attr.Type return, can you give an example?
There was a problem hiding this comment.
Types that handle TF values (list, map, set) use getValueElementType (T is attr.Value).
Types that handle nested structs (object, nested_list, nested_map, nested_set) use getElementType (T is any), which requires traversal of the struct via reflection to generate the resulting type.
There was a problem hiding this comment.
thanks, what about adding code comments with this or making the func names more explicit, e.g. getCollectionType and getNestedType
There was a problem hiding this comment.
Missed this one. Yes, I like that more. Changing in the followup PR.
| fmt.Sprintf(`%T has usupported type: %s`, value.Interface(), valueType), | ||
| )} | ||
| } | ||
|
|
There was a problem hiding this comment.
nit: my preference is not to have so many empty lines
There was a problem hiding this comment.
👌Removing empty line at 30 to follow the pattern. 0baa18b
| switch v := value.(type) { | ||
| case customtypes.ObjectValueInterface: | ||
| return resolveObjectAttr(ctx, v) | ||
| case customtypes.NestedListValueInterface: | ||
| return resolveNestedListAttr(ctx, v) | ||
| case customtypes.NestedMapValueInterface: | ||
| return resolveNestedMapAttr(ctx, v) | ||
| } |
There was a problem hiding this comment.
q: Did we missing handling NestSet type here?
There was a problem hiding this comment.
No need to handle here since unmarshal does not generate unknowns (we do not merge sets with existing state).
| withObjType: true, | ||
| goldenFileName: "primitive-attributes", | ||
| }, | ||
| "Nested attributes": { |
There was a problem hiding this comment.
nit: Given custom type is how we now manage all nested attributes, would consider renaming the existing test Custom type attributes to Nested attributes and just ensuring we are capturing the existing nested attributes scenarios.
There was a problem hiding this comment.
Left the custom in the test name to avoid confusion since it covers both the "nested" and non "nested" custom types.
The existing scenarios are covered 👌
Merging to unblock next PR, if you insist on a different name I can change it in the next.
| } | ||
|
|
||
| type SchemaOptions struct { | ||
| UseCustomNestedTypes *bool `yaml:"use_custom_nested_types"` // Tmp flag to disable custom nested types usage until typing is supported for all nested attributes. Defaults to true. - CLOUDP-352973 |
There was a problem hiding this comment.
just to double check that we don't envision that we'll need this flag in some edge cases for new resources until we fix some unexpected issues
There was a problem hiding this comment.
We are removing all non-custom code so there is nothing to fall back to :D.
I am confident enough after getting complex resources like cluster_api & search_index_api working with custom types.
manupedrozo
deleted the
CLOUDP-352973-autogen-non-custom-types-cleanup
branch
Description
Removing usage of non-custom
object,list,set&maptypes in autogen code now that we have custom versions for all of them.use_custom_nested_types, all resources already use custom types.I'll follow up with a PR to remove object types generation code (the
withObjTypesflag is always set to false anyways).As agreed offline, object types will not be needed since we took the custom types + reflection approach for conversion between TF schema and API json.
Link to any related issue(s): CLOUDP-352973
Type of change:
Required Checklist:
Further comments