Skip to content

Commit 0841eb8

Browse files
committed
Enable skipping sanity checks using --force (#18, #21)
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.
1 parent daa41a2 commit 0841eb8

File tree

4 files changed

+174
-69
lines changed

4 files changed

+174
-69
lines changed

README.rst

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,11 +206,6 @@ intended you have no right to complain ;-).
206206
that sections in user-specific configuration files override sections by the
207207
same name in system-wide configuration files. For more details refer to the
208208
online documentation."
209-
"``-u``, ``--use-sudo``","Enable the use of ""sudo"" to rotate backups in directories that are not
210-
readable and/or writable for the current user (or the user logged in to a
211-
remote system over SSH)."
212-
"``-n``, ``--dry-run``","Don't make any changes, just print what would be done. This makes it easy
213-
to evaluate the impact of a rotation scheme without losing any backups."
214209
"``-C``, ``--removal-command=CMD``","Change the command used to remove backups. The value of ``CMD`` defaults to
215210
``rm ``-f``R``. This choice was made because it works regardless of whether
216211
""backups to be rotated"" are files or directories or a mixture of both.
@@ -219,6 +214,17 @@ intended you have no right to complain ;-).
219214
represented as regular directory trees that can be deleted at once with a
220215
single 'rmdir' command (even though according to POSIX semantics this
221216
command should refuse to remove nonempty directories, but I digress)."
217+
"``-u``, ``--use-sudo``","Enable the use of ""sudo"" to rotate backups in directories that are not
218+
readable and/or writable for the current user (or the user logged in to a
219+
remote system over SSH)."
220+
"``-f``, ``--force``","If a sanity check fails an error is reported and the program aborts. You
221+
can use ``--force`` to continue with backup rotation instead. Sanity checks
222+
are done to ensure that the given DIRECTORY exists, is readable and is
223+
writable. If the ``--removal-command`` option is given then the last sanity
224+
check (that the given location is writable) is skipped (because custom
225+
removal commands imply custom semantics)."
226+
"``-n``, ``--dry-run``","Don't make any changes, just print what would be done. This makes it easy
227+
to evaluate the impact of a rotation scheme without losing any backups."
222228
"``-v``, ``--verbose``",Increase logging verbosity (can be repeated).
223229
"``-q``, ``--quiet``",Decrease logging verbosity (can be repeated).
224230
"``-h``, ``--help``",Show this message and exit.

rotate_backups/__init__.py

