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

Run post-hooks even if backup actions fail #514

Merged
merged 1 commit into from
May 23, 2018

Conversation

carlpett
Copy link
Contributor

Fixes #511

This pulls all the action execution out into a new function (should there be matching changes in unit tests for this?), then ensures post-action hooks are run regardless of errors in the actions, and then returns any errors (including any in the post hook)

@carlpett carlpett force-pushed the post-hooks-after-error branch from 9919150 to b66b90c Compare May 23, 2018 16:17
@ncdc
Copy link
Contributor

ncdc commented May 23, 2018

Thanks @carlpett! For the time being, I think it's ok to leave the unit tests as they're currently structured. We may split backupItem() into multiple helper functions and then unit test them individually, as I wrote about here, but we haven't decided on that just yet.

@ncdc ncdc added this to the v0.9.0 milestone May 23, 2018
@ncdc ncdc removed the Enhancement label May 23, 2018
@ncdc ncdc requested a review from skriss May 23, 2018 16:23
@ncdc ncdc assigned ncdc and skriss May 23, 2018
@ncdc ncdc self-requested a review May 23, 2018 16:23
Copy link
Contributor

@ncdc ncdc left a comment

Choose a reason for hiding this comment

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

This was mostly a cut & paste, right?

backupErrs := make([]error, 0)
err = ib.executeActions(log, obj, groupResource, name, namespace, metadata)
if err != nil {
log.WithError(err).Error("Error executing backup actions")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd change "backup" to "item"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@carlpett carlpett force-pushed the post-hooks-after-error branch from b66b90c to b2ec87f Compare May 23, 2018 16:45
@carlpett
Copy link
Contributor Author

This was mostly a cut & paste, right?
Yes.

Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM.

@ncdc
Copy link
Contributor

ncdc commented May 23, 2018

LGTM

@ncdc ncdc merged commit 6dbde59 into vmware-tanzu:master May 23, 2018
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.

3 participants