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

ensure_writable limits the applicability of this awesome script and should be removed #18

Closed
btittelbach opened this issue Jul 2, 2019 · 2 comments

Comments

@btittelbach
Copy link

btittelbach commented Jul 2, 2019

First, let me thank you for creating this nice script, which does exactly what I was looking for and so saved me from having to write something less fancy myself.

However, in order to make it work the way I needed it, I had to remove the ensure_writable() check.

I'm proposing that you might consider removing this check altogether and instead display a meaningful error message when the delete command actually fails and not before.
This script is, after all, intended for admins who should know what they are doing.
The check fails under certain conditions when the actual removal command would have performed perfectly fine.

In my case it wanted to give a --removal-command like e.g. sudo rm <very specific directorytree> or e.g. sudo zfs destroy <only certain sapshots> and NOT run the script using sudo or give the --use-sudo option, causing test or find to run as root, as there is no need for that. (It is good practice to limit sudo to one very specific command for that specific user running rotate-backups.)

This all would have worked perfectly out of the box (it does now), except for the overzealous ensure_writable() check at the beginning, which, though well intended, is actually not helpful and prohibits using the script in a safe manner.

xolox added a commit that referenced this issue Feb 11, 2020
The mentioned issue / pull request made it clear to me that its
very well possible for these (well intentioned) sanity checks
to fail. Nevertheless I'm not inclined to just remove them.
I do believe the changes here and in daa41a2 suitably
accommodate the use case described in issue 18.
@xolox
Copy link
Owner

xolox commented Feb 12, 2020

Hey Bernhard and thanks for the feedback!

Your use case made me realize that ensure_writable() should be skipped when --removal-command is given because custom removal commands imply custom semantics (as your use case shows).

Furthermore this made me realize that these sanity checks can misjudge a situation in such a way that they prevent rotate-backups from being used, which is obviously bad. To this end I've added a new option --force that will make the program continue even if sanity checks failed.

Both of these changes are part of rotate-backups 7.0 which was just released. I hope and believe these changes adequately resolve your issue.

I'm going to close this issue now, but if you notice anything wrong feel free to reopen this issue or open a new issue.

@xolox xolox closed this as completed Feb 12, 2020
@btittelbach
Copy link
Author

sound great :-)
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

No branches or pull requests

2 participants