-
Notifications
You must be signed in to change notification settings - Fork 386
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
Make IterFields take in aliases #3207
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #3207 +/- ##
==========================================
+ Coverage 83.57% 83.85% +0.28%
==========================================
Files 168 170 +2
Lines 15021 15315 +294
==========================================
+ Hits 12554 12843 +289
+ Misses 1729 1721 -8
- Partials 738 751 +13
|
65a0442
to
8f65cbf
Compare
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.
Making good progress. Let's see if there is a way we can continue to use StructToSchema as the main entrypoint so that developers can easily switch to using this custom implementation without having to rewrite their calls to StructToSchema. This makes library code like WorkspaceData/AccountData more portable to the new model (and would immediately solve problems like #3211 is trying to solve)
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.
Couple more comments.
clusters/resource_cluster.go
Outdated
|
||
func (ClusterResourceProvider) UnderlyingType() compute.ClusterSpec { | ||
return compute.ClusterSpec{} | ||
type ClusterResourceProvider struct { |
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.
Let's just call this Cluster
, in the same way that we have SqlEndpoint
.
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.
I think Cluster
is already defined, so how about naming it ClusterSpec
?
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.
Either is better than ClusterResourceProvider
:)
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!
Changes
ResourceProviderStruct
take effect initerFields
function so thatStructToData
andDataToStructPointer
will take information about the aliasesUnderlyingType
method because it is no longer neededClusterResourceProvider
struct so that reflection will work withoutUnderlyingType
iterField
check alias informationTests
make test
run locallydocs/
folderinternal/acceptance