Propagate inputs to outputs during preview. #3327
Conversation
|
@lukehoban @CyrusNajmabadi I would like to get this in as soon as possible s.t. we can begin vetting these changes downstream. I am happy to provide any additional details and answer additional questions as needed. |
|
Any update on this? This is a critical change for the reliability of pulumi's previews. Without it, we really cannot trust it. |
These changes restore a more-correct version of the behavior that was disabled with #3014. The original implementation of this behavior was done in the SDKs, which do not have access to the complete inputs for a resource (in particular, default values filled in by the provider during `Check` are not exposed to the SDK). This lack of information meant that the resolved output values could disagree with the typings present in a provider SDK. Exacerbating this problem was the fact that unknown values were dropped entirely, causing `undefined` values to appear in unexpected places. By doing this in the engine and allowing unknown values to be represented in a first-class manner in the SDK, we can attack both of these issues. Although this behavior is not _strictly_ consistent with respect to the resource model--in an update, a resource's output properties will come from its provider and may differ from its input properties--this behavior was present in the product for a fairly long time without significant issues. In the future, we may be able to improve the accuracy of resource outputs during a preview by allowing the provider to dry-run CRUD operations and return partially-known values where possible. These changes also introduce new APIs in the Node and Python SDKs that work with unknown values in a first-class fashion: - A new parameter to the `apply` function that indicates that the callback should be run even if the result of the apply contains unknown values - `containsUnknowns` and `isUnknown`, which return true if a value either contains nested unknown values or is exactly an unknown value - The `Unknown` type, which represents unknown values The primary use case for these APIs is to allow nested, properties with known values to be accessed via the lifted property accessor even when the containing property is not fully know. A common example of this pattern is the `metadata.name` property of a Kubernetes `Namespace` object: while other properties of the `metadata` bag may be unknown, `name` is often known. These APIs allow `ns.metadata.name` to return a known value in this case. In order to avoid exposing downlevel SDKs to unknown values--a change which could break user code by exposing it to unexpected values--a language SDK must indicate whether or not it supports first-class unknown values as part of each `RegisterResourceRequest`. These changes also allow us to avoid breaking user code with the new behavior introduced by the prior commit. Fixes #3190.
pgavlin
force-pushed
the
pgavlin/propagateInputs
branch
from
a0c94ca
to
a5feec4
Compare
The fundamental issue is that we had code that assumed that any output where `isKnown` resolves to `false` would also resolve to a value that contained some first-class unknown. When this assumption was violated, it was possible to end up with outputs with values that were incorrectly treated as known, especially when used with the new variant of `apply`.
pgavlin
force-pushed
the
pgavlin/propagateInputs
branch
from
86acccc
to
78cc8f3
Compare
|
@CyrusNajmabadi @lukehoban PTAL. I've update the fix to preserve backwards-compatibility for |
|
Overall i'm ok with the conceptual change here. However, i'm hoping there are a few things we can do to:
If that's doable, i'd love to see that happen, and will be happy to review asap. Once we reach a conclusion on this part, then i can review the python bits. I'm avoiding that for now until we can decide the best way to maintain the ts/js bits. Thanks! |
|
Overall js/ts side is fine with me. some minor sugestions on potential ways to clear things up. LMK when the python side is ready to be reviewed in case it needs any corresponding changes. i can do tomorrow! |
| // advantage of this to allow proxied property accesses to return known values even if other properties of | ||
| // the containing object are unknown. | ||
| public apply<U>(func: (t: T) => Input<U>, runWithUnknowns?: boolean): Output<U> { | ||
| const applied = Promise.all([this.promise(/*withUnknowns*/ true), this.isKnown, this.isSecret]) |
There was a problem hiding this comment.
so it's unclear to me why you souldn't just pass runWithUnknowns in to this.promise(). If the user is not passing a value in (or passing in false), then we wouldn't want unknowns from the underlying promise right? We'd only care about htem if the user actually passed in true here, right? in other words, unilaterally passing in true here doesn't seem necessary. if it is, can we doc why?
|
LGTM. There are a couple of previous review comments that weren't addressed. if it would be easy to do so, i'd like that. but it's not a huge deal. |
This reverts commit 137fd54. We do not want to include these changes in 1.5.2.
This is a reapplication of the changes from #3245, with a couple critical bugfixes. The original changes are in the first commit; the fixes are in the second.