Skip to content

Conversation

@VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented May 29, 2025

This PR adds support for correctly serializing and de-serializing properties with a Dynamic Type schema. This ensures that properties of this schema type can be correctly saved to state and then read back with any provider version.

There are a few changes which had to be made to make this all work:

  1. When marshalling values to JSON we now need to pass in the schema type in order to check if they are Dynamic type. Previously we were using the value type and assumed the two match. This is not correct for properties with a Dynamic schema type. We also thread this information through various places.
  2. When recording RawStateDeltas (these are the "diffs" between the TF state and Pulumi state), we need to make sure that properties with a Dynamic schema type always get a Replace delta. This is a full recording of the TF value, instead of a "diff" with the pulumi state. This is because the JSON representation of properties with Dynamic schema type need to contain their value type as well as the value itself. This is then used to read the state back into the correct runtime type.
  3. Added a cty and hcty valueshim implementations for the cty package and the Hashi fork of the cty package. This is necessary because the SDKv1 uses the cty package while the SDKv2 uses the hcty package. The two implementations are practically identical.
  4. Some unit and integration tests to make sure this all works correctly and stays that way.

fixes #3078

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented May 29, 2025

@VenelinMartinov VenelinMartinov marked this pull request as draft May 29, 2025 10:27
@VenelinMartinov VenelinMartinov changed the title Add a test for DynamicPseudoType reproducing the error [WIP] Implement Dynamic Type serialization May 29, 2025
@VenelinMartinov VenelinMartinov changed the base branch from vvm/pf_dynamic_diff_err to vvm/clean_up_unnecessary_shim_interfaces May 29, 2025 12:00
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 0b9e04a to 1a85114 Compare May 29, 2025 12:00
@codecov
Copy link

codecov bot commented May 29, 2025

Codecov Report

Attention: Patch coverage is 68.96552% with 90 lines in your changes missing coverage. Please review.

Project coverage is 68.62%. Comparing base (246c306) to head (2c92446).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/valueshim/cty.go 3.44% 28 Missing ⚠️
pkg/valueshim/hcty.go 87.61% 10 Missing and 3 partials ⚠️
pkg/valueshim/tftype_json.go 74.28% 6 Missing and 3 partials ⚠️
pkg/valueshim/tfvalue.go 79.31% 4 Missing and 2 partials ⚠️
pkg/pf/internal/schemashim/datasource.go 0.00% 4 Missing ⚠️
...kg/pf/internal/schemashim/object_pseudoresource.go 0.00% 4 Missing ⚠️
pkg/pf/internal/schemashim/resource.go 0.00% 4 Missing ⚠️
pkg/tfbridge/rawstate.go 85.18% 3 Missing and 1 partial ⚠️
pkg/pf/proto/object.go 0.00% 3 Missing ⚠️
pkg/pf/proto/resource.go 0.00% 3 Missing ⚠️
... and 6 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3099      +/-   ##
==========================================
- Coverage   68.71%   68.62%   -0.10%     
==========================================
  Files         336      336              
  Lines       43690    43811     +121     
==========================================
+ Hits        30022    30064      +42     
- Misses      11954    12037      +83     
+ Partials     1714     1710       -4     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@VenelinMartinov VenelinMartinov force-pushed the vvm/clean_up_unnecessary_shim_interfaces branch from ba5a08d to 6259947 Compare May 29, 2025 13:14
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch 2 times, most recently from a49af42 to 42f324d Compare May 29, 2025 13:38
@VenelinMartinov VenelinMartinov force-pushed the vvm/clean_up_unnecessary_shim_interfaces branch from 8d6a25c to f6dce5d Compare June 3, 2025 16:30
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 48e0f35 to 0bbbf1c Compare June 3, 2025 16:30
Base automatically changed from vvm/clean_up_unnecessary_shim_interfaces to main June 3, 2025 16:56
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 0bbbf1c to d2a5a22 Compare June 5, 2025 14:46
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 16d20be to 2a50b2b Compare June 9, 2025 09:55
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 62b2953 to a580ec9 Compare June 9, 2025 10:53
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 335724a to 478e3cb Compare June 9, 2025 11:28
@VenelinMartinov VenelinMartinov marked this pull request as ready for review June 9, 2025 11:51
@VenelinMartinov VenelinMartinov requested review from a team and t0yv0 and removed request for t0yv0 June 9, 2025 11:51
@VenelinMartinov VenelinMartinov force-pushed the vvm/fix_dynamic_parsing_pf branch from 88b4680 to e14db58 Compare June 9, 2025 11:57
@VenelinMartinov VenelinMartinov changed the title [WIP] Implement Dynamic Type serialization Implement Dynamic Type serialization Jun 9, 2025
Copy link
Contributor

@Zaid-Ajaj Zaid-Ajaj left a comment

Choose a reason for hiding this comment

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

LGTM, just one question about now including timeouts in the delta

@VenelinMartinov VenelinMartinov enabled auto-merge (squash) June 10, 2025 12:49
@VenelinMartinov VenelinMartinov merged commit 67ba100 into main Jun 10, 2025
138 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/fix_dynamic_parsing_pf branch June 10, 2025 12:51
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v3.110.0.

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.

DynamicPseudoType is not yet supported

4 participants