Code review comment for lp:~adeuring/launchpad/lp-lazr.jobrunner

Revision history for this message
Abel Deuring (adeuring) wrote :

Some additional changes are necessary to use lazr.jobrunner (see also https://code.launchpad.net/~adeuring/lazr.jobrunner/use_job_repr_in_logging/+merge/98821):

The change in lib/lp/registry/tests/test_process_job_sources_cronjob.py reflects the change from logger.debug() in the original BaseJobRunner.runJob() method to logger.info() in lazr.jobrunner.

The second change: I added the method Job._doOops() again to BasJobRunner. The reason:

The implementation of _doOops() in lazr.jobrunner calls oops_report.publish(). The makes sense because the module does not make any asssumptions about a setup for some sort automatic OOPS generation: It simply publishes the OOPS report and the logs an error message.

This conflicts with the way how Launchpad generates an OOPS in scripts: lp.services.scripts.logger has a class OopsHandler(logging.Handler), which automatically generates an OOPS report, when logger.error() is called. So, a call of _doOops() from lazr.jobrunner first publishes an OOPS, then logs the error -- and a log handler publishes the same OPS report again.

tests:

bin/test -vv \
-t lp.registry.tests.test_process_job_sources_cronjob.ProcessJobSourceGroupsTest.test_processed \
-t lp.registry.tests.test_process_job_sources_cronjob.ProcessJobSourceTest.test_processed \
-t lp.code.scripts.tests.test_create_merge_proposals.TestCreateMergeProposals.test_oops \
-t lp.code.scripts.tests.test_reclaim_branch_space.TestReclaimBranchSpaceScript.test_reclaimbranchspace_script_logs_oops \
-t lp.code.scripts.tests.test_sendbranchmail.TestSendbranchmail.test_sendbranchmail_handles_oops \
-t lp.translations.tests.test_rosetta_branches_script.TestRosettaBranchesScript.test_rosetta_branches_script_oops \
-t lp.registry.tests.test_membership_notification_job.MembershipNotificationJobTest.test_smoke_admining_team

no lint

« Back to merge proposal