-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fix IPV6 cidr blocks #197
Fix IPV6 cidr blocks #197
Conversation
Depends on pulumi/pulumi-aws-native#1797 Because of pulumi/pulumi-aws-native#1798 we need to add a special case for Ipv6 cidr blocks that are added to the VPC via a `VPCCidrBlock` resource. This PR adds special handling where we track both the `Vpc` and the `VpcCidrBlock` resource and swap any references.
src/graph.ts
Outdated
@@ -115,15 +115,19 @@ export class GraphBuilder { | |||
// Map of resource logicalId to GraphNode. Allows for easy lookup by logicalId | |||
cfnElementNodes: Map<string, GraphNode>; | |||
|
|||
// If the app has a VpcCidrBlock resource, this will be set to the GraphNode representing it | |||
vpcCidrBlockNode?: GraphNode; |
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.
Nit: I'd prefer if we'd save that in the result of GraphBuilder::build
. E.g. turn this into a GraphResult
struct that has the necessary params we need.
I wouldn't expect a Builder
to store state beyond what's needed to build
src/graph.ts
Outdated
this.vpcCidrBlockNode = node; | ||
} | ||
if (resource.Type === 'AWS::EC2::VPC') { | ||
this.vpcNode = node; |
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.
Can a stack only have a single 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.
I think that case would be exceedingly rare, but I should be able to update this to handle that case
src/graph.ts
Outdated
// Here we switching the dependency to be on the `VpcCidrBlock` resource (since that will also have a dependency | ||
// on the VPC resource) | ||
if ( | ||
logicalId === this.vpcNode?.logicalId && |
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.
Along the lines of my earlier Q, what happens if the stack has multiple VPCs?
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.
Nice!
Depends on pulumi/pulumi-aws-native#1797
Because of pulumi/pulumi-aws-native#1798 we need to add a special case for Ipv6 cidr blocks that are added to the VPC via a
VPCCidrBlock
resource.This PR adds special handling where we track both the
Vpc
and theVpcCidrBlock
resource and swap any references.