Lines changed: 122 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@
2727
from executor.concurrent import CommandPool
2828
from executor.contexts import RemoteContext, create_context
2929
from humanfriendly import Timer, coerce_boolean, format_path, parse_path, pluralize
30-
from humanfriendly.text import compact, concatenate, split
30+
from humanfriendly.text import concatenate, split
3131
from natsort import natsort
3232
from property_manager import (
3333
PropertyManager,
@@ -294,6 +294,28 @@ def exclude_list(self):
294294
"""
295295
return []
296296

297+
@mutable_property
298+
def force(self):
299+
"""
300+
:data:`True` to continue if sanity checks fail, :data:`False` to raise an exception.
301+
302+
Sanity checks are performed before backup rotation starts to ensure
303+
that the given location exists, is readable and is writable. If
304+
:attr:`removal_command` is customized then the last sanity check (that
305+
the given location is writable) is skipped (because custom removal
306+
commands imply custom semantics, see also `#18`_). If a sanity check
307+
fails an exception is raised, but you can set :attr:`force` to
308+
:data:`True` to continue with backup rotation instead (the default is
309+
obviously :data:`False`).
310+
311+
.. seealso:: :func:`Location.ensure_exists()`,
312+
:func:`Location.ensure_readable()` and
313+
:func:`Location.ensure_writable()`
314+
315+
.. _#18: https://github.com/xolox/python-rotate-backups/issues/18
316+
"""
317+
return False
318+
297319
@cached_property(writable=True)
298320
def include_list(self):
299321
"""
@@ -477,7 +499,7 @@ class together to implement backup rotation with an easy to use Python
477499
# https://github.com/xolox/python-rotate-backups/issues/18 for
478500
# more details about one such use case).
479501
if not self.dry_run and (self.removal_command == DEFAULT_REMOVAL_COMMAND):
480-
location.ensure_writable()
502+
location.ensure_writable(self.force)
481503
most_recent_backup = sorted_backups[-1]
482504
# Group the backups by the rotation frequencies.
483505
backups_by_frequency = self.group_backups(sorted_backups)
@@ -563,7 +585,7 @@ def collect_backups(self, location):
563585
backups = []
564586
location = coerce_location(location)
565587
logger.info("Scanning %s for backups ..", location)
566-
location.ensure_readable()
588+
location.ensure_readable(self.force)
567589
for entry in natsort(location.context.list_entries(location.directory)):
568590
match = TIMESTAMP_PATTERN.search(entry)
569591
if match:
@@ -733,49 +755,105 @@ def key_properties(self):
733755
"""
734756
return ['ssh_alias', 'directory'] if self.is_remote else ['directory']
735757

736-
def ensure_exists(self):
737-
"""Make sure the location exists."""
738-
if not self.context.is_directory(self.directory):
739-
# This can also happen when we don't have permission to one of the
740-
# parent directories so we'll point that out in the error message
741-
# when it seems applicable (so as not to confuse users).
742-
if self.context.have_superuser_privileges:
743-
msg = "The directory %s doesn't exist!"
744-
raise ValueError(msg % self)
745-
else:
746-
raise ValueError(compact("""
747-
The directory {location} isn't accessible, most likely
748-
because it doesn't exist or because of permissions. If
749-
you're sure the directory exists you can use the
750-
--use-sudo option.
751-
""", location=self))
752-
753-
def ensure_readable(self):
754-
"""Make sure the location exists and is readable."""
755-
self.ensure_exists()
756-
if not self.context.is_readable(self.directory):
757-
if self.context.have_superuser_privileges:
758-
msg = "The directory %s isn't readable!"
759-
raise ValueError(msg % self)
758+
def ensure_exists(self, override=False):
759+
"""
760+
Sanity check that the location exists.
761+
762+
:param override: :data:`True` to log a message, :data:`False` to raise
763+
an exception (when the sanity check fails).
764+
:returns: :data:`True` if the sanity check succeeds,
765+
:data:`False` if it fails (and `override` is :data:`True`).
766+
:raises: :exc:`~exceptions.ValueError` when the sanity
767+
check fails and `override` is :data:`False`.
768+
769+
.. seealso:: :func:`ensure_readable()`, :func:`ensure_writable()` and :func:`add_hints()`
770+
"""
771+
if self.context.is_directory(self.directory):
772+
logger.verbose("Confirmed that location exists: %s", self)
773+
return True
774+
elif override:
775+
logger.notice("It seems %s doesn't exist but --force was given so continuing anyway ..", self)
776+
return False
777+
else:
778+
message = "It seems %s doesn't exist or isn't accessible due to filesystem permissions!"
779+
raise ValueError(self.add_hints(message % self))
780+
781+
def ensure_readable(self, override=False):
782+
"""
783+
Sanity check that the location exists and is readable.
784+
785+
:param override: :data:`True` to log a message, :data:`False` to raise
786+
an exception (when the sanity check fails).
787+
:returns: :data:`True` if the sanity check succeeds,
788+
:data:`False` if it fails (and `override` is :data:`True`).
789+
:raises: :exc:`~exceptions.ValueError` when the sanity
790+
check fails and `override` is :data:`False`.
791+
792+
.. seealso:: :func:`ensure_exists()`, :func:`ensure_writable()` and :func:`add_hints()`
793+
"""
794+
# Only sanity check that the location is readable when its
795+
# existence has been confirmed, to avoid multiple notices
796+
# about the same underlying problem.
797+
if self.ensure_exists(override):
798+
if self.context.is_readable(self.directory):
799+
logger.verbose("Confirmed that location is readable: %s", self)
800+
return True
801+
elif override:
802+
logger.notice("It seems %s isn't readable but --force was given so continuing anyway ..", self)
760803
else:
761-
raise ValueError(compact("""
762-
The directory {location} isn't readable, most likely
763-
because of permissions. Consider using the --use-sudo
764-
option.
765-
""", location=self))
766-
767-
def ensure_writable(self):
768-
"""Make sure the directory exists and is writable."""
769-
self.ensure_exists()
770-
if not self.context.is_writable(self.directory):
771-
if self.context.have_superuser_privileges:
772-
msg = "The directory %s isn't writable!"
773-
raise ValueError(msg % self)
804+
message = "It seems %s isn't readable!"
805+
raise ValueError(self.add_hints(message % self))
806+
return False
807+
808+
def ensure_writable(self, override=False):
809+
"""
810+
Sanity check that the directory exists and is writable.
811+
812+
:param override: :data:`True` to log a message, :data:`False` to raise
813+
an exception (when the sanity check fails).
814+
:returns: :data:`True` if the sanity check succeeds,
815+
:data:`False` if it fails (and `override` is :data:`True`).
816+
:raises: :exc:`~exceptions.ValueError` when the sanity
817+
check fails and `override` is :data:`False`.
818+
819+
.. seealso:: :func:`ensure_exists()`, :func:`ensure_readable()` and :func:`add_hints()`
820+
"""
821+
# Only sanity check that the location is readable when its
822+
# existence has been confirmed, to avoid multiple notices
823+
# about the same underlying problem.
824+
if self.ensure_exists(override):
825+
if self.context.is_writable(self.directory):
826+
logger.verbose("Confirmed that location is writable: %s", self)
827+
return True
828+
elif override:
829+
logger.notice("It seems %s isn't writable but --force was given so continuing anyway ..", self)
774830
else:
775-
raise ValueError(compact("""
776-
The directory {location} isn't writable, most likely due
777-
to permissions. Consider using the --use-sudo option.
778-
""", location=self))
831+
message = "It seems %s isn't writable!"
832+
raise ValueError(self.add_hints(message % self))
833+
return False
834+
835+
def add_hints(self, message):
836+
"""
837+
Provide hints about failing sanity checks.
838+
839+
:param message: The message to the user (a string).
840+
:returns: The message including hints (a string).
841+
842+
When superuser privileges aren't being used a hint about the
843+
``--use-sudo`` option will be added (in case a sanity check failed
844+
because we don't have permission to one of the parent directories).
845+
846+
In all cases a hint about the ``--force`` option is added (in case the
847+
sanity checks themselves are considered the problem, which is obviously
848+
up to the operator to decide).
849+
850+
.. seealso:: :func:`ensure_exists()`, :func:`ensure_readable()` and :func:`ensure_writable()`
851+
"""
852+
sentences = [message]
853+
if not self.context.have_superuser_privileges:
854+
sentences.append("If filesystem permissions are the problem consider using the --use-sudo option.")
855+
sentences.append("To continue despite this failing sanity check you can use --force.")
856+
return " ".join(sentences)
779857

780858
def match(self, location):
781859
"""

rotate_backups/cli.py

Lines changed: 30 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# rotate-backups: Simple command line interface for backup rotation.
22
#
33
# Author: Peter Odding <peter@peterodding.com>
4-
# Last Change: August 2, 2018
4+
# Last Change: February 12, 2020
55
# URL: https://github.com/xolox/python-rotate-backups
66

77
"""
@@ -143,17 +143,6 @@
143143
same name in system-wide configuration files. For more details refer to the
144144
online documentation.
145145
146-
-u, --use-sudo
147-
148-
Enable the use of `sudo' to rotate backups in directories that are not
149-
readable and/or writable for the current user (or the user logged in to a
150-
remote system over SSH).
151-
152-
-n, --dry-run
153-
154-
Don't make any changes, just print what would be done. This makes it easy
155-
to evaluate the impact of a rotation scheme without losing any backups.
156-
157146
-C, --removal-command=CMD
158147
159148
Change the command used to remove backups. The value of CMD defaults to
@@ -165,6 +154,26 @@
165154
single 'rmdir' command (even though according to POSIX semantics this
166155
command should refuse to remove nonempty directories, but I digress).
167156
157+
-u, --use-sudo
158+
159+
Enable the use of `sudo' to rotate backups in directories that are not
160+
readable and/or writable for the current user (or the user logged in to a
161+
remote system over SSH).
162+
163+
-f, --force
164+
165+
If a sanity check fails an error is reported and the program aborts. You
166+
can use --force to continue with backup rotation instead. Sanity checks
167+
are done to ensure that the given DIRECTORY exists, is readable and is
168+
writable. If the --removal-command option is given then the last sanity
169+
check (that the given location is writable) is skipped (because custom
170+
removal commands imply custom semantics).
171+
172+
-n, --dry-run
173+
174+
Don't make any changes, just print what would be done. This makes it easy
175+
to evaluate the impact of a rotation scheme without losing any backups.
176+
168177
-v, --verbose
169178
170179
Increase logging verbosity (can be repeated).
@@ -214,11 +223,11 @@ def main():
214223
selected_locations = []
215224
# Parse the command line arguments.
216225
try:
217-
options, arguments = getopt.getopt(sys.argv[1:], 'M:H:d:w:m:y:I:x:jpri:c:r:uC:nvqh', [
226+
options, arguments = getopt.getopt(sys.argv[1:], 'M:H:d:w:m:y:I:x:jpri:c:r:uC:fnvqh', [
218227
'minutely=', 'hourly=', 'daily=', 'weekly=', 'monthly=', 'yearly=',
219228
'include=', 'exclude=', 'parallel', 'prefer-recent', 'relaxed',
220-
'ionice=', 'config=', 'use-sudo', 'dry-run', 'removal-command=',
221-
'verbose', 'quiet', 'help',
229+
'ionice=', 'config=', 'removal-command=', 'use-sudo', 'force',
230+
'dry-run', 'verbose', 'quiet', 'help',
222231
])
223232
for option, value in options:
224233
if option in ('-M', '--minutely'):
@@ -248,15 +257,17 @@ def main():
248257
kw['io_scheduling_class'] = value
249258
elif option in ('-c', '--config'):
250259
kw['config_file'] = parse_path(value)
260+
elif option in ('-C', '--removal-command'):
261+
removal_command = shlex.split(value)
262+
logger.info("Using custom removal command: %s", removal_command)
263+
kw['removal_command'] = removal_command
251264
elif option in ('-u', '--use-sudo'):
252265
use_sudo = True
266+
elif option in ('-f', '--force'):
267+
kw['force'] = True
253268
elif option in ('-n', '--dry-run'):
254269
logger.info("Performing a dry run (because of %s option) ..", option)
255270
kw['dry_run'] = True
256-
elif option in ('-C', '--removal-command'):
257-
removal_command = shlex.split(value)
258-
logger.info("Using custom removal command: %s", removal_command)
259-
kw['removal_command'] = removal_command
260271
elif option in ('-v', '--verbose'):
261272
coloredlogs.increase_verbosity()
262273
elif option in ('-q', '--quiet'):

rotate_backups/tests.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
# Test suite for the `rotate-backups' Python package.
22
#
33
# Author: Peter Odding <peter@peterodding.com>
4-
# Last Change: February 11, 2020
4+
# Last Change: February 12, 2020
55
# URL: https://github.com/xolox/python-rotate-backups
66

77
"""Test suite for the `rotate-backups` package."""
@@ -13,6 +13,7 @@
1313
import os
1414

1515
# External dependencies.
16+
from executor import ExternalCommandFailed
1617
from executor.contexts import RemoteContext
1718
from humanfriendly.testing import TemporaryDirectory, TestCase, run_cli, touch
1819
from six.moves import configparser
@@ -393,6 +394,15 @@ def test_removal_command(self):
393394
commands = program.rotate_backups(root, prepare=True)
394395
assert any(cmd.command_line[0] == 'rmdir' for cmd in commands)
395396

397+
def test_force(self):
398+
"""Test that sanity checks can be overridden."""
399+
with TemporaryDirectory(prefix='rotate-backups-', suffix='-test-suite') as root:
400+
for date in '2019-03-05', '2019-03-06':
401+
os.mkdir(os.path.join(root, date))
402+
with readonly_directory(root):
403+
program = RotateBackups(force=True, rotation_scheme=dict(monthly='always'))
404+
self.assertRaises(ExternalCommandFailed, program.rotate_backups, root)
405+
396406
def test_ensure_writable(self):
397407
"""Test that ensure_writable() complains when the location isn't writable."""
398408
with TemporaryDirectory(prefix='rotate-backups-', suffix='-test-suite') as root:

0 commit comments

Comments
 (0)