Update FailedRunsNotificationCronJob to report more clearly#101
Update FailedRunsNotificationCronJob to report more clearly#101evenicoulddoit wants to merge 3 commits intoTivix:masterfrom
Conversation
evenicoulddoit
force-pushed
the
failed-notifications-improvements
branch
from
cdb858a to
c1e3614
Compare
4 similar comments
tab-cmd
left a comment
tab-cmd
left a comment
There was a problem hiding this comment.
This looks pretty good, I left some comments for you to address. Once done, I will test locally again and merge. Thanks for the contributions 👍
django_cron/cron.py
Outdated
| detailing the error. | ||
| """ | ||
| RUN_EVERY_MINS = 30 | ||
| RUN_EVERY_MINS = 0 |
There was a problem hiding this comment.
Put this in settings to and add to readme. There may be a case when someone won't want this to run every time their crons run
| code = 'django_cron.FailedRunsNotificationCronJob' | ||
|
|
||
| def do(self): | ||
| self.config = self.get_config() |
There was a problem hiding this comment.
add documentation as you do below
docs/sample_cron_configurations.rst
Outdated
| ----------------------- | ||
|
|
||
| By deafult parallels runs are not allowed (for security reasons). However if you | ||
| By default parallels runs are not allowed (for security reasons). However if you |
There was a problem hiding this comment.
while you're here: should be By default parallel runs ...
| """ | ||
| Report in Slack that the given Cron job failed. | ||
| """ | ||
| slack.post("ERROR - Cron job '{0}' failed.".format(cron_cls.code)) |
django_cron/cron.py
Outdated
| A regular job to send email reports for failed Cron jobs. | ||
|
|
||
| The job log is used to check for all unreported failures for each job | ||
| classes specified within the CRON_CLASSES dictionary. When the number of |
django_cron/cron.py
Outdated
|
|
||
| def do(self): | ||
| self.config = self.get_config() | ||
| cron_classes = [get_class(c_name) for c_name in settings.CRON_CLASSES] |
There was a problem hiding this comment.
Be as explicit as possible throughout. c_name to class_name , cron_cls to cron_class (this is referenced a lot), and avoid single letter variables. While it isn't difficult to follow it would be nice.
| This sample cron job checks for any unreported failed jobs for each job class | ||
| provided in your ``CRON_CLASSES`` list, and reports them as necessary. The job | ||
| is set to run on each ``runcrons`` task, and the default process is to email | ||
| all the users specified in the ``ADMINS`` settings list when a job fails more |
There was a problem hiding this comment.
the default process is to email all the users specified in the
ADMINSsettings list when a job fails more ...
add using the default from email from settings list as well
| if not job.is_success: | ||
| failures += 1 | ||
| message += 'Job ran at %s : \n\n %s \n\n' % (job.start_time, job.message) | ||
| if len(failed_reports) > 1: |
There was a problem hiding this comment.
shouldn't this always be true / == to the min_failures? Why not just define times before first definition of subject and add to it there?
There was a problem hiding this comment.
Not necessarily, because the failed report job doesn't have to run every time that another job does. For example, you could set the failed reported to run every day, so its entirely possible for the number of failures to exceed the MIN_NUM_FAILURES option.
* Fix typos in documentation * Add a docstring to `do()` * Namespace configuration, and add documentation for all settings * Extend test coverage
|
@sci-tab thanks for going through the PR, should have addressed all your comments |
JedrzejMaluszczak
force-pushed
the
master
branch
8 times, most recently
from
edf6cd4 to
c0bbe43
Compare
3 participants
failure_reportedflag added to each log entry, which is used to avoid notifying users multiple times about the same failed job>=to account for when the job in question has run multiple times before the notification job runs