-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Send output values to transforms for dependency tracking #15637
Conversation
7c63df9
to
fe1a595
Compare
Changelog[uncommitted] (2024-03-19)Features
|
e625a63
to
5f33588
Compare
This needs a bit more set handling for the dependency arrays, going to try and get #15671 and maybe some other small changes in first to minimise diff size. |
4d25406
to
5bf73d9
Compare
5bf73d9
to
0b47e8a
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.
This looks good go me, with one question inline.
...engine--transform-functions-are-sent-output-values-with-property-dependency-information.yaml
Outdated
Show resolved
Hide resolved
} else { | ||
// If we ran transforms we would have merged all the dependencies togther already, but if we didn't we want to | ||
// ensure any output values add their dependencies to the dependencies map we send to the provider. | ||
for key, output := range props { |
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.
I'm not sure I'm following this change properly. Since we didn't upgrade the props in the case where there are no transforms, couldn't we just use the old code from https://github.com/pulumi/pulumi/pull/15637/files#diff-984b3279d61b588030474831898bb76d093d863ca2d0015bb081d842602a9ab7L1665? Or why is this change necessary?
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.
This is pretty much that code but for Set instead of slice.
Does this only happen if that property has dependencies? |
Yes |
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Tentative changelog: ### Features - [docs] Implement constructor syntax examples for every resource in typescript, python, csharp and go [#15624](#15624) - [docs] Implement YAML constructor syntax examples in the docs [#15791](#15791) - [engine] Send output values with property dependency information to transform functions [#15637](#15637) - [engine] Add a --continue-on-error flag to pulumi destroy [#15727](#15727) - [sdk/go] Make `property.Map` keyed by `string` not `MapKey` [#15767](#15767) - [sdk/nodejs] Make function serialization work with typescript 4 and 5 [#15761](#15761) - [sdk/python] Improve the error message when depends_on is passed objects of the wrong type [#15737](#15737) ### Bug Fixes - [auto/{go,nodejs,python}] Make sure to read complete lines before trying to deserialize them as engine events [#15778](#15778) - [cli/plugin] Fix installing local language plugins on Windows [#15715](#15715) - [engine] Don't delete stack outputs on failed deployments [#15754](#15754) - [engine] Fix a panic when updating provider version in a run using --target [#15716](#15716) - [engine] Handle that Assets & Archives can be returned from providers without content. [#15736](#15736) - [engine] Fix the engine trying to delete a protected resource caught in a replace chain [#15776](#15776) - [sdkgen/docs] Add missing newline for `Coming soon!` [#15783](#15783) - [programgen/dotnet] Fix generated code for a list of resources used in resource option DependsOn [#15773](#15773) - [programgen/{dotnet,go}] Fixes emitted code for object expressions assigned to properties of type Any [#15770](#15770) - [sdk/go] Fix lookup of plugin and program dependencies when using Go workspaces [#15743](#15743) - [sdk/nodejs] Export automation.tag.TagMap type [#15774](#15774) - [sdk/python] Wait only for pending outputs in the Python SDK, not all pending asyncio tasks [#15744](#15744) ### Miscellaneous - [sdk/nodejs] Reorganize function serialization tests [#15753](#15753) - [sdk/nodejs] Move mockpackage tests to closure integration tests [#15757](#15757)
Description
This engine change is necessary for correct dependency tracking of properties through transform functions.
Unlike other parts of the runtime/provider/engine interface transform functions do not have a property dependencies map sent or returned. Transforms rely entirely on the dependency arrays in output values for their dependency tracking.
This change takes the property dependency map and upgrades all the input properties to be output values tracking those same dependencies (if a property doesn't have any dependencies it doesn't need to be upgraded to an output value).
This upgrade is only done if we're using transform functions, there's currently no need to do this unless we're using transforms.
After running the transforms we downgrade the output values back to plain/secret/computed values if we're registering a custom resource. If we're registering a component resource we just leave all the properties upgraded as output values.
We also rebuild the resources dependencies slice and propertyDependency map with the dependencies recorded in the transformed properties. If the transform didn't change the properties this will just be the same data we encoded into the output values in the properties structure.
There are a couple of things to keep in mind with this change.
Firstly using a transform can cause a property that was being sent to a component provider as just a
resource.NumberProperty
to start being sent as anOutputProperty
with theNumberProperty
as its element. Component providers should handle that, but it's feasible that it could break some component code. This is a fairly limited blast radius as it will only happen when that user starts using a transform function.Secondly, we can't know in the engine if a property dependencies are from a top level output value or a nested output. That is SDKs send both of the following property shapes the same way to the engine for custom resources:
Currently both would get upgraded to
Output(Array([Number(1)]), ["dep"])
. For a component resource the SDK will send output values, and as long as they define a superset of the dependencies in the property map then we don't add the top-level output tracking the same dependency set.Checklist
make tidy
to update any new dependenciesmake lint
to verify my code passes the lint checkgofumpt
make changelog
and committed thechangelog/pending/<file>
documenting my change