Exit with non-zero if any job failed or was skipped

Currently any errors coming the underlying tarsnap commands are logged. This is
not threaded back to the entrypoint of the application though, so we end up
exiting with a zero code despite one or more of the jobs having failed. This
makes monitoring the status of tarsnapper impossible. One would have to know to
regularly check its logs to ensure it's actually backing things up.

Fixes miracle2k#39

tarsnapper/script.py

@@ -262,6 +262,27 @@ def timedelta_string(value):
     except ValueError as e:
         raise argparse.ArgumentTypeError('invalid delta value: %r (suffix d, s allowed)' % e)
 
+class Result(object):
+    def __init__(self, success, error=None):
+        if success != (error is None):
+            raise ValueError("If success is True, error must be None")
+        self.success = success
+        self.error = error
+
+    def __str__(self):
+        if self.success:
+            return "Success"
+        else:
+            return "Error: %s" % self.error
+
+    @classmethod
+    def Failure(self, error=''):
+        return Result(success=False, error=error)
+
+    @classmethod
+    def Success(self):
+        return Result(success=True)
+
 
 class Command(object):
 
@@ -300,6 +321,7 @@ def run(self, job):
         backups = sorted(unsorted_backups, key=lambda x: x[1], reverse=True)
         for backup, _ in backups:
             print("  %s" % backup)
+        return Result.Success()
 
 
 class ExpireCommand(Command):
@@ -316,12 +338,13 @@ def setup_arg_parser(self, parser):
     def expire(self, job):
         if not job.deltas:
             self.log.info("Skipping '%s', does not define deltas", job.name)
-            return
+            return Result.Failure("Skipped %s" % job.name)
 
         self.backend.expire(job)
+        return Result.Success()
 
     def run(self, job):
-        self.expire(job)
+        return self.expire(job)
 
 
 class MakeCommand(ExpireCommand):
@@ -357,7 +380,7 @@ def validate_args(self, args):
     def run(self, job):
         if not job.sources:
             self.log.info("Skipping '%s', does not define sources", job.name)
-            return
+            return Result.Failure("Skipped '%s'" % job.name)
 
         if job.exec_before:
             self.backend._exec_util(job.exec_before)
@@ -390,6 +413,7 @@ def run(self, job):
             try:
                 self.backend.make(job)
             except Exception:
+                skipped = True
                 self.log.exception("Something went wrong with backup job: '%s'",
                     job.name)
 
@@ -401,6 +425,11 @@ def run(self, job):
         if not skipped and not self.args.no_expire:
             self.expire(job)
 
+        if skipped:
+            return Result.Failure("Skipped '%s'" % job.name)
+        else:
+            return Result.Success()
+
 
 COMMANDS = {
     'make': MakeCommand,
@@ -541,16 +570,22 @@ def main(argv):
         jobs_to_run = jobs
 
     command = args.command(args, log)
+    results = []
     try:
         for job in jobs_to_run.values():
-            command.run(job)
+            results.append(command.run(job))
 
         for plugin in PLUGINS:
             plugin.all_jobs_done(args, global_config, args.command)
     except TarsnapError:
         log.exception("tarsnap execution failed:")
         return 1
 
+    if all([result.success for result in results]):
+        return 0
+    else:
+        return 1
+
 
 def run():
     sys.exit(main(sys.argv[1:]) or 0)

tests/test_script.py

@@ -11,6 +11,13 @@
     TarsnapBackend, MakeCommand, ListCommand, ExpireCommand, parse_args,
     DEFAULT_DATEFORMAT)
 
+def assertAllSucceeded(results):
+    for result in results:
+        assert result.success
+
+def assertAllFailed(results):
+    for result in results:
+        assert not result.success
 
 class FakeBackend(TarsnapBackend):
 
@@ -66,9 +73,10 @@ def run(self, jobs, archives, **args):
         cmd = self.command_class(argparse.Namespace(**final_args),
                                  self.log, backend_class=FakeBackend)
         cmd.backend.fake_archives = archives
+        results = []
         for job in (jobs if isinstance(jobs, list) else [jobs]):
