Propagate inputs to outputs during preview. #3245
Conversation
|
@lukehoban most of these changes are from #3216. The input-propagation changes are all in I'll add tests and update the commit message & PR comment tomorrow. |
pgavlin
force-pushed
the
pgavlin/propagateInputs
branch
from
281d12b
to
8be0036
Compare
|
Working on test coverage now. |
|
Okay, unit tests are in. Integration tests for Python and Node are next. |
pgavlin
force-pushed
the
pgavlin/propagateInputs
branch
from
c5ae908
to
5cf78a7
Compare
| @@ -208,6 +208,8 @@ func (s *CreateStep) Apply(preview bool) (resource.Status, StepCompleteFunc, err | |||
| s.new.ID = id | |||
| s.new.Outputs = outs | |||
| } | |||
| } else { | |||
| s.new.Outputs = s.new.Inputs | |||
| } | |||
There was a problem hiding this comment.
could we flip the 'if' we're in. i'm a big fan of if case simpleStuff else really complex stuff versus if !case complex stuff else really simple stuff.
|
|
||
| const result1 = output1.foo; | ||
| assert.equal(await result1.isKnown, true); | ||
| assert.equal(await result1.promise(), "foo"); |
| """ | ||
|
|
||
| def contains_unknowns(val: Any) -> bool: | ||
| return rpc.contains_unknowns(val) |
There was a problem hiding this comment.
todo: review the python side.
|
done with initial pass. still have to review python. zero impl concerns. all my stuff is about doc'ing api/impl for the best long term health of this complex part of the sdk. |
| // runWithUnknowns requests that `func` is run even if `isKnown` resolves to `false`. This is used to allow | ||
| // callers to process fully- or partially-unknown values and return a known result. the output proxy takes | ||
| // advantage of this to allow proxied property accesses to return known values even if other properties of | ||
| // the containing object are unknown. |
|
Everything looks good (though i haven't reviewed python). You can move forward if you want, or you can wait on me to fully sign off once i check that too. |
There was a problem hiding this comment.
LGTM.
There's more change here than I expected - and I'm having some trouble grasping the full set of potential impact this could have to make sure we've done the appropriate testing. But all the changes look right in isolation. I think we should try to land and make sure we get this exposed to more real world validation in master over the next week.
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. Fixes #3190.
These changes allow the Node and Python SDKs to work with unknown values in a first-class fashion by introducing a handful of new APIs: - 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.
Co-Authored-By: CyrusNajmabadi <cyrus.najmabadi@gmail.com>
pgavlin
force-pushed
the
pgavlin/propagateInputs
branch
from
d747660
to
6dadfa8
Compare
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
Checkare not exposed to the SDK). This lack of information meant thatthe 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
undefinedvalues to appear inunexpected 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 allow the Node and Python SDKs to work with unknown values
in a first-class fashion by introducing a handful of new APIs:
applyfunction that indicates that thecallback should be run even if the result of the apply contains
unknown values
containsUnknownsandisUnknown, which return true if a valueeither contains nested unknown values or is exactly an unknown value
Unknowntype, which represents unknown valuesThe 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.nameproperty of a KubernetesNamespaceobject: while other properties of the
metadatabag may be unknown,nameis often known. These APIs allowns.metadata.nameto return aknown 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.