Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a fallback for detailed diff replace decisions to ensure detailed diff is presentation-only #2747

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Dec 16, 2024

This change adds a fallback for the detailed diff replace decision. This ensures that the detailed diff is presentation only.

If we fail to identify the reason for a replace in the detailed diff calculation we mark it against __meta, similar to what we did before:

replaces = append(replaces, "__meta")

If we incorrectly identify a non-existent replace we demote it to an update/create/delete.

This is flagged behind the Accurate Previews flag.

fixes #2674
fixes #2726

@VenelinMartinov VenelinMartinov changed the title Add a fallback for replace decisions to ensure detailed diff is presentation-only Add a fallback for detailed diff replace decisions to ensure detailed diff is presentation-only Dec 16, 2024
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 36cecf5 to e76e9a7 Compare December 16, 2024 11:37
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 83.72093% with 7 lines in your changes missing coverage. Please review.

Project coverage is 68.84%. Comparing base (505e6eb) to head (f3e38a9).

Files with missing lines Patch % Lines
pkg/pf/tfbridge/provider_diff.go 0.00% 4 Missing ⚠️
pkg/tfbridge/detailed_diff.go 91.89% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@                           Coverage Diff                           @@
##           vvm/pf_tests_disable_accurate_previews    #2747   +/-   ##
=======================================================================
  Coverage                                   68.83%   68.84%           
=======================================================================
  Files                                         302      302           
  Lines                                       38736    38775   +39     
=======================================================================
+ Hits                                        26663    26693   +30     
- Misses                                      10562    10569    +7     
- Partials                                     1511     1513    +2     

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

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a couple optional style nits, but it looks good.

pkg/tfbridge/detailed_diff.go Outdated Show resolved Hide resolved
pkg/tfbridge/detailed_diff.go Show resolved Hide resolved
pkg/tfbridge/detailed_diff.go Outdated Show resolved Hide resolved
Copy link
Member

@t0yv0 t0yv0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very important. Can you add a quick note on flagging, does it roll out right away or it's flagged with accurate previews? Thanks.

@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/disable_accurate_previews_for_pf_tests December 16, 2024 15:50
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 619c4b5 to c83b590 Compare December 16, 2024 15:50
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch 4 times, most recently from 9156d7f to 4a091b8 Compare December 16, 2024 16:19
Base automatically changed from vvm/disable_accurate_previews_for_pf_tests to master December 16, 2024 17:23
@VenelinMartinov VenelinMartinov enabled auto-merge (squash) December 16, 2024 17:25
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 4a091b8 to 8313f97 Compare December 16, 2024 17:53
@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/pf_tests_disable_accurate_previews December 17, 2024 09:29
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 8313f97 to f3e38a9 Compare December 17, 2024 09:29
@VenelinMartinov VenelinMartinov merged commit 1a62de2 into vvm/pf_tests_disable_accurate_previews Dec 17, 2024
1 check passed
@VenelinMartinov VenelinMartinov deleted the vvm/detailed_diff_replace_fallback branch December 17, 2024 09:29
@VenelinMartinov VenelinMartinov restored the vvm/detailed_diff_replace_fallback branch December 17, 2024 09:29
VenelinMartinov added a commit that referenced this pull request Dec 17, 2024
… diff is presentation-only (#2757)

This change adds a fallback for the detailed diff replace decision. This
ensures that the detailed diff is presentation only.

If we fail to identify the reason for a replace in the detailed diff
calculation we mark it against `__meta`, similar to what we did before:
https://github.com/pulumi/pulumi-terraform-bridge/blob/a952164c556a86f46dac2ac34e915143cfd7abd8/pkg/tfbridge/provider.go#L1262
If we incorrectly identify a non-existent replace we demote it to an
update/create/delete.

This is flagged behind the Accurate Previews flag.

I've stood up
#2747 again as it
got accidentally merged into another branch.

fixes #2674
fixes #2726
VenelinMartinov added a commit that referenced this pull request Dec 17, 2024
This re-enables the detailed diff replace tests for the SDKv2 bridge for
lists and sets. These were previously skipped because of
#2726 which is
fixed in #2747
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants