-
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
Transformations #3174
Transformations #3174
Conversation
@pgavlin @CyrusNajmabadi @hausdorff This is ready for review. |
53a9712
to
c3d0ac6
Compare
Fixes #2068.
Also adds some more doc comments.
c3d0ac6
to
ecea588
Compare
I really like the simplicity and power here. Overall no real complaints. In terms of your questions:
What you have here intuitively made sense to me. Especially with how the parent transformations come into play, but the child tranforms also get 'last say' to do what they want.
I feel like it would not be uncommon in how a top-level user might make a component, passing in a transformation, and the component could itself pass in its own transformations to other components. I'm ok with it being an array.
I don't care.
I'm ok with that. we can document this accordingly. even in the apply-tags to everything case, you'll have several clear top-level points where you can provide the transform to the resources you care most about. |
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.
Preemptively signing off. I'm not seeing anything here that worries me.
327e904
to
a5a8791
Compare
Turns out this did introduce new issues - mostly because the |
sdk/nodejs/resource.ts
Outdated
* this indicates that the resource will not be transformed. | ||
*/ | ||
export type ResourceTransformation = | ||
(type: string, name: string, props: Inputs, opts: ResourceOptions) => ResourceTransformationResult | undefined; |
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.
So if I want to modify a property, presumably I need to do it like
pulumi.output(props).apply(...)
?
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.
Yes - or pulumi.output(props["myprop"]).apply(myprop => ...)
in the case that you just want to overwrite a single property in a way that is conditional on the value passed in.
This is slightly more cumbersome to author - but is more expressive - as it allows things like injecting new dependencies. For a low level power-user feature like this - my feeling was that providing flexibility/expressivity is more valuable than providing ease-of-authoring.
I've added a new test that is motivated by the scenario in #2068 (comment) which shows how this can be taken advantage of for a real (and fairly complex) scenario involving injecting a dependency on one child into another child.
Also - FWIW - in most common cases I've come up with - the transformation is overwriting the value or injecting a new default - so doesn't actually need to look at the existing input value.
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.
The only high-level feedback I have is that it might be nice to have fully-resolved inputs before running transformations, but that I think that might drastically complicate the code. LGTM otherwise.
/** | ||
* The new properties to use in place of the original `props` | ||
*/ | ||
props: Inputs; |
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.
Do we envision supporting name transforms eventually? (May help with auto-naming stuff.)
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.
Ahh, nevermind, I realize now that you can simply set the physical resource name property. No need to change the logical name.
Include the Resource reference in the args bag. Also add more complex stateful transformation test.
The goal of transformations is to allow outer code to override the behaviour of inner code. So to be consistent, we should apply parent transformations after applying child transformations, from the bottom of the hierarchy up to the stack. This ensure stack level transformations have the ability to observe and override if necessary all other layers of transformations.
Opened #3283 to track adding this to Python as well. We should do that ASAP as a follow-up. |
Adds Python support for resource transformations aligned with the existing NodeJS support in #3174. This PR also moves processing of transformations to earlier in the resource construction process (for both NodeJS and Python) to ensure that invariants established in the constructor cannot be violated by transformations. This change can technically be a breaking change, but given that (a) the transformations features was just released in 1.3.0 and (b) the cases where this is a breaking change are uncommon and unlikely to have been reliable anyway - it feels like a change we should make now. Fixes #3283.
Adds the ability to provide
transformations
to modify the properties and resource options that will be used for any child resource of a component or stack.This offers an "escape hatch" to modify the behaviour of a component by peeking behind it's abstraction. For example, it can be used to add a resource option (
additionalSecretOutputs
,aliases
,protect
, etc.) to a specific known child of a component, or to modify some input property to a child resource if the component does not (yet) expose the ability to control that input directly. It could also be used for more interesting scenarios - such as:import
onto all resources by doing a lookup into a name=>id mappingBecause this feature makes it possible to peek behind a component abstraction, it must be used with care in cases where the component is versioned independently of the use of transformations. Also, this can result in "spooky action at a distance", so should be used judiciously. That said - this can be used as an escape hatch to unblock a wide variety of common use cases without waiting on changes to be made in a component implementation, as well
Each transformation is passed the
name
,type
,props
andopts
that are passed into theResource
constructor for any resource descended from the resource that has the transformation applied. The transformation callback can optionally return alternate versions of theprops
andopts
to be used in place of the originally provided values.Transformations are applied iteratively in order of specificity - so stack transformations are applied first, then parent transformations (in order), then child transformations (in order). The
props
andopts
are passed through each layer of these transformations.Questions for reviewers:
transformations
instead of justtransformation
? It seems exceedingly rare to provide multiple at one callsite - and in the case this is necessary, it seems the user could compose them functionally themselves.transformations
vs.transforms
(vs. something else)?Fixes #2068.