Skip to content
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 AWS::EC2::NatGateway #76

Merged
merged 1 commit into from
Mar 9, 2016
Merged

Add support for AWS::EC2::NatGateway #76

merged 1 commit into from
Mar 9, 2016

Conversation

tjcorr
Copy link
Collaborator

@tjcorr tjcorr commented Mar 7, 2016

fixes #72

@@ -244,6 +181,18 @@ trait Subnet extends AvailabilityZone with Outputs {

f(sub) ++ sub.andOutput(name, name)
}

def nat(routeTables: Seq[`AWS::EC2::RouteTable`])(implicit s: `AWS::EC2::Subnet`) = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to accept the VPC Gateway Attachment resource and set it as a DependsOn for the EIP. See http://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-ec2-eip.html

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I had done some standing up of NATs using this code that seemed to work just fine without the depends. Perhaps I just got lucky, either way better safe than sorry.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts on whether we should pass it explicitly or implicitly?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other places where subnets are implicit? If that's an established convention, I suppose it's fine. However, it seems better to me as an explicit parameter.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Builder for EC2 currently takes an implicit subnet and we are routinely using implicit VPCs. I'm fairly neutral on which way we go though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the Subnet one makes sense as an implicit since this function is used nested within a subnet. The VPC Gateway attachment isn't in that hierarchy though, so i'd make it explicit.

adding in  depends upon VPCGatewayAttachment for NAT EIP

adding in  depends upon  for NATGateway
tjcorr pushed a commit that referenced this pull request Mar 9, 2016
Add support for AWS::EC2::NatGateway
@tjcorr tjcorr merged commit 17e3731 into master Mar 9, 2016
@tjcorr tjcorr deleted the NatGateway branch March 9, 2016 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add AWS::EC2::NatGateway and deprecate custom type
3 participants