-
Notifications
You must be signed in to change notification settings - Fork 9
Conversation
5952980
to
ba6ecdc
Compare
|
looks good to me |
Visualising dependency graph
|
@@ -525,6 +526,8 @@ def rds(self, template): | |||
], | |||
VpcId=Ref("VPC"), | |||
GroupDescription="SG for EC2 Access to RDS", | |||
# translates to DependsOn="AttachGateway", | |||
DependsOn=[r.title for r in self._find_resources(template, "AWS::EC2::VPCGatewayAttachment")], |
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 line is a little long, and also the call to _find_resources
is repeated further down. I'd prefer to see this extracted out to a separate variable such that it can be split into two lines, and be available for re-use later on:
resources = self._find_resources(template, "AWS::EC2::VPCGatewayAttachment")
...
DependsOn=[r.title for r in resources],
...
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.
Both of these calls use the template
object as an argument, which is passed when each template gets populated with resources above eg:
def elb(self, template):
and
def rds(self, template):
and during construction we create and add these resources, but it is not mandatory that the resources would always exist. During self.process() they exist because of this:
def process(self):
template = self.base_template()
vpc = self.vpc()
map(template.add_resource, vpc)
iam = self.iam()
map(template.add_resource, iam)
ec2 = self.ec2()
map(template.add_resource, ec2)
...but for example during
https://github.com/ministryofjustice/bootstrap-cfn/blob/fils-dependency-implementation/tests/tests.py#L292
..this no longer applies because no vpc has been populated (as it has been populated here: https://github.com/ministryofjustice/bootstrap-cfn/blob/fils-dependency-implementation/tests/tests.py#L376-L389 ).
We could make this a more generic function (and I was looking for one, which is how I found _find_resources()
) if you think it's clearer but it felt redundant to me. Or I can just split this to multiple lines as before.
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 've made it like this in another commit (unpushed)
DependsOn=[
r.title
for r in self._find_resources(
template, "AWS::EC2::VPCGatewayAttachment")],
Alternatively we can do
@classmethod
def _filter_list_by_attr(cls, input_list, attr_name, attr_value):
filtered = (r for r in input_list
if hasattr(r, attr_name) and
getattr(r, attr_name) == attr_value)
return filtered
though we would only benefit from that for the ScalingGroup ...
Another approach would be to do this via a different function, in the same fashion we use _attach_elbs
, that way we can find the resources near the end and update the respective DependsOn attributes. This third method is probably the cleanest way but I 'd rather we get this in and refactor later.
a5a5e14
to
a745224
Compare
This implementation puts the dependencies in this order: <almost everything> ScalingGroup -> RDSInstance (this takes the longest) -> DB Sec Group -> VPC attachment -> InternetGateway -> VPC ``` DELETE_COMPLETE AWS::IAM::InstanceProfile InstanceProfile-DELETE_COMPLETE-2016-06-21T23:29:31.854Z DELETE_IN_PROGRESS AWS::IAM::Role BaseHostRole-DELETE_IN_PROGRESS-2016-06-21T23:29:34.051Z DELETE_COMPLETE AWS::IAM::Role BaseHostRole-DELETE_COMPLETE-2016-06-21T23:29:34.661Z DELETE_COMPLETE AWS::EC2::SecurityGroup elb-DELETE_COMPLETE-2016-06-21T23:30:55.163Z DELETE_COMPLETE AWS::RDS::DBInstance RDSInstance-DELETE_COMPLETE-2016-06-21T23:31:12.548Z DELETE_IN_PROGRESS AWS::RDS::DBSubnetGroup RDSSubnetGroup-DELETE_IN_PROGRESS-2016-06-21T23:31:14.500Z DELETE_COMPLETE AWS::RDS::DBSubnetGroup RDSSubnetGroup-DELETE_COMPLETE-2016-06-21T23:31:15.324Z DELETE_IN_PROGRESS AWS::EC2::Subnet SubnetC-DELETE_IN_PROGRESS-2016-06-21T23:31:17.022Z DELETE_IN_PROGRESS AWS::EC2::Subnet SubnetB-DELETE_IN_PROGRESS-2016-06-21T23:31:17.103Z DELETE_IN_PROGRESS AWS::EC2::SecurityGroup DatabaseSG-DELETE_IN_PROGRESS-2016-06-21T23:31:17.224Z DELETE_COMPLETE AWS::EC2::SecurityGroup DatabaseSG-DELETE_COMPLETE-2016-06-21T23:31:18.963Z DELETE_IN_PROGRESS AWS::EC2::VPCGatewayAttachment AttachGateway-DELETE_IN_PROGRESS-2016-06-21T23:31:21.458Z DELETE_IN_PROGRESS AWS::EC2::Subnet SubnetA-DELETE_IN_PROGRESS-2016-06-21T23:31:27.522Z DELETE_COMPLETE AWS::EC2::Subnet SubnetC-DELETE_COMPLETE-2016-06-21T23:31:32.859Z DELETE_COMPLETE AWS::EC2::Subnet SubnetB-DELETE_COMPLETE-2016-06-21T23:31:33.030Z DELETE_COMPLETE AWS::EC2::VPCGatewayAttachment AttachGateway-DELETE_COMPLETE-2016-06-21T23:31:37.421Z DELETE_IN_PROGRESS AWS::EC2::InternetGateway InternetGateway-DELETE_IN_PROGRESS-2016-06-21T23:31:39.451Z DELETE_COMPLETE AWS::EC2::Subnet SubnetA-DELETE_COMPLETE-2016-06-21T23:31:43.439Z DELETE_COMPLETE AWS::EC2::InternetGateway InternetGateway-DELETE_COMPLETE-2016-06-21T23:31:55.217Z DELETE_IN_PROGRESS AWS::EC2::VPC VPC-DELETE_IN_PROGRESS-2016-06-21T23:31:57.098Z Stack successfully deleted ``` It also splits the test_rds test into two: * *test_rds* which tests RDS with a basic VPC (no vpc template resources) * *test_rds_with_vpc_dependencies* tests RDS with a VPC Gateway attachment
If the ELB is internet facing, it requires a VPCGatewayAttachment so that it can retrieve a public IP from the Internet Gateway. This change adds a DependsOn dependency for every ELB with scheme internet-facing so that when the stack gets deleted the ELB gets deleted before the VPCGatewayAttachment resources.
a745224
to
c529041
Compare
Tested on ci-deploy: works a treat :) |
This implementation puts the dependencies in this order:
ScalingGroup -> RDSInstance (this takes the longest)
-> DB Sec Group -> VPC attachment -> InternetGateway -> VPC