pulumi / pulumi-terraform-bridge

A library allowing providers built with the Terraform Plugin SDK to be bridged into Pulumi.
Apache License 2.0
183 stars 41 forks source link

Handle diags from gRPC state upgrader #2053

Closed t0yv0 closed 3 weeks ago

t0yv0 commented 3 weeks ago

Isolating this from an AWS test failure; handling diags turns a panic into a regular error. Looks like the error message is still unfortunate. The test is isolated from https://github.com/pulumi/pulumi-aws/issues/4015

codecov[bot] commented 3 weeks ago

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 61.44%. Comparing base (4503342) to head (fd63dca). Report is 1 commits behind head on master.

Files Patch % Lines
pkg/tfshim/sdk-v2/provider2.go 20.00% 0 Missing and 4 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2053 +/- ## ========================================== + Coverage 61.37% 61.44% +0.07% ========================================== Files 334 334 Lines 44947 44951 +4 ========================================== + Hits 27585 27621 +36 + Misses 15836 15805 -31 + Partials 1526 1525 -1 ```

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

VenelinMartinov commented 3 weeks ago

Yeah I've seen the odd errors with the json parsing but I was under the impression that these only happen on type mismatches, which should be handled by the new type checking.

Otherwise the changes here looks sensible.

t0yv0 commented 3 weeks ago

State parsing is not handled by type-checking and it's an odd place to put it since the user has more direct control of resource config than state. It's an interesting thought though perhaps we could. At least when mismatches of this sort happen it could provide better errors. Unfortunately the existing type-checking is not reusable at this level since it's written against pulumi types and values not TF-level types and values.

VenelinMartinov commented 3 weeks ago

I wonder if type-checking for the state values would be a good workaround for https://github.com/pulumi/pulumi-terraform-bridge/issues/1667 - it'd alert the user about a possible issue much sooner.

Users can then act on that, do manual state edits, etc - still not a full fix but a better experience than what we have now.

t0yv0 commented 3 weeks ago

Maybe. IN theory with https://github.com/pulumi/pulumi-terraform-bridge/issues/1667 this situation should be guaranteed not to arise with new code but in practice it might. I wonder if this is tantamount to just not recovering cty.Value failures with approximate methods, the strict methods will type check and fail fast with some information

t0yv0 commented 3 weeks ago

I think maybe let's check this in, I'll follow up with some quick double-check to figure out if the error message can be better here.

VenelinMartinov commented 3 weeks ago

I think the current strategy is to install a panic handler if we get any type errors - so we still allow a recovery but if we panic during the recovery we catch it and print the helpful error.

t0yv0 commented 3 weeks ago

Yeah no, the error currently printed is anything but helpful.