-
Notifications
You must be signed in to change notification settings - Fork 99
list: add primitive, list and map types #1177
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
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.
Renamed this to follow the naming convention in the directory
austinvalle
left a comment
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, left some nit comments as well as a comment about adding NumberAttribute, but that can all be done in future PRs if you prefer 🚀
In terms of tests I don't think there are any missing from here, there may be some more "integration" like tests in packages like fwserver and such for GetProviderSchema, but I think those are kind of overkill anyways 😅 . The testing in this PR seems reasonable; if we want to add more testing, we could consider writing tests over in terraform-provider-corner later 👍🏻
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.
Unrelated to this PR, as we can always come back later and add more types
Is it written down anywhere the full list of attribute types we'll end up supporting for list configs? I couldn't find it in any of the RFCs 👀
Since the configuration syntax is in a nested block, we should be able to support all of the nested types as well (nested attributes, nested blocks), unless we are explicitly choosing not to support them.
If we don't support either nested types, we might want to consider also supporting single object types via an ObjectAttribute.
I'd also be interested if we are going to support DynamicAttribute / DynamicPseudoType, which is also kind of a "primitive" attribute depending on how you view it 😆
And related to collections, the set attributes as well
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.
It seems it was agreed to limit support to lists and maps of primitives in an internal discussion. DynamicAttribute sounds like a helpful thing to have here though so I'll add that in a follow up PR.
| // Use ListNestedAttribute if the underlying elements should be objects and | ||
| // require definition beyond type information. |
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.
nit: Depending on if we are supporting nested attribute types, this might need to be removed 👀
Same comment for MapAttribute and it's pkg docs
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.
Will address this comment in a follow up PR adding DynamicAttribute
Description
Adds the following types to list schema
I suspect some additional tests are missing somewhere else as well...
Rollback Plan
Changes to Security Controls
None