From 3b57b263c437148f140bf98f474b08fbd2a535ed Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Sat, 20 Jul 2019 11:36:37 +0100 Subject: [PATCH 1/5] Add exit value from run_job --- django_crontab/crontab.py | 7 +++++-- tests/test_crontab.py | 18 +++++++++--------- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/django_crontab/crontab.py b/django_crontab/crontab.py index af0db39..09b5fb3 100755 --- a/django_crontab/crontab.py +++ b/django_crontab/crontab.py @@ -163,7 +163,7 @@ def run_job(self, job_hash): fcntl.flock(lock_file, fcntl.LOCK_EX | fcntl.LOCK_NB) except IOError: logger.warning('Tried to start cron job %s that is already running.', job) - return + return False # parse the module and function names from the job module_path, function_name = job_name.rsplit('.', 1) @@ -175,6 +175,7 @@ def run_job(self, job_hash): func(*job_args, **job_kwargs) except: logger.exception('Failed to complete cronjob at %s', job) + return False # if the LOCK_JOBS option is specified in settings if self.settings.LOCK_JOBS: @@ -183,7 +184,9 @@ def run_job(self, job_hash): fcntl.flock(lock_file, fcntl.LOCK_UN) except IOError: logger.exception('Error unlocking %s', lock_file_name) - return + return False + + return True def __hash_job(self, job): """ diff --git a/tests/test_crontab.py b/tests/test_crontab.py index 8566914..6ce4c12 100644 --- a/tests/test_crontab.py +++ b/tests/test_crontab.py @@ -169,7 +169,7 @@ def test_remove_job_but_keep_anything_else(): @patch('tests.cron.cron_job') def test_run_no_arg_format1_job(method_to_call): crontab = Crontab() - crontab.run_job('4f30993ab69a8c5763ce55f762ef0433') + assert crontab.run_job('4f30993ab69a8c5763ce55f762ef0433') method_to_call.assert_called_with() @@ -177,7 +177,7 @@ def test_run_no_arg_format1_job(method_to_call): @patch('tests.cron.cron_job') def test_run_no_arg_format1_job_with_suffix(method_to_call): crontab = Crontab() - crontab.run_job('53e6fe5b66bd870e396d574ba1503c33') + assert crontab.run_job('53e6fe5b66bd870e396d574ba1503c33') method_to_call.assert_called_with() @@ -185,7 +185,7 @@ def test_run_no_arg_format1_job_with_suffix(method_to_call): @patch('tests.cron.cron_job') def test_run_args_only_format2_job(method_to_call): crontab = Crontab() - crontab.run_job('369f8418b0f8cf1fff78c547516aa8e0') + assert crontab.run_job('369f8418b0f8cf1fff78c547516aa8e0') method_to_call.assert_called_with(1, 'two') @@ -193,22 +193,22 @@ def test_run_args_only_format2_job(method_to_call): @patch('tests.cron.cron_job') def test_run_format2_job(method_to_call): crontab = Crontab() - crontab.run_job('13e8169dffe273b8b0c5f8abe1b6f643') + assert crontab.run_job('13e8169dffe273b8b0c5f8abe1b6f643') method_to_call.assert_called_with(1, 'two', test=34, a='s2') @override_settings(CRONJOBS=[('*/1 * * * *', 'tests.cron.cron_job', [1, 'two'], dict(), 'some suffix')]) @patch('tests.cron.cron_job') -def test_run_args_only_format2_job(method_to_call): +def test_run_args_only_format3_job(method_to_call): crontab = Crontab() - crontab.run_job('fefa68aed4ff509331ee6a5b62ea5e5c') + assert crontab.run_job('fefa68aed4ff509331ee6a5b62ea5e5c') method_to_call.assert_called_with(1, 'two') @override_settings(CRONJOBS=[('* * * * *', 'tests.test_crontab.recursive_job_for_lock_checking')], CRONTAB_LOCK_JOBS=True) def test_locked_job(): crontab = Crontab() - crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b') + assert crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b') def recursive_job_for_lock_checking(): @@ -217,12 +217,12 @@ def recursive_job_for_lock_checking(): else: recursive_job_for_lock_checking.called_already = True crontab = Crontab() - crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b') + assert not crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b') recursive_job_for_lock_checking.called_already = False @raises(RuntimeError) def test_job_not_found(): crontab = Crontab() - crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b') + assert not crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b') From 2820eec08664a444b874922944784a7792ac16f9 Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Sat, 20 Jul 2019 11:45:17 +0100 Subject: [PATCH 2/5] Make command exit with non-zero code on run_job failure --- django_crontab/management/commands/crontab.py | 5 ++++- tests/test_command.py | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/django_crontab/management/commands/crontab.py b/django_crontab/management/commands/crontab.py index 8c1b798..3c655c2 100755 --- a/django_crontab/management/commands/crontab.py +++ b/django_crontab/management/commands/crontab.py @@ -3,6 +3,7 @@ from django.core.management.base import BaseCommand from django_crontab.crontab import Crontab +import sys class Command(BaseCommand): """ @@ -32,7 +33,9 @@ def handle(self, *args, **options): with Crontab(**options) as crontab: # initialize a Crontab class with any specified options crontab.remove_jobs() # remove all jobs specified in settings from the crontab elif options['subcommand'] == 'run': # run command - Crontab().run_job(options['jobhash']) # run the job with the specified hash + result = Crontab().run_job(options['jobhash']) # run the job with the specified hash + if not result: + sys.exit(1) else: # output the help string if the user entered something not specified above print(self.help) diff --git a/tests/test_command.py b/tests/test_command.py index 4addf24..8132ee3 100644 --- a/tests/test_command.py +++ b/tests/test_command.py @@ -68,3 +68,12 @@ def test_help_command(show_mock, run_mock, remove_mock, add_mock): assert not run_mock.called assert not show_mock.called +@patch.object(Crontab, 'add_jobs') +@patch.object(Crontab, 'remove_jobs') +@patch.object(Crontab, 'run_job', return_value=False) +def test_run_command_with_error(run_mock, remove_mock, add_mock): + with assert_raises(SystemExit): + call_command('crontab', 'run', 'abc123') + assert not remove_mock.called + assert not add_mock.called + run_mock.assert_called_once_with('abc123') From ede49481a41d65039857c20709301cea3d93be88 Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Sat, 20 Jul 2019 11:45:37 +0100 Subject: [PATCH 3/5] Add test that run_job returns false on Exception --- tests/test_crontab.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/test_crontab.py b/tests/test_crontab.py index 6ce4c12..5a739ae 100644 --- a/tests/test_crontab.py +++ b/tests/test_crontab.py @@ -205,6 +205,14 @@ def test_run_args_only_format3_job(method_to_call): method_to_call.assert_called_with(1, 'two') +@override_settings(CRONJOBS=[('*/1 * * * *', 'tests.cron.cron_job')]) +@patch('tests.cron.cron_job', side_effect=Exception('foo')) +def test_job_exception(method_to_call): + crontab = Crontab() + assert not crontab.run_job('4f30993ab69a8c5763ce55f762ef0433') + method_to_call.assert_called_with() + + @override_settings(CRONJOBS=[('* * * * *', 'tests.test_crontab.recursive_job_for_lock_checking')], CRONTAB_LOCK_JOBS=True) def test_locked_job(): crontab = Crontab() From 02498722ea0c6fdb920708a1bd4656b6785fdb7b Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Sat, 20 Jul 2019 12:08:36 +0100 Subject: [PATCH 4/5] Fix flake8 issues --- django_crontab/crontab.py | 3 +-- django_crontab/management/commands/crontab.py | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/django_crontab/crontab.py b/django_crontab/crontab.py index 09b5fb3..b3842a1 100755 --- a/django_crontab/crontab.py +++ b/django_crontab/crontab.py @@ -173,7 +173,7 @@ def run_job(self, job_hash): # run the function try: func(*job_args, **job_kwargs) - except: + except BaseException: logger.exception('Failed to complete cronjob at %s', job) return False @@ -211,4 +211,3 @@ def __get_job_by_hash(self, job_hash): 'No job with hash %s found. It seems the crontab is out of sync with your settings.CRONJOBS. ' 'Run "python manage.py crontab add" again to resolve this issue!' % job_hash ) - diff --git a/django_crontab/management/commands/crontab.py b/django_crontab/management/commands/crontab.py index 3c655c2..8ebfda5 100755 --- a/django_crontab/management/commands/crontab.py +++ b/django_crontab/management/commands/crontab.py @@ -5,6 +5,7 @@ import sys + class Command(BaseCommand): """ Define the Django management commands add, show, remove and run From b6e7a8c489fc22e3c318d5a9e84e0858c5be36f7 Mon Sep 17 00:00:00 2001 From: Tom Parker-Shemilt Date: Sat, 20 Jul 2019 12:12:16 +0100 Subject: [PATCH 5/5] Removing "flake8: " prefix to noqa seems to fix the basestring issues --- django_crontab/crontab.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/django_crontab/crontab.py b/django_crontab/crontab.py index b3842a1..f78490d 100755 --- a/django_crontab/crontab.py +++ b/django_crontab/crontab.py @@ -14,7 +14,7 @@ from django_crontab.app_settings import Settings -string_type = basestring if sys.version_info[0] == 2 else str # flake8: noqa +string_type = basestring if sys.version_info[0] == 2 else str # noqa: F821 logger = logging.getLogger(__name__)