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

Non-zero exit code on run_job failure #100

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions django_crontab/crontab.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

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

Not quite sure why this was now failing, as it's got nothing to do with my changes. Changing to an explicit ignore seems to work fine


logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -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)
Expand All @@ -173,8 +173,9 @@ 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

# if the LOCK_JOBS option is specified in settings
if self.settings.LOCK_JOBS:
Expand All @@ -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):
"""
Expand All @@ -208,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
)

6 changes: 5 additions & 1 deletion django_crontab/management/commands/crontab.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
from django.core.management.base import BaseCommand
from django_crontab.crontab import Crontab

import sys


class Command(BaseCommand):
"""
Expand Down Expand Up @@ -32,7 +34,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)
9 changes: 9 additions & 0 deletions tests/test_command.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')
26 changes: 17 additions & 9 deletions tests/test_crontab.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,46 +169,54 @@ 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()


@override_settings(CRONJOBS=[('*/1 * * * *', 'tests.cron.cron_job', '> /home/myhome/logs/cron_job.log')])
@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()


@override_settings(CRONJOBS=[('*/1 * * * *', 'tests.cron.cron_job', [1, 'two'])])
@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')


@override_settings(CRONJOBS=[('*/1 * * * *', 'tests.cron.cron_job', [1, 'two'], {'test': 34, 'a': 's2'})])
@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):
Copy link
Author

Choose a reason for hiding this comment

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

I got warnings about this being a duplicate name with the above test, so I renamed it

crontab = Crontab()
crontab.run_job('fefa68aed4ff509331ee6a5b62ea5e5c')
assert crontab.run_job('fefa68aed4ff509331ee6a5b62ea5e5c')
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()
crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b')
assert crontab.run_job('4a8e5d03cf136a16c7d120c41efb602b')


def recursive_job_for_lock_checking():
Expand All @@ -217,12 +225,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')