search

pulumi / pulumi-datadog

An Datadog Pulumi resource package, providing multi-language access to Datadog
Apache License 2.0
18 stars 6 forks source link

Reduce recursive types #570

Closed iwahbe closed 1 week ago

iwahbe commented 3 weeks ago

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

Fixes https://github.com/pulumi/pulumi-datadog/issues/561

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

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

github-actions[bot] commented 3 weeks ago

Does the PR have any schema changes?

Found 1699 breaking changes:

Types

Maintainer note: consult the runbook for dealing with any breaking changes.

iwahbe commented 3 weeks ago

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

VenelinMartinov commented 2 weeks ago

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 commented 2 weeks ago

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 (e0ccb5bfa10968b3ebda31a4b8ba8fd23b078b4b), L106 is:

https://github.com/pulumi/pulumi-datadog/blob/e0ccb5bfa10968b3ebda31a4b8ba8fd23b078b4b/provider/resources.go#L106

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

VenelinMartinov commented 2 weeks ago

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

iwahbe commented 2 weeks ago

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

Good catch. That should be fixed now.

iwahbe commented 2 weeks ago

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).

iwahbe commented 2 weeks ago

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 https://github.com/pulumi/pulumi-datadog/pull/570#issuecomment-2168954440, 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.

iwahbe commented 2 weeks ago

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 commented 1 week ago

Have we had multiple eyes or attempts on https://github.com/pulumi/pulumi-terraform-bridge/issues/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 commented 1 week ago

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.