Separate diff decision from presentation

[VenelinMartinov]

Adds a DiffEqualDecisionOverride to the shim layer which allows provider implementations to specify a diff decision instead of relying on the shim layer to do that.

Also adds a DiffEqualDecisionOverride to the PRC sdkv2 implementation which opentofu does.

Also adds a feature flag EnableAccurateBridgePreview which guards this feature.

will fix once rolled out: #2293
will fix once rolled out: #1501
will undo once rolled out: #1502 as the underlying issue was fixed during the PRC work.
Stacked on #2380

[codecov]

Codecov Report

Attention: Patch coverage is 60.34483% with 23 lines in your changes missing coverage. Please review.

Project coverage is 57.41%. Comparing base (9668009) to head (ab1262f).
Report is 5 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/tfbridge/provider.go	44.44%	13 Missing and 2 partials ⚠️
pkg/tfshim/sdk-v1/instance_diff.go	0.00%	2 Missing ⚠️
pkg/tfshim/sdk-v2/instance_diff.go	0.00%	2 Missing ⚠️
pkg/tfshim/sdk-v2/provider2.go	91.30%	2 Missing ⚠️
pkg/tfshim/tfplugin5/instance_diff.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2379      +/-   ##
==========================================
- Coverage   57.44%   57.41%   -0.03%     
==========================================
  Files         369      369              
  Lines       50189    50230      +41     
==========================================
+ Hits        28831    28841      +10     
- Misses      19779    19809      +30     
- Partials     1579     1580       +1     

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

[t0yv0]

I've added some nits on the code style but also a couple of higher level questions:

1. Are corner cases here where we introduced original hacks already covered well by tests, can you point out which ones, I'd like to give it a read to double-check.

2. Feels like a relatively big deal, originally we talked about rolling this under the "accurate previews" flag, but this PR suggests you're thinking maybe including it in a regular bridge update. Perhaps we have enough confidence to do that, just was making sure it's an informed decision.

3. Out of scope for this PR, but I wonder if you've noticed collectionDiffs feature; that seems to be still hanging around but I was hoping we can eventually remove this (maybe not yet in this PR), WDYT? It was about explaining these "mysterious diffs" which we computed incorrectly. Perhaps we can file a reminder to clean it out in the Accurate Previews epic.

	if !diff.HasNoChanges() {
		changes = pulumirpc.DiffResponse_DIFF_SOME
		// Perhaps collectionDiffs can shed some light and locate the changes to the end-user.
		for path, diff := range dd.collectionDiffs {
			detailedDiff[path] = diff
		}
	}

[VenelinMartinov]

Hm, looking at this again, perhaps we can not merge this safely: This changes the Diff/NoDiff decision from being derived from the detailedDiff.

I thought this only affected the workaround added in #1502 but it does have bigger implications - might be best to hold off here and merge as part of the bigger changes.

[VenelinMartinov]

On your questions:

Are corner cases here where we introduced original hacks already covered well by tests, can you point out which ones, I'd like to give it a read to double-check.
Did some delving into the git history and here is my understanding:

• Use TF Diff Attributes to set diff response changes when SkipDetailedDiffForChanges feature flag is set #1502 was added due to issues in GCP and AWS
• The GCP issue is related to labels and covered by the labels test in https://github.com/pulumi/pulumi-gcp/pull/1310/files#diff-2e4fffe32720b3d310b4336cfdde6519ab1bf6d9e0d82b418299817fdf42a29fR191
• The AWS issue is related to empty tags: Fixes empty tag handling pulumi-aws#2944 - also covered by tests in AWS
• Both the providers' test passed with this change enabled. That makes me believe that this issue is fixed and the workaround is no longer needed.

Feels like a relatively big deal, originally we talked about rolling this under the "accurate previews" flag, but this PR suggests you're thinking maybe including it in a regular bridge update. Perhaps we have enough confidence to do that, just was making sure it's an informed decision.

Yeah, upon revisiting, this might not be safe to merge without a feature flag, will hold off.

Out of scope for this PR, but I wonder if you've noticed collectionDiffs feature; that seems to be still hanging around but I was hoping we can eventually remove this (maybe not yet in this PR), WDYT? It was about explaining these "mysterious diffs" which we computed incorrectly. Perhaps we can file a reminder to clean it out in the Accurate Previews epic.

Yeah, noticed it but it is out of scope for this PR - here I only wanted to separate the Diff/NoDiff decision from the detailed diff - the detailed diff issues should be addressed as part of #2294

[VenelinMartinov]

I've added a feature flag for this, along with an env var for it.

I'll set up CI to run with the feature flag so that we do not repeat mistakes from the PRC rollout

[t0yv0]

I'll set up CI to run with the feature flag so that we do not repeat mistakes from the PRC rollout

Excellent, I was just jotting down a ticket to make tests go in a matrix fashion against the flag state space. Thank you.

[t0yv0]

Thank you! Very important piece for us.

[pulumi-bot]

This PR has been shipped in release v3.91.1.