-
Notifications
You must be signed in to change notification settings - Fork 104
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
Add support for custom providers passed in ComponentResourceOptions #176
Conversation
d40cd57
to
46dc7c4
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.
The provider should be inherited from the parent component - which is the canonical approach we’ve encouraged for Component design.
That should cause everything (or nearly everything) in this PR to not be necessary - as you already get the same result without the need to manually pass this information down from the component options to child options.
What issue did you run into which caused you to create this PR?
@lukehoban That's what I thought, but then I've seen that all the resources are created using the default provider (even though an explicit provider was passed). |
7910073
to
731a505
Compare
nodejs/awsx/ec2/natGateway.ts
Outdated
@@ -31,7 +31,7 @@ export class NatGateway | |||
constructor(name: string, vpc: x.ec2.Vpc, args: NatGatewayArgs | ExistingNatGatewayArgs, opts: pulumi.ComponentResourceOptions = {}) { | |||
super("awsx:x:ec2:NatGateway", name, {}, { parent: vpc, ...opts }); | |||
|
|||
const parentOpts = { parent: this }; | |||
const parentOpts = { parent: vpc }; |
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 doesn't seem right. we'd want the child resources to see this NatGateway as their parent. is there a reason this needed ot happen?
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.
Fixed
nodejs/awsx/ec2/vpc.ts
Outdated
@@ -71,12 +71,12 @@ export class Vpc extends pulumi.ComponentResource { | |||
enableDnsHostnames: utils.ifUndefined(args.enableDnsHostnames, true), | |||
enableDnsSupport: utils.ifUndefined(args.enableDnsSupport, true), | |||
instanceTenancy: utils.ifUndefined(args.instanceTenancy, "default"), | |||
}); | |||
}, { parent: this }); |
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 good.
nodejs/awsx/ec2/vpc.ts
Outdated
this.id = this.vpc.id; | ||
|
||
// Create the appropriate subnets. Default to a single public and private subnet for each | ||
// availability zone if none were specified. | ||
const topology = new VpcTopology(this, name, cidrBlock, numberOfAvailabilityZones, opts); | ||
const topology = new VpcTopology(this, name, cidrBlock, numberOfAvailabilityZones, { parent: this, ...opts }); |
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.
...opts
should be necessary. we should just need to pass along us as the parent.
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.
Fixed. Only opts
left
db7b588
to
a73beb0
Compare
a73beb0
to
18b066d
Compare
Closing out as the primary parts of this PR have been merged in. Thanks for the contribution @igorshapiro ! |
No description provided.