-
Notifications
You must be signed in to change notification settings - Fork 44
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
OmitType and TypeName to support sharing types #2409
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2409 +/- ##
=======================================
Coverage 57.66% 57.66%
=======================================
Files 369 369
Lines 50121 50148 +27
=======================================
+ Hits 28902 28920 +18
- Misses 19641 19650 +9
Partials 1578 1578 ☔ View full report in Codecov by Sentry. |
This does not check that the underlying type is structurally equal to the new replacement type. We could add this check. |
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.
This is a very surgical change, which is great. I would mark the new fields on info.Schema
as experimental in their respective doc comments, just to hedge against future iteration on the design.
pkg/tfgen/generate.go
Outdated
switch { | ||
case t.typePrefixOverride != nil && other.typePrefixOverride == nil: | ||
return false | ||
case t.typePrefixOverride == nil && other.typePrefixOverride != nil: | ||
return false | ||
case t.typePrefixOverride != nil && other.typePrefixOverride != nil && | ||
*t.typePrefixOverride != *other.typePrefixOverride: | ||
return false | ||
} |
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:
I think this is clearer as an if
statement. It looks odd to me to have 3 branches that all lead to the same body.
if (t.typePrefixOverride != nil && other.typePrefixOverride == nil) ||
(t.typePrefixOverride == nil && other.typePrefixOverride != nil) ||
(t.typePrefixOverride != nil && other.typePrefixOverride != nil &&
*t.typePrefixOverride != *other.typePrefixOverride) {
return false
}
That said, this is just an inequality check made painful by nil values. Nitpicking, I would express this as the inverse of an equality check:
switch { | |
case t.typePrefixOverride != nil && other.typePrefixOverride == nil: | |
return false | |
case t.typePrefixOverride == nil && other.typePrefixOverride != nil: | |
return false | |
case t.typePrefixOverride != nil && other.typePrefixOverride != nil && | |
*t.typePrefixOverride != *other.typePrefixOverride: | |
return false | |
} | |
if eq := t.typePrefixOverride == other.typePrefixOverride || | |
(t.typePrefixOverride != nil && other.typePrefixOverride != nil && | |
*t.typePrefixOverride == *other.typePrefixOverride); !eq { | |
return false | |
} |
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.
This is an F# habit that may be out of place but I find individual cases more tractable than a large bool expr.
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.
Very nice! You mentioned tools to help provider maintainers spot these repeated types in the schemas. Is the plan to add these to upstream upgrades or tfgen or something else?
I've spent a lot of time last night getting Quicksight sharing rolled out to AWS against this functionality and found it can be made to work but it is quite awkward. This PR reduces Quicksight from 30,000 to 7,000 types - there is still some work to specify sharing. Couple of learnings there:
In AWS I built a stateful facade that emulates this by emitting
Perhaps we can automate the following flow.
A question is what to do with dynamically bridged providers. It might be desirable there, since they are subject to type explosion as well. I think once we are certain of this functionality, we should default to detecting sharing in dynamically bridged providers, picking arbitrary names for the shared types. |
3920161
to
11d4115
Compare
Tweaked the API to make a lot more usable for QuickSight use case. PTAL. |
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
Introduce OmitType and TypeName flags to enforce type sharing. Some of the upstream providers generate very large concrete schemata. TF is not being materially affected, just high RAM demands for in-memory processing. The example is inspired by QuickSight types in AWS. Pulumi is affected significantly. In Pulumi the default projection is going to generate named types for every instance of the shared schema. This leads to SDK bloat and issues with "filename too long." With this change it is possible for the provider maintainer opt into explicit sharing of types, and ensure that the type names for the shared types have shorter meaningful prefixes. At definition type the user can specify the type name to generate, which can be very short, and replace the automatically implied ReallyLongPrefixedTypeName like this: ```go "visuals": { Elem: &info.Schema{ TypeName: tfbridge.Ref("Visual"), }, }, ``` At reference time in another resource, the user can reuse an already generated type by token. This already worked before this change but had the downside of still generating unused helper types and causing SDK bloat. ```go "visuals": { Elem: &info.Schema{ Type: "testprov:index/Visual:Visual", }, }, ``` With this change it is possible to instruct the bridge to stop generating the unused helper types: ```go "visuals": { Elem: &info.Schema{ Type: "testprov:index/Visual:Visual", OmitType: true }, }, ```
This PR has been shipped in release v3.91.0. |
Introduce OmitType and TypeName flags to enforce type sharing.
Some of the upstream providers generate very large concrete schemata. TF is not being materially affected, just high RAM demands for in-memory processing. The example is inspired by QuickSight types in AWS. Pulumi is affected significantly. In Pulumi the default projection is going to generate named types for every instance of the shared schema. This leads to SDK bloat and issues with "filename too long."
With this change it is possible for the provider maintainer opt into explicit sharing of types, and ensure that the type names for the shared types have shorter meaningful prefixes.
At definition type the user can specify the type name to generate, which can be very short, and replace the automatically implied ReallyLongPrefixedTypeName like this:
At reference time in another resource, the user can reuse an already generated type by token. This already worked before this change but had the downside of still generating unused helper types and causing SDK bloat.
With this change it is possible to instruct the bridge to stop generating the unused helper types: