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

Only delete snapshot if it exists. #16826

Merged
merged 2 commits into from Aug 4, 2017
Merged

Only delete snapshot if it exists. #16826

merged 2 commits into from Aug 4, 2017

Conversation

ashercodeorg
Copy link
Contributor

@ashercodeorg ashercodeorg commented Aug 4, 2017

Seeing undefined method 'delete' for nil:NilClass (NoMethodError) in the wild (Honeybadger).

@ashercodeorg
Copy link
Contributor Author

ashercodeorg commented Aug 4, 2017

Is this an appropriate fix? Note that I don't completely understand why it might be nil.

Given that this change seems not to hurt anything (e.g., if something wasn't being deleted, it still isn't being deleted), the only way this PR could seemingly be bad is if it hid something action needed to be taken on.

Copy link
Contributor

@ewjordan ewjordan left a comment

Choose a reason for hiding this comment

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

I think that's reasonable - it might make sense to also add an error message to the chat room to let us know something went wrong? Pretty much any AWS call can theoretically fail, that must be what's happening if this is throwing.

@ashercodeorg
Copy link
Contributor Author

Added a message to Slack#cron-daily. Nice idea!

@ashercodeorg ashercodeorg merged commit 6591ef6 into staging Aug 4, 2017
@ashercodeorg ashercodeorg deleted the trySnapshotDelete branch August 4, 2017 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants