Skip to content

Conversation

@dodgex
Copy link
Contributor

@dodgex dodgex commented Aug 23, 2025

I just found, that my backups did not notify me of a failure (POST_COMMANDS_FAILURE) as the PRE_COMMANDS script was the one failing. Luckily the part failing was not crucial for the actual data to be backed up but still bad enough.

This PR is an attempt, to avoid that issue for the future. It adds PRE_COMMANDS_FAILURE and ABORT_ON_PRE_COMMANDS_FAILURE.

  • PRE_COMMANDS_FAILURE allows to define commands to execute in case of an non-zero exit of PRE_COMMANDS. To Achieve that, I modified the run_commands helper, to return a non-zero exit code, when any of the pre commands fails.
  • ABORT_ON_PRE_COMMANDS_FAILURE can be set to "true" to fail (exit 1) the whole backup when the pre commands failed.

I added this to backup as well as prune and check.

I opted against re-using POST_COMMANDS_FAILURE, to avoid unexpected behaviour for users upgrading.

Fixes #91

@dodgex dodgex force-pushed the feature/pre-command-failure branch from ee29de6 to 51405b1 Compare August 23, 2025 11:09
@dodgex
Copy link
Contributor Author

dodgex commented Aug 23, 2025

@djmaze I hope you find some time to review this PR. I am not exactly sure, if this solution is the best one.

Also, I wonder if it would make sense, to move the rm -f /run/lock/backup.lock to the run_exit_commands? That function should always be called when the script exits due to the trap and so avoid unwanted remaining locks?

@markus-kuepper
Copy link

Handling a Pre Command failure handling sounds like a good feature, we were even not aware that backup is not executed on Pre Command failure, as it was not specified in the docs...

Copy link
Owner

@djmaze djmaze left a comment

Choose a reason for hiding this comment

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

Sorry for the late reply. This looks good, thanks! I think we can do it like this.

It needs two additional tests though. Are you willing to do that? Otherwise I can do it later.

@dodgex
Copy link
Contributor Author

dodgex commented Dec 8, 2025

Sorry for the late reply. This looks good, thanks! I think we can do it like this.

It needs two additional tests though. Are you willing to do that? Otherwise I can do it later.

Sorry for the late response too, i saw your comment but it was a busy day and i forgot to reply...

What tests exactly do you mean? I can't find anything test related stuff? Nevermind, I just realized the spec folder is the tests. But I have to Idea how to run those tests...

So I would be happy if you could add the tests. The branch on my fork is open for maintainer to commit.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

POST_COMMANDS_FAILURE are not executed when PRE_COMMANDS fails

3 participants