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

SDKv2 Only return a detailed diff if a diff is detected #2750

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Dec 16, 2024

Only return a detailed diff if a diff is detected. This should prevent any mismatches between the detailed diff and the diff decision. Mismatches could only happen in case of a bug somewhere and that might cause issues with the engine.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Dec 16, 2024

@VenelinMartinov VenelinMartinov changed the title Only return a detaied diff if a diff is detected SDKv2 Only return a detailed diff if a diff is detected Dec 16, 2024
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 36cecf5 to e76e9a7 Compare December 16, 2024 11:37
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from f86989b to 995278d Compare December 16, 2024 11:37
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

Attention: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.85%. Comparing base (8313f97) to head (d31fb69).

Files with missing lines Patch % Lines
pkg/tfbridge/provider.go 50.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@                          Coverage Diff                           @@
##           vvm/detailed_diff_replace_fallback    #2750      +/-   ##
======================================================================
- Coverage                               68.86%   68.85%   -0.02%     
======================================================================
  Files                                     301      301              
  Lines                                   38765    38765              
======================================================================
- Hits                                    26696    26690       -6     
- Misses                                  10557    10561       +4     
- Partials                                 1512     1514       +2     

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

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.

Probably reasonable, but wondering if this behavior is under-tested since there's no changes to tests? Can we add a quick one?

@t0yv0 t0yv0 self-requested a review December 16, 2024 15:25
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.

I'm going to assume the rest of the PR stack provides tests for this but please double check 🙏

@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/return_detailed_diff_only_if_diff_some branch from edbbf6c to 23080b9 Compare December 16, 2024 15:50
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from c83b590 to 0dde15e Compare December 16, 2024 15:51
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from 23080b9 to 5b3b827 Compare December 16, 2024 15:51
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 0dde15e to 8ba928e Compare December 16, 2024 16:01
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from 5b3b827 to dc936a0 Compare December 16, 2024 16:01
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 8ba928e to 9bf7301 Compare December 16, 2024 16:09
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from dc936a0 to ed84692 Compare December 16, 2024 16:09
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Dec 16, 2024

This would only trigger in the case when the Diff decision and the Detailed Diff do not agree. AFAIK this should only happen in the presence of a bug. I can test this manually by overriding something in the code with the wrong logic but I don't believe there is a way to test this otherwise. Do you have any suggestions here @t0yv0?

@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from ed84692 to 59fab15 Compare December 16, 2024 16:17
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 9156d7f to 4a091b8 Compare December 16, 2024 16:19
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from 59fab15 to 1e04a7c Compare December 16, 2024 16:19
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 4a091b8 to 8313f97 Compare December 16, 2024 17:53
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from 1e04a7c to d31fb69 Compare December 16, 2024 17:53
@VenelinMartinov VenelinMartinov force-pushed the vvm/detailed_diff_replace_fallback branch from 8313f97 to f3e38a9 Compare December 17, 2024 09:29
Base automatically changed from vvm/detailed_diff_replace_fallback to vvm/pf_tests_disable_accurate_previews December 17, 2024 09:29
@VenelinMartinov VenelinMartinov force-pushed the vvm/return_detailed_diff_only_if_diff_some branch from d31fb69 to e58abca Compare December 17, 2024 09:29
@VenelinMartinov VenelinMartinov force-pushed the vvm/pf_tests_disable_accurate_previews branch from 1a62de2 to 505e6eb Compare December 17, 2024 09:31
@VenelinMartinov VenelinMartinov marked this pull request as draft December 17, 2024 09:49
Base automatically changed from vvm/pf_tests_disable_accurate_previews to master December 17, 2024 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants