-
Notifications
You must be signed in to change notification settings - Fork 400
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
Update Aliases function for ResourceProvider to handle recursive structures more gracefully #3338
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.
Seems reasonable, small suggestions but we're almost there.
common/reflect_resource.go
Outdated
@@ -41,7 +41,7 @@ var kindMap = map[reflect.Kind]string{ | |||
// Generic interface for ResourceProvider. Using CustomizeSchema and Aliases functions to keep track of additional information | |||
// on top of the generated go-sdk struct. This is used to replace manually maintained structs with `tf` tags. | |||
type ResourceProvider interface { | |||
Aliases() map[string]string | |||
Aliases() map[string]map[string]string |
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.
Can you add the doccomment here? What are the keys and values?
By the way, we may want to make Aliases
optional for ResourceProvider. If a resource has no aliases, we shouldn't be forced to add an empty Aliases function, right?
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.
Added comments with an example.
For making Aliases
optional, I played around with the idea of adding another interface without Aliases()
but I think it's a bit more complicated since we also have RecursiveResourceProvider
. I think I'm probably going to leave it as it is and try to resolve this later on. For now I think let's just merge this to unblock the next steps for the jobs migration
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.
Discussed offline. We'll do this.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3338 +/- ##
==========================================
- Coverage 83.47% 83.47% -0.01%
==========================================
Files 175 176 +1
Lines 16095 16166 +71
==========================================
+ Hits 13436 13494 +58
- Misses 1846 1854 +8
- Partials 813 818 +5
|
common/reflect_resource.go
Outdated
@@ -41,7 +41,7 @@ var kindMap = map[reflect.Kind]string{ | |||
// Generic interface for ResourceProvider. Using CustomizeSchema and Aliases functions to keep track of additional information | |||
// on top of the generated go-sdk struct. This is used to replace manually maintained structs with `tf` tags. | |||
type ResourceProvider interface { | |||
Aliases() map[string]string | |||
Aliases() map[string]map[string]string |
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.
Discussed offline. We'll do this.
Co-authored-by: Miles Yucht <miles@databricks.com>
Changes
Aliases
function return a map of map which can derive aliases from the parent type name and field name.recursion_tracking.go
functions to makegetNameForType
more sharableTests
make test
run locallydocs/
folderinternal/acceptance