Reduce recursive types

[iwahbe]

I'm adding some custom schema transforms to reduce recursive types, similar to what we did for wafv2 in AWS.

Fixes #561

On master, running go build ./... on a Pulumi program that only instantiates a datadog.Provder consumes 10.8GB (10848649216).

On 32a0758, running go build ./... on the same Pulumi program consumes 5.5GB (5565906944).

[iwahbe]

I'm going to copy some of the schema primitives developed here into the bridge after this PR merges. They are useful stopgaps for pulumi/pulumi-terraform-bridge#1468.

[VenelinMartinov]

Great change! Github is struggling with the diff here. There's a commented out line in resources.go:106 - is that a type which is missed?

[iwahbe]

Great change! Github is struggling with the diff here. There's a commented out line in resources.go:106 - is that a type which is missed?

On the latest commit (e0ccb5b), L106 is:

pulumi-datadog/provider/resources.go

Line 106 in e0ccb5b

Fields: map[string]*tfbridge.SchemaInfo{

I don't see any extra comments in resources.go.

[VenelinMartinov]

My bad, I meant dashboard.go: https://github.com/pulumi/pulumi-datadog/pull/570/files#diff-2a792a5637ad41f6d67c809f2eef654a402244b5c9081abd7e28950eed5f8f5dR106

[iwahbe]

My bad, I meant dashboard.go: https://github.com/pulumi/pulumi-datadog/pull/570/files#diff-2a792a5637ad41f6d67c809f2eef654a402244b5c9081abd7e28950eed5f8f5dR106

Good catch. That should be fixed now.

[blampe]

dashboard.go seems complex enough to warrant some unit tests. The final result is so different that it's hard to know (or trust) that this is doing the Right Thing.

Do we need to include some type aliases for the things this removes?

index 96d00715..5c119c70 100644
--- a/provider/cmd/pulumi-resource-datadog/schema.json
+++ b/provider/cmd/pulumi-resource-datadog/schema.json
@@ -22,7 +22,6 @@
                 "azure": "Azure",
                 "datadog": "Datadog",
                 "gcp": "Gcp",
-                "index": "index",
                 "opsgenie": "OpsGenie",
                 "pagerduty": "PagerDuty",
                 "slack": "Slack"

Is this expected?

[iwahbe]

dashboard.go seems complex enough to warrant some unit tests. The final result is so different that it's hard to know (or trust) that this is doing the Right Thing.

I'll look at adding an integration test that works with the new types.

Do we need to include some type aliases for the things this removes?

This PR only effects types, not resources. We don't have an alias mechanism for types.

index 96d00715..5c119c70 100644
--- a/provider/cmd/pulumi-resource-datadog/schema.json
+++ b/provider/cmd/pulumi-resource-datadog/schema.json
@@ -22,7 +22,6 @@
                 "azure": "Azure",
                 "datadog": "Datadog",
                 "gcp": "Gcp",
-                "index": "index",
                 "opsgenie": "OpsGenie",
                 "pagerduty": "PagerDuty",
                 "slack": "Slack"

Is this expected?

Yes. This change is incidental to the PR, but it is expected (and it is a no-op).

[guineveresaenger]

Is this something we should implement in the bridge via pulumi/pulumi-terraform-bridge#1468? This feels like something we'd want to fix in general. Please disregard if this is not feasible or practical.

[iwahbe]

Is this something we should implement in the bridge via pulumi/pulumi-terraform-bridge#1468? This feels like something we'd want to fix in general. Please disregard if this is not feasible or practical.

Yes it is, but pulumi/pulumi-terraform-bridge#1468 turned out to be a surprisingly hard problem. Like I said in #570 (comment), I'll move the tools developed here to the bridge after this PR merges (and we know the tools are good). We will need to leave detecting this kind of unrolled mutual recursion for a later time.

[blampe]

1.55M lines removed definitely deserves some kind of award.

The integration test is good assurance things still work. I was hoping we could unit test this to make inputs/outputs clearer, but I defer to your judgement if you think that's feasible.

I don't fully understand how this will behave for folks who already have some of these types in their state, but if this is backwards-compatible then go for it!

[iwahbe]

1.55M lines removed definitely deserves some kind of award.

The integration test is good assurance things still work. I was hoping we could unit test this to make inputs/outputs clearer, but I defer to your judgement if you think that's feasible.

After this merges I'll add unit tests to the schema helper functions and move them to the bridge. I'd like to get this out to users now, and the integration test gives me enough confidence to do that.

I don't fully understand how this will behave for folks who already have some of these types in their state, but if this is backwards-compatible then go for it!

Types don't go into state, only values. This change only effects the SDK, and is transparent to the runtime provider and the Pulumi engine. The change is breaking if you are using these types, but the fix is simply using the new names. You should not need state edits.

[t0yv0]

Have we had multiple eyes or attempts on pulumi/pulumi-terraform-bridge#1468 ? I'm not sure I understand what the holdup is, if bridge internals get in the way, writing a simplifying pass func Simplify(PackageSpec) PackageSpec should be fairly straightforward no? Identify if two types look like equivalent-ish recursive versions of each other (A~B), pick the type with the shortest token A, and rewrite all references to B to references to A.

[iwahbe]

The holdup was working with bridge internals. It would be much simpler to make a general func Simplify(*PackageSpec) error pass, which is what this PR is. Identifying is still hard, but much simpler at the schema level.