Enable output values by default #8014
Conversation
justinvp
force-pushed
the
justin/outputvalues_enable
branch
2 times, most recently
from
ea380a1
to
44d9d06
Compare
Enable output values by default in the resource monitor and change the polarity of the envvar from `PULUMI_ENABLE_OUTPUT_VALUES` to `PULUMI_DISABLE_OUTPUT_VALUES`.
Marshal all unknown output values as `resource.MakeComputed(resource.NewStringProperty(""))` when not keeping output values, which is consistent with what the SDKs do.
Otherwise, when `v.OutputValue().Element` is nil, `resource.MakeComputed(v.OutputValue().Element)` will be marshaled as a null value rather than as an unknown sentinel.
Before we expanded the meaning of `resource.Output`, `MarshalProperties` always skipped output values:
```go
if v.IsOutput() {
logging.V(9).Infof("Skipping output property for RPC[%s]: %v", opts.Label, key)
}
```
As part of expanding the meaning of `resource.Output`, I'd adjusted `MarshalProperties` to only skip output values when the value was unknown and when not keeping output values:
```go
if v.IsOutput() && !v.OutputValue().Known && !opts.KeepOutputValues {
logging.V(9).Infof("Skipping output property for RPC[%s]: %v", opts.Label, key)
}
```
However, this doesn't work the way we want when marshaling properties that include unknown output values to a provider that does not accept outputs. In that case, `opts.KeepOutputValues` will be `false` because we want the marshaler to fall back to returning non-output-values (e.g. unknown sentinel value for unknown output values), but instead of getting the intended fallback values, the unknown output values are skipped (not what we want).
I suspect we may be able to delete the output value skipping in `MarshalProperties` altogether (it's odd that it is skipping `resource.Output` but not `resource.Computed`), but to avoid any unintended side effects of doing that, instead, this commit introduces a new `MarshalOptions.DontSkipOutputs` option that can be set to `true` to opt-in to not skipping output values when marshaling. The check in `MarshalProperties` now looks like this:
```go
if !opts.DontSkipOutputs && v.IsOutput() && !v.OutputValue().Known {
logging.V(9).Infof("Skipping output property for RPC[%s]: %v", opts.Label, key)
}
```
`opts.DontSkipOutputs` is set to `true` when marshaling properties for calls to a provider's `Construct` and `Call`.
This commit adds support for deserializing output values, which is needed in some cases when serialized inputs are returned as outputs in the SDK.
This commit adds support for deserializing output values, which is needed in some cases when serialized inputs are returned as outputs in the SDK.
justinvp
force-pushed
the
justin/outputvalues_enable
branch
from
44d9d06
to
fa645fe
Compare
|
@pgavlin, @t0yv0, please take another look. Some of the existing MLC tests failed when output values were enabled by default and the subsequent commits here address the problems.
|
| @@ -161,12 +161,32 @@ describe("runtime", () => { | |||
| ...isSecret && { secret: isSecret }, | |||
| ...(deps.length > 0) && { dependencies: deps }, | |||
| }, | |||
| expectedRoundTrip: new Output(resources, | |||
| Promise.resolve(isKnown ? tv.expected : undefined), Promise.resolve(isKnown), | |||
| Promise.resolve(isSecret), Promise.resolve([])), | |||
There was a problem hiding this comment.
The last element is weird, I am guessing this corresponds to
/** @internal */
public constructor(
resources: Set<Resource> | Resource[] | Resource,
promise: Promise<T>,
isKnown: Promise<boolean>,
isSecret: Promise<boolean>,
allResources: Promise<Set<Resource> | Resource[] | Resource> | undefined) {
Is it possible to skip it (default undefined)?
It's weird that the constructor has two parameters where resources can be passed in but there are probably reasons.
There was a problem hiding this comment.
It's weird that the constructor has two parameters where resources can be passed in but there are probably reasons.
Yeah, it's a little strange. It was introduced in #3680. I'm not entirely familiar with the backstory, but looks like it was done this way to address some SxS problems.
Is it possible to skip it (default undefined)?
We might be able to make it optional with a default value of undefined.
|
Still looking good, read through the extra commits. Flags are getting unfortunately complex but I appreciate why this is happening. |
Enable output values by default in the resource monitor and change the polarity of the envvar from
PULUMI_ENABLE_OUTPUT_VALUEStoPULUMI_DISABLE_OUTPUT_VALUES.We've had a cron job running in the
pulumi/examplesrepro with the feature both enabled and disabled, and everything is looking good on those runs (as expected), so we're comfortable enabling it by default.Fixes #8007