pulumi / pulumi-terraform-bridge

A library allowing Terraform providers to be bridged into Pulumi.
Apache License 2.0
199 stars 43 forks source link

Fix PF provider_server detailed diff handling #2628

Closed VenelinMartinov closed 17 hours ago

VenelinMartinov commented 1 week ago

This change fixes an issue with the provider_server implementation's detailed diff handling. Previously passing a detailed diff to it would result in the previews being deleted. This is tested as part of https://github.com/pulumi/pulumi-terraform-bridge/pull/2629

The problem was that we were re-calculating the diffs and replaces keys for the GRPC Diff protocol in the provider_server implementation but also doing that incorrectly. Instead this change now makes provider_server's marshalDiff just pass through the diffs and replaces which we have already calculated in https://github.com/pulumi/pulumi-terraform-bridge/blob/1d6b032f3e376af4667c6c4d80a65eff072df807/pkg/pf/tfbridge/provider_diff.go#L108-L109

fixes https://github.com/pulumi/pulumi-terraform-bridge/issues/2620

VenelinMartinov commented 1 week ago

This change is part of the following stack:

Change managed by git-spice.

codecov[bot] commented 1 week ago

Codecov Report

Attention: Patch coverage is 61.76471% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.39%. Comparing base (bc33162) to head (12bb0f7). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/pf/internal/plugin/provider_server.go 61.76% 13 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2628 +/- ## ========================================== + Coverage 69.35% 69.39% +0.03% ========================================== Files 301 301 Lines 38558 38560 +2 ========================================== + Hits 26743 26759 +16 + Misses 10295 10281 -14 Partials 1520 1520 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.


🚨 Try these New Features:

VenelinMartinov commented 3 days ago

We already have changes to our copy, so resolving the divergence is out of scope here. Getting rid of our copy is blocked on the config encoding work. The upstream copy does not seem to work correctly for the detailed diff either but is only used in tests.

EDIT: Added some unit tests. The tests in https://github.com/pulumi/pulumi-terraform-bridge/pull/2629 should add quite broad coverage here. I have separated this into a different PR to make reviewing easier.

t0yv0 commented 3 days ago

Very surprised but this seems to hold?

https://github.com/search?q=org%3Apulumi%20plugin.NewProviderServer&type=code

Looks like these are mostly non-production uses.

t0yv0 commented 3 days ago

Can you point out which line had the bug you're fixing? Very non-obvious.

t0yv0 commented 3 days ago

I'm inclined to get this in given your finding that this Core code is not in prod anywhere. We need to maintain a working copy then. Oof. Please address the enum cast concern the rest are nits.