-
Notifications
You must be signed in to change notification settings - Fork 25
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
Proposed fix for issue #5 #6
base: master
Are you sure you want to change the base?
Conversation
nonspecialist
commented
Aug 22, 2017
- Determine whether an RDS resource is a cluster prior to snapshotting
- use the appropriate snapshot method
- missing `self` call - removed exception for flow control.
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.
Pushed updates and additional tests to cover the new code.
I'm happy with it now, hopefully @wobeng will be able to validate this works as expected once its merged.
lambda/backuplambda.py
Outdated
|
||
def is_cluster(self, resource): | ||
try: | ||
return resource['DBClusterIdentifier'] is not None |
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 do this as
return 'DBClusterIdentifier' in resource
And not need the try except.
lambda/backuplambda.py
Outdated
current_snap = self.conn.create_db_snapshot(DBInstanceIdentifier=self.resolve_backupable_id(resource), | ||
DBSnapshotIdentifier=snapshot_id, | ||
Tags=aws_tagset) | ||
if is_cluster(resource): |
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.
if self.is_cluster(resource):
Looking further into the API I think this is a bit more complex than the current approach supports. I might have to actually build a cluster and see what responses come back from AWS to make it work. |
Take a look at #8 . It has db cluster support |