-            cmd.run(job)
-        return cmd
+            results.append(cmd.run(job))
+        return (cmd, results)
 
     def job(self, deltas='1d 2d', name='test', **kwargs):
         """Make a job object.
@@ -98,19 +106,23 @@ def tset_parse(self):
 
     def test_pass_along(self):
         # Short option
-        cmd = self.run(self.job(), [], tarsnap_options=(('o', '1'),))
+        (cmd, results) = self.run(self.job(), [], tarsnap_options=(('o', '1'),))
+        assertAllSucceeded(results)
         assert cmd.backend.match([('-o', '1', '--list-archives')])
 
         # Long option
-        cmd = self.run(self.job(), [], tarsnap_options=(('foo', '1'),))
+        (cmd, results) = self.run(self.job(), [], tarsnap_options=(('foo', '1'),))
+        assertAllSucceeded(results)
         assert cmd.backend.match([('--foo', '1', '--list-archives')])
 
         # No value
-        cmd = self.run(self.job(), [], tarsnap_options=(('foo',),))
+        (cmd, results) = self.run(self.job(), [], tarsnap_options=(('foo',),))
+        assertAllSucceeded(results)
         assert cmd.backend.match([('--foo', '--list-archives')])
 
         # Multiple values
-        cmd = self.run(self.job(), [], tarsnap_options=(('foo', '1', '2'),))
+        (cmd, results) = self.run(self.job(), [], tarsnap_options=(('foo', '1', '2'),))
+        assertAllSucceeded(results)
         assert cmd.backend.match([('--foo', '1', '2', '--list-archives')])
 
 
@@ -119,35 +131,40 @@ class TestMake(BaseTest):
     command_class = MakeCommand
 
     def test(self):
-        cmd = self.run(self.job(), [])
+        (cmd, results) = self.run(self.job(), [])
+        assertAllSucceeded(results)
         assert cmd.backend.match([
             ('-c', '-f', 'test-.*', '.*'),
             ('--list-archives',)
         ])
 
     def test_no_sources(self):
         """If no sources are defined, the job is skipped."""
-        cmd = self.run(self.job(sources=None), [])
+        (cmd, results) = self.run(self.job(sources=None), [])
+        assertAllFailed(results)
         assert cmd.backend.match([])
 
     def test_excludes(self):
-        cmd = self.run(self.job(excludes=['foo']), [])
+        (cmd, results) = self.run(self.job(excludes=['foo']), [])
+        assertAllSucceeded(results)
         assert cmd.backend.match([
             ('-c', '--exclude', 'foo', '-f', 'test-.*', '.*'),
             ('--list-archives',)
         ])
 
     def test_no_expire(self):
-        cmd = self.run(self.job(), [], no_expire=True)
+        (cmd, results) = self.run(self.job(), [], no_expire=True)
+        assertAllSucceeded(results)
         assert cmd.backend.match([
             ('-c', '-f', 'test-.*', '.*'),
         ])
 
     def test_exec(self):
         """Test ``exec_before`` and ``exec_after`` options.
         """
-        cmd = self.run(self.job(exec_before="echo begin", exec_after="echo end"),
+        (cmd, results) = self.run(self.job(exec_before="echo begin", exec_after="echo end"),
                        [], no_expire=True)
+        assertAllSucceeded(results)
         assert cmd.backend.match([
             ('echo begin'),
             ('-c', '-f', 'test-.*', '.*'),
@@ -160,24 +177,26 @@ class TestExpire(BaseTest):
     command_class = ExpireCommand
 
     def test_nothing_to_do(self):
-        cmd = self.run(self.job(deltas='1d 10d'), [
+        (cmd, results) = self.run(self.job(deltas='1d 10d'), [
             self.filename('1d'),
             self.filename('5d'),
         ])
+        assertAllSucceeded(results)
         assert cmd.backend.match([
             ('--list-archives',)
         ])
 
     def test_no_deltas(self):
         """If a job does not define deltas, we skip it."""
-        cmd = self.run(self.job(deltas=None), [
+        (cmd, results) = self.run(self.job(deltas=None), [
             self.filename('1d'),
             self.filename('5d'),
         ])
+        assertAllFailed(results)
         assert cmd.backend.match([])
 
     def test_something_to_expire(self):
-        cmd = self.run(self.job(deltas='1d 2d'), [
+        (cmd, results) = self.run(self.job(deltas='1d 2d'), [
             self.filename('1d'),
             self.filename('5d'),
         ])
@@ -187,10 +206,11 @@ def test_something_to_expire(self):
         ])
 
     def test_aliases(self):
-        cmd = self.run(self.job(deltas='1d 2d', aliases=['alias']), [
+        (cmd, results) = self.run(self.job(deltas='1d 2d', aliases=['alias']), [
             self.filename('1d'),
             self.filename('5d', name='alias'),
         ])
+        assertAllSucceeded(results)
         assert cmd.backend.match([
             ('--list-archives',),
             ('-d', '-f', 'alias-.*'),
@@ -201,22 +221,24 @@ def test_date_name_mismatch(self):
         we won't stumble over "home-dev-$date". This can be an issue
         due to the way we try to parse the dates in filenames.
         """
-        cmd = self.run(self.job(name="home"), [
+        (cmd, results) = self.run(self.job(name="home"), [
             self.filename('1d', name="home-dev"),
         ])
+        assertAllSucceeded(results)
 
 
 class TestList(BaseTest):
 
     command_class = ListCommand
 
     def test(self):
-        cmd = self.run([self.job(), self.job(name='foo')], [
+        (cmd, results) = self.run([self.job(), self.job(name='foo')], [
             self.filename('1d'),
             self.filename('5d'),
             self.filename('1d', name='foo'),
             self.filename('1d', name='something-else'),
         ])
+        assertAllSucceeded(results)
         # We ask to list two jobs, but only one --list-archives call is
         # necessary.
         assert cmd.backend.match([