Skip to content

Avoid roundtrip to json when converting Cty to map[string]any #3023

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Apr 23, 2025

This PR is a pure refactor. When converting cty values to map[string]any in the SDKv2, we go through the JSON representation and from JSON to map[string]any. This PR cuts out the JSON part.

Note that this also drops the code for CapsuleTypes and DynamicPseudoType.
The code for DynamicPseudoTypes was clearly wrong and it turns out not used, as Dynamic Values are just treated as unknown.
Capsule types are not a thing in the bridge or the SDKv2.

@VenelinMartinov
Copy link
Contributor Author

This change is part of the following stack:

Change managed by git-spice.

Copy link

codecov bot commented Apr 23, 2025

Codecov Report

Attention: Patch coverage is 95.55556% with 2 lines in your changes missing coverage. Please review.

Project coverage is 68.61%. Comparing base (1036ce2) to head (327c740).
Report is 12 commits behind head on master.

Files with missing lines Patch % Lines
pkg/tfshim/sdk-v2/object_from_cty.go 95.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3023      +/-   ##
==========================================
+ Coverage   68.24%   68.61%   +0.37%     
==========================================
  Files         332      334       +2     
  Lines       42366    43236     +870     
==========================================
+ Hits        28911    29667     +756     
- Misses      11858    11899      +41     
- Partials     1597     1670      +73     

☔ 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 marked this pull request as draft April 23, 2025 11:57
@VenelinMartinov VenelinMartinov marked this pull request as ready for review May 1, 2025 10:18
@VenelinMartinov VenelinMartinov requested review from t0yv0 and a team May 1, 2025 10:18
@VenelinMartinov VenelinMartinov force-pushed the vvm/avoid_json_roundtrip_sdkv2 branch from 8d3ea1d to ec2c208 Compare May 1, 2025 10:41
@VenelinMartinov VenelinMartinov force-pushed the vvm/avoid_json_roundtrip_sdkv2 branch from ec2c208 to 327c740 Compare May 1, 2025 10:48
@t0yv0
Copy link
Member

t0yv0 commented May 1, 2025

The code for DynamicPseudoTypes was clearly wrong and it turns out not used

This code was borrowed from upstream I believe, I doubt it is wrong but perhaps it is less appropriate for this context.

return dec.Decode(v)
}

// objectFromCtyValue takes a cty.Value and converts it to JSON object.
Copy link
Member

Choose a reason for hiding this comment

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

Why is this comment removed?

case cty.String:
return val.AsString()
case cty.Number:
return val.AsBigFloat().Text('f', -1)
Copy link
Member

Choose a reason for hiding this comment

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

This is an observable change right, or it isn't? I like this I've found the val.AsBigFloat().Text('f', -1) is a good method we use elsewhere to preserve precision. It might work better than what this code had before. But it's different. Should the type be json.Number instead of string? Why is it a string that we are returning here?

"github.com/pulumi/pulumi/sdk/v3/go/common/util/contract"
)

func objectFromCtyValue(val cty.Value) map[string]any {
Copy link
Member

Choose a reason for hiding this comment

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

This is a pre-existing problem but if we're editing it let's annotate how this functionality is used in the project. Doing a quick search, it is used to implement

Object(sch SchemaMap) (map[string]interface{}, error)
which is only used by MakeTerraformResult, so the resulting map[string]any is passed to MakeTerraformOutputs and it is its only job in fact. This could be helpful to annotate at least in a comment.

So the ultimate purpose of this code is to convert cty.Value received from TF to the PropertyMap Pulumi domain.

Couple of questions:

  • why do you believe DynamicPseudoType is unused or will be unused in the future? It is quite plausible that TF providers would return a DynamicPseudoType-encoded cty.Values and this PR is deciding to drop support, why is it a good idea?

  • similarly for marks and capsules, why is it that that upstream providers would never use them? We found a very interesting use case for sensitive markers with @corymhall recently when working on the Terraform module project, it helped us propagate secret bits through the TF engine using sensitive() functions - https://github.com/pulumi/pulumi-terraform-module/blob/19814c58e25c402bc3ec8135d1c9817ec0118307/pkg/tfsandbox/tf_file.go#L10 - I have a hunch that these may end up being Marks on the cty.Value wire. There is a possibility that these will be received from TF providers.

Copy link
Member

Choose a reason for hiding this comment

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

The other bit that makes this code somewhat more difficult to maintain than necessary is that we are using a map[string]any type which is not tight enough on what values are admissible, this means changes here need to scrutinize whether MakeTerraformOutputs is going to interpret the code correctly to finish translating to PropertyMap. What if we undertook a refactor that replaced map[string]interface{} with a stricter type when communicating from the shim layer to MakeTerraformOutputs? It could make a few things less ambiguous in the code especially:

  • how unknowns are represented
  • how numbers are represented (reading this I'm still worried MakeTerraformOutputs will not always transform the string back to a PropertyMap number here, this is just not great)

@t0yv0 t0yv0 self-requested a review May 1, 2025 14:21
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.

We could go ahead with this but I had a few comments I would like to get some clarity on first thanks!

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