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

Fix PF provider_server detailed diff handling #2628

Merged
merged 4 commits into from
Nov 25, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Nov 18, 2024

This change fixes an issue with the provider_server implementation's detailed diff handling. Previously passing a detailed diff to it would result in the previews being deleted. This is tested as part of #2629

The problem was that we were re-calculating the diffs and replaces keys for the GRPC Diff protocol in the provider_server implementation but also doing that incorrectly. Instead this change now makes provider_server's marshalDiff just pass through the diffs and replaces which we have already calculated in

replaceKeys := topLevelPropertyKeySet(resSchemaMap, resFields, planResp.RequiresReplace)
changedKeys := topLevelPropertyKeySet(resSchemaMap, resFields, diffAttributePaths(tfDiff))

fixes #2620

Copy link

codecov bot commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 61.76471% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.39%. Comparing base (bc33162) to head (12bb0f7).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
pkg/pf/internal/plugin/provider_server.go 61.76% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2628      +/-   ##
==========================================
+ Coverage   69.35%   69.39%   +0.03%     
==========================================
  Files         301      301              
  Lines       38558    38560       +2     
==========================================
+ Hits        26743    26759      +16     
+ Misses      10295    10281      -14     
  Partials     1520     1520              

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


🚨 Try these New Features:

@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from b677d0f to 3d1d6b5 Compare November 19, 2024 14:09
@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/pf_cross_tests_for_computed_in_sets November 19, 2024 18:11
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 3d1d6b5 to e2a772c Compare November 19, 2024 18:11
@VenelinMartinov VenelinMartinov marked this pull request as draft November 19, 2024 18:12
@VenelinMartinov VenelinMartinov changed the base branch from vvm/pf_cross_tests_for_computed_in_sets to pf-cross-tests-for-computed-set November 19, 2024 18:41
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from e2a772c to cdb2b0f Compare November 19, 2024 18:41
@VenelinMartinov VenelinMartinov changed the base branch from pf-cross-tests-for-computed-set to vvm/pf_cross_tests_for_sets_with_defaults November 19, 2024 18:53
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from cdb2b0f to b1a1cbf Compare November 19, 2024 18:53
@VenelinMartinov VenelinMartinov force-pushed the vvm/pf_cross_tests_for_sets_with_defaults branch from 2bd520a to 02d9ef9 Compare November 20, 2024 14:09
@VenelinMartinov VenelinMartinov changed the base branch from vvm/pf_cross_tests_for_sets_with_defaults to vvm/pf_diff_tests_for_secrets November 20, 2024 14:10
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from b1a1cbf to 27b43ab Compare November 20, 2024 14:10
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 27b43ab to 906cb5a Compare November 20, 2024 14:13
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 906cb5a to f7fd6eb Compare November 20, 2024 17:42
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from f7fd6eb to 0dac56b Compare November 20, 2024 19:18
Base automatically changed from vvm/pf_diff_tests_for_secrets to master November 21, 2024 10:51
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 0dac56b to 72c930a Compare November 21, 2024 12:42
@VenelinMartinov VenelinMartinov changed the base branch from master to vvm/pf_diff_test_long_list November 21, 2024 13:04
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 72c930a to 4fe072a Compare November 21, 2024 13:04
Base automatically changed from vvm/pf_diff_test_long_list to master November 21, 2024 13:59
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.

This is code that is copied over from pulumi/.../plugin, so I'm hesitant to change it. Can we delegate to upstream's implementation?

Also, this change needs tests in this PR. For stacked PRs, each PR still needs to be atomically OK.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Nov 22, 2024

We already have changes to our copy, so resolving the divergence is out of scope here. Getting rid of our copy is blocked on the config encoding work. The upstream copy does not seem to work correctly for the detailed diff either but is only used in tests.

EDIT: Added some unit tests. The tests in #2629 should add quite broad coverage here. I have separated this into a different PR to make reviewing easier.

@VenelinMartinov VenelinMartinov force-pushed the vvm/pv_tranform_for_removing_secrets_and_outputs branch from f5786e2 to fb926f8 Compare November 22, 2024 19:18
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 4c61646 to 4acb27b Compare November 22, 2024 19:18
Base automatically changed from vvm/pv_tranform_for_removing_secrets_and_outputs to master November 22, 2024 20:20
@t0yv0
Copy link
Member

t0yv0 commented Nov 22, 2024

Very surprised but this seems to hold?

https://github.com/search?q=org%3Apulumi%20plugin.NewProviderServer&type=code

Looks like these are mostly non-production uses.

@t0yv0
Copy link
Member

t0yv0 commented Nov 22, 2024

Can you point out which line had the bug you're fixing? Very non-obvious.

@t0yv0
Copy link
Member

t0yv0 commented Nov 22, 2024

I'm inclined to get this in given your finding that this Core code is not in prod anywhere. We need to maintain a working copy then. Oof. Please address the enum cast concern the rest are nits.

@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from 4acb27b to 10b8e75 Compare November 25, 2024 12:23
Comment on lines +121 to +131
diffs = make([]string, len(diff.ChangedKeys))
for i, k := range diff.ChangedKeys {
diffs[i] = string(k)
}
replaces = make([]string, len(diff.ReplaceKeys))
for i, k := range diff.ReplaceKeys {
replaces[i] = string(k)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you point out which line had the bug you're fixing? Very non-obvious.

The fix is really here - diffs and replaces should be top-level keys, which we already calculate before coming here. The meat of this change is to use these instead of re-calculating these incorrectly.
@t0yv0

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.

🚢

@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_provider_server_detailed_diff1 branch from f8aba52 to 12bb0f7 Compare November 25, 2024 14:19
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.

Please open a bug in core so they can make this change upstream.

@VenelinMartinov VenelinMartinov merged commit ea35eb2 into master Nov 25, 2024
17 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/fix_provider_server_detailed_diff1 branch November 25, 2024 15:23
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.

CLI does not display preview for PF when detailed diff is specified
3 participants