Handle diags from gRPC state upgrader

[t0yv0]

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 pulumi/pulumi-aws#4015

[t0yv0]

This is the essential change. Do not ignore diagnostics, including errors, fail fast here.

[t0yv0]

The error message is not a great one.

It's losing attribute information from this String() method:

// ValidationError wraps validation errors reported by shims (currently shim v2 and tf5)
type ValidationError struct {
	AttributePath cty.Path
	Summary       string
	Detail        string
}

func (e ValidationError) Error() string {
	if e.Detail != "" {
		return fmt.Sprintf("%s: %s", e.Summary, e.Detail)
	}
	return e.Summary
}

We could have had compute_resources.$.ec2_configuration which is essential for debugging this.

[t0yv0]

The actual message is also terrible because the internals of tfprotov5.UpgradeResourceState handler use a raw json parser and expect types to match; no more type checking. This works out because in TF the data that's passed in is already type-conformant. Any quick ideas to improve here .

[codecov]

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 ⚠️

Additional details and impacted files

@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[VenelinMartinov]

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]

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]

I wonder if type-checking for the state values would be a good workaround for #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]

Maybe. IN theory with #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]

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]

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]

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