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

[Breaking] Remove UnknownValue from shim.Schema #1974

Merged
merged 1 commit into from
May 15, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented May 15, 2024

It looks like nothing uses this code, so we don't need to keep it around. While this is technically a breaking change, I don't expect that anyone would use this method.

It looks like nothing uses this code, so we don't need to keep it around. While this is
technically a breaking change, I don't expect that anyone would use this method.
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.67%. Comparing base (98847bf) to head (ba4194d).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1974      +/-   ##
==========================================
+ Coverage   60.55%   60.67%   +0.11%     
==========================================
  Files         331      331              
  Lines       44726    44648      -78     
==========================================
+ Hits        27084    27090       +6     
+ Misses      16120    16036      -84     
  Partials     1522     1522              

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

@t0yv0
Copy link
Member

t0yv0 commented May 15, 2024

No concerns of this change breaking anyone, but there is some knowledge encoded here that's worth calling out, at the very least in #1904

Similar code to this one has moved (or been copied) to MakeTerraformInputs and is causing problems.

Fundamentally my theory of what this is about is that list/set-nested blocks in TF can never accept null or unknown value but in Pulumi the projection of these blocks does indeed have to handle receiving nulls and unknowns. It appears that it has attempted doing so by replacing them with plausible empty values.

Is this common knowledge or should I take time to document/track a bit more? If this is understood I'm LGTM on merging the simplification.

@iwahbe
Copy link
Member Author

iwahbe commented May 15, 2024

I think that as long as the code is present in MakeTerraformInputs (where it is actually used), then I'm happy with it as is.

Fundamentally my theory of what this is about is that list/set-nested blocks in TF can never accept null or unknown value but in Pulumi the projection of these blocks does indeed have to handle receiving nulls and unknowns. It appears that it has attempted doing so by replacing them with plausible empty values.

This is definitely what the code is supposed to do. The code is just never invoked.

Is this common knowledge or should I take time to document/track a bit more? If this is understood I'm LGTM on merging the simplification.

I don't think it's common knowledge but it is present in the part of the code base where it is needed: MakeTerraformInputs.

@iwahbe iwahbe merged commit 38a6882 into master May 15, 2024
11 checks passed
@iwahbe iwahbe deleted the remove-UnknownValue-from-shim.Schema branch May 15, 2024 16:01
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.

None yet

2 participants