Remove experimental flag for SkipDetailedDiff

[guineveresaenger]

Removes the flag option XSkipDetailedDiffForChanges and rolls out behavior to all providers.

This change will break the next bridge release for pulumi-gcp and pulumi-aws. When updating those providers to these changes, the XSkipDetailedDiffForChanges provider info option must be removed.

Note that the maxItemsOne migration tests needed to be updated to show that a change of [] to "" reflects a diff on the field in question now. This makes sense to me, since a nil slice is different from an empty string, but this may actually not be desired behavior, so I'd appreciate scrutiny here. Either way, the functionality under test there requires a diff to happen on MaxItemsOne migrations and that does not change.

Fixes #1501.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.01%. Comparing base (94c1de4) to head (682a19a).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1893      +/-   ##
==========================================
- Coverage   60.02%   60.01%   -0.01%     
==========================================
  Files         327      327              
  Lines       43994    43994              
==========================================
- Hits        26407    26403       -4     
- Misses      16096    16099       +3     
- Partials     1491     1492       +1     

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

[t0yv0]

I'm nervous as this exposes the entire AWS to new behavior over a codebase where the root cause has not been identified or fix properly tested. This change promotes a workaround from a tactical fix to the canonical behavior. Why? I'd like to request to see some evidence that the underlying problem has been fixed since the time of filing of #1501 and now; have we backfilled coverage for makeDetailedDiff or factored it properly? Have we put this code under rapid-randomized tests or newly written table driven tests that increased coverage?

[t0yv0]

See comment above.

[t0yv0]

Ugh, I apologize, https://github.com/pulumi/pulumi-aws/blob/upstream-v5.46.0/provider/resources.go#L5872 this is already AWS behavior somehow so there can be no possible objection on AWS grounds. I'm still ambitious for more confident testing and refactoring here but that will have to wait until we have time to do it.

[guineveresaenger]

Thanks for taking a look, @t0yv0 - you are correct that this behavior has been in place for AWS for a while now. I was basing this change on this comment that was primarily addressing the plan distortion.

Yes, we need to make makeDetailedDiff better. That's not this change. I can look into how we can test the (non-detailed) diff better than it is today.

[iwahbe]

Given that this change is rolled out already to our 2 largest bridged providers, I'm confident that it is well tested.

It would be great to have more testing in the bridge, but I don't think that is necessary to merge.

[t0yv0]

We know that detailed diff is not working correctly. Not all users report issues they run into, but some do and there is a growing number of issues. If it's not working correctly the problem is more than just the current implementation is well tested; if it's passing tests they must not be the interesting tests. Debugging one more customer orange AWS bug that bottoms up at the detailed diff problems, I've correlated some thoughts on this in #1895 - that's afaik the best way forward from what I know of today, but admittedly it still leaves set diffing behavior lacking; if we can think of a better way to do that that'd be wonderful.

[t0yv0]

IN particular when merging this I'd not consider #1501 adequately or sustainably fixed by this change.

[guineveresaenger]

@t0yv0 - would it be alright with you to merge and follow the future work in #1895?

[t0yv0]

Totally no objections to merging - mea culpa I forgot the context that AWS already runs with this - so it's a no-op for AWS and I have no stake in the other providers anymore thanks! I'll just reopen #1501 if this merges.