Skip to content
This repository was archived by the owner on Oct 23, 2025. It is now read-only.

Ignore records without PhysicalResourceId when yielding CF Stack resources #365

Merged
merged 1 commit into from
Sep 28, 2016
Merged

Ignore records without PhysicalResourceId when yielding CF Stack resources #365

merged 1 commit into from
Sep 28, 2016

Conversation

@jmcs
Copy link
Member

@jmcs jmcs commented Sep 28, 2016

Fixes #359.

@coveralls
Copy link

coveralls commented Sep 28, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 89.447% when pulling 1570cc0 on no-PhysicalResourceId into fe537a4 on master.

rafaelcaricio
rafaelcaricio suggested changes Sep 28, 2016
View reviewed changes
senza/manaus/cloudformation.py
resource_type = resource["ResourceType"]
if resource_type == ResourceType.route53_record_set:
physical_resource_id = resource.get('PhysicalResourceId')
if physical_resource_id is None:
Copy link
Contributor

@rafaelcaricio rafaelcaricio Sep 28, 2016

Choose a reason for hiding this comment

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

Maybe we should just check if the status is DELETE_COMPLETE? Sounds more contextualised than we cannot check because there is no key found in the dict.

Copy link
Member Author

@jmcs jmcs Sep 28, 2016

Choose a reason for hiding this comment

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

Some resources with delete_complete have a PhysicalResourceId. From the same stack trace:

{'LastUpdatedTimestamp': datetime.datetime(..), 
 'LogicalResourceId': 'xxxLBSecurityGroup', 
 'PhysicalResourceId': 'xxx-xxx-xx-xxxBSecurityGroup-xxxxxx', 
 'ResourceStatus': 'DELETE_COMPLETE', 
 'ResourceType': 'AWS::EC2::SecurityGroup' }

Granted none of them are Route53 records but I couldn't find documentation on this.

@rafaelcaricio
Copy link
Contributor

rafaelcaricio commented Sep 28, 2016

👍

@jmcs jmcs merged commit ce45f37 into master Sep 28, 2016
@tuxlife tuxlife deleted the no-PhysicalResourceId branch October 31, 2016 14:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@rafaelcaricio rafaelcaricio rafaelcaricio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rafaelcaricio rafaelcaricio

@hjacobs hjacobs

@ainmosni ainmosni

@jkozlowicz jkozlowicz

@last-ent last-ent

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jmcs @coveralls @rafaelcaricio @hjacobs @ainmosni @jkozlowicz @last-ent @mawenzel