-
Notifications
You must be signed in to change notification settings - Fork 50
Fix required properties with default in TF projecting as non-optional in Pulumi #3035
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3035 +/- ##
=======================================
Coverage ? 68.59%
=======================================
Files ? 335
Lines ? 43414
Branches ? 0
=======================================
Hits ? 29782
Misses ? 11952
Partials ? 1680 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
03e17f3
to
8e476ca
Compare
This change is part of the following stack: Change managed by git-spice. |
d183add
to
dce432c
Compare
e9bec3e
to
76c4ae7
Compare
76c4ae7
to
64e0342
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo some nits.
Given recent findings in pulumi-aws let's flag this so some providers can implement as part of a major version change. |
@t0yv0 which findings? I've tested this on AWS and I don't think it is affected at all |
Something like pulumi/pulumi-aws#5483 might indicate that at scale user programs rely on type details because output types can unexpectedly get reused in negative type positions. While arguably we do not support this, breaking a lot of programs is bad form; I think that since this change involves type changes we could minimize this impact for example by rolling it out through major releases for Tier 1 providers. |
461c765
to
2d8f21d
Compare
@t0yv0 no types in either GCP or AWS are affected - I think this isn't as widely impacting as initially assumed. Seems redundant to add the additional complexity of flagging the rollout here. |
Can you add a flag to turn it off if needed? On by default but can be turned off. Thanks. |
This PR has been shipped in release v3.109.0. |
…Kv1 providers (#3101) ~Pure refactor.~ Now that adding methods to the `shim` interfaces is no longer breaking we can remove some of the unnecessary interface clutter. The only effective change here is that the SDKv1 now implements `HasDefault`, which means #3035 will apply to it as well. This is unlikely to have a meaningful impact.
This PR fixes the schema generation for TF properties which are marked as
Required
and have a default.In TF these become optional, as the default will get applied. This PR makes this also the case in Pulumi. SDKs should no longer require such properties to be specified as inputs.
This change should be fully backwards-compatible, see here for a per-language discussion.
fixes #2204