launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03751
[Merge] lp:~allenap/launchpad/job-status-validator into lp:launchpad
Gavin Panella has proposed merging lp:~allenap/launchpad/job-status-validator into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~allenap/launchpad/job-status-validator/+merge/62509
This removes the status/_status/_set_status issue in Job (where status
is a gettable property, _status represents the database column, and
_set_status is what Job's state transition methods call).
Why?
- I've lost time before wondering why queries aren't working before
realising I need to use Job._status, so this clears that up.
- It simplifies the code because everything goes via the status
property, and all transitions are validated.
- Even code within Job can't (accidentally) cheat on status
transitions, to the extent that a helper, prepare_job(), was
introduced in the tests in order to get a Job in a desired starting
state.
--
https://code.launchpad.net/~allenap/launchpad/job-status-validator/+merge/62509
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~allenap/launchpad/job-status-validator into lp:launchpad.
=== modified file 'lib/lp/buildmaster/model/builder.py'
--- lib/lp/buildmaster/model/builder.py 2011-02-01 18:14:57 +0000
+++ lib/lp/buildmaster/model/builder.py 2011-05-26 16:08:28 +0000
@@ -846,7 +846,7 @@
BuildQueue,
BuildQueue.job == Job.id,
BuildQueue.builder == self.id,
- Job._status == JobStatus.RUNNING,
+ Job.status == JobStatus.RUNNING,
Job.date_started != None).one()
def getCurrentBuildFarmJob(self):
@@ -913,7 +913,7 @@
Coalesce(BuildQueue.virtualized, True)),
Processor.id == BuildQueue.processorID,
Job.id == BuildQueue.jobID,
- Job._status == JobStatus.WAITING).group_by(
+ Job.status == JobStatus.WAITING).group_by(
Processor, Coalesce(BuildQueue.virtualized, True))
result_dict = {'virt': {}, 'nonvirt': {}}
=== modified file 'lib/lp/buildmaster/model/buildqueue.py'
--- lib/lp/buildmaster/model/buildqueue.py 2010-08-30 15:00:23 +0000
+++ lib/lp/buildmaster/model/buildqueue.py 2011-05-26 16:08:28 +0000
@@ -553,6 +553,6 @@
# Avoid corrupt build jobs where the builder is None.
BuildQueue.builder != None,
# status is a property. Let's use _status.
- Job._status == JobStatus.RUNNING,
+ Job.status == JobStatus.RUNNING,
Job.date_started != None)
return result_set
=== modified file 'lib/lp/code/model/branch.py'
--- lib/lp/code/model/branch.py 2011-05-12 11:39:29 +0000
+++ lib/lp/code/model/branch.py 2011-05-26 16:08:28 +0000
@@ -1155,8 +1155,8 @@
BranchJob,
BranchJob.branch == self,
Job.id == BranchJob.jobID,
- Job._status != JobStatus.COMPLETED,
- Job._status != JobStatus.FAILED,
+ Job.status != JobStatus.COMPLETED,
+ Job.status != JobStatus.FAILED,
BranchJob.job_type == BranchJobType.UPGRADE_BRANCH)
return jobs.count() > 0
=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py 2011-05-11 13:59:38 +0000
+++ lib/lp/code/model/branchjob.py 2011-05-26 16:08:28 +0000
@@ -972,8 +972,8 @@
Job.id == BranchJob.jobID,
BranchJob.branch == branch,
BranchJob.job_type == BranchJobType.ROSETTA_UPLOAD,
- Job._status != JobStatus.COMPLETED,
- Job._status != JobStatus.FAILED)
+ Job.status != JobStatus.COMPLETED,
+ Job.status != JobStatus.FAILED)
if since is not None:
match = And(match, Job.date_created > since)
jobs = store.using(BranchJob, Job).find((BranchJob), match)
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2011-05-12 21:33:10 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2011-05-26 16:08:28 +0000
@@ -208,7 +208,7 @@
BranchMergeProposalJob.job_type ==
BranchMergeProposalJobType.UPDATE_PREVIEW_DIFF,
BranchMergeProposalJob.job == Job.id,
- Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
+ Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
job = jobs.order_by(Job.scheduled_start, Job.date_created).first()
if job is not None:
return BranchMergeProposalJobFactory.create(job)
=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py 2011-05-12 21:33:10 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py 2011-05-26 16:08:28 +0000
@@ -774,7 +774,7 @@
TargetBranch = ClassAlias(Branch)
clauses = [
BranchMergeProposalJob.job == Job.id,
- Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
+ Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]),
BranchMergeProposalJob.branch_merge_proposal ==
BranchMergeProposal.id, BranchMergeProposal.source_branch ==
SourceBranch.id, BranchMergeProposal.target_branch ==
@@ -789,7 +789,7 @@
# the date_created, then job type. This should give us all creation
# jobs before comment jobs.
jobs = jobs.order_by(
- Desc(Job._status), Job.date_created,
+ Desc(Job.status), Job.date_created,
Desc(BranchMergeProposalJob.job_type))
# Now only return one job for any given merge proposal.
ready_jobs = []
=== modified file 'lib/lp/code/model/tests/test_branchjob.py'
--- lib/lp/code/model/tests/test_branchjob.py 2011-05-11 13:59:38 +0000
+++ lib/lp/code/model/tests/test_branchjob.py 2011-05-26 16:08:28 +0000
@@ -1342,7 +1342,7 @@
job = self._makeRosettaUploadJob()
job.job.start()
job.job.complete()
- job.job._status = JobStatus.FAILED
+ job.job.status = JobStatus.FAILED
unfinished_jobs = list(RosettaUploadJob.findUnfinishedJobs(
self.branch))
self.assertEqual([], unfinished_jobs)
=== modified file 'lib/lp/code/scripts/tests/test_scan_branches.py'
--- lib/lp/code/scripts/tests/test_scan_branches.py 2010-10-26 15:47:24 +0000
+++ lib/lp/code/scripts/tests/test_scan_branches.py 2011-05-26 16:08:28 +0000
@@ -76,7 +76,7 @@
result = store.find(
BranchJob,
BranchJob.jobID == Job.id,
- Job._status == JobStatus.WAITING,
+ Job.status == JobStatus.WAITING,
BranchJob.job_type == BranchJobType.REVISION_MAIL,
BranchJob.branch == db_branch)
self.assertEqual(result.count(), 1)
=== modified file 'lib/lp/registry/model/packagingjob.py'
--- lib/lp/registry/model/packagingjob.py 2011-03-23 16:28:51 +0000
+++ lib/lp/registry/model/packagingjob.py 2011-05-26 16:08:28 +0000
@@ -193,7 +193,7 @@
packaging.sourcepackagename.id,
PackagingJob.productseries_id == packaging.productseries.id,
PackagingJob.job_type == cls.class_job_type,
- Job._status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
+ Job.status.is_in([JobStatus.WAITING, JobStatus.RUNNING]))
result.order_by(PackagingJob.id)
job = result.first()
if job is None:
=== modified file 'lib/lp/registry/model/persontransferjob.py'
--- lib/lp/registry/model/persontransferjob.py 2011-03-26 21:50:45 +0000
+++ lib/lp/registry/model/persontransferjob.py 2011-05-26 16:08:28 +0000
@@ -358,7 +358,7 @@
conditions = [
PersonTransferJob.job_type == cls.class_job_type,
PersonTransferJob.job_id == Job.id,
- Job._status.is_in(Job.PENDING_STATUSES)]
+ Job.status.is_in(Job.PENDING_STATUSES)]
arg_conditions = []
if from_person is not None:
arg_conditions.append(
=== modified file 'lib/lp/registry/tests/test_person_merge_job.py'
--- lib/lp/registry/tests/test_person_merge_job.py 2011-04-06 23:48:44 +0000
+++ lib/lp/registry/tests/test_person_merge_job.py 2011-05-26 16:08:28 +0000
@@ -174,7 +174,7 @@
def test_find_only_pending_or_running(self):
# find() only returns jobs that are pending.
for status in JobStatus.items:
- removeSecurityProxy(self.job.job)._status = status
+ removeSecurityProxy(self.job.job).status = status
if status in Job.PENDING_STATUSES:
self.assertEqual([self.job], self.find())
else:
=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py 2011-05-19 15:15:16 +0000
+++ lib/lp/services/job/model/job.py 2011-05-26 16:08:28 +0000
@@ -46,6 +46,13 @@
(current_status, requested_status))
+def validate_status_transition(job, attr, status):
+ assert attr == "status"
+ if status not in job._valid_transitions[job.status]:
+ raise InvalidTransition(job.status, status)
+ return status
+
+
class Job(SQLBase):
"""See `IJob`."""
@@ -63,8 +70,9 @@
log = StringCol()
- _status = EnumCol(enum=JobStatus, notNull=True, default=JobStatus.WAITING,
- dbName='status')
+ status = EnumCol(
+ enum=JobStatus, notNull=True, default=JobStatus.WAITING,
+ dbName='status', storm_validator=validate_status_transition)
attempt_count = IntCol(default=0)
@@ -91,13 +99,6 @@
JobStatus.RUNNING,
JobStatus.SUSPENDED))
- def _set_status(self, status):
- if status not in self._valid_transitions[self._status]:
- raise InvalidTransition(self._status, status)
- self._status = status
-
- status = property(lambda x: x._status)
-
def acquireLease(self, duration=300):
"""See `IJob`."""
if (self.lease_expires is not None
@@ -118,41 +119,41 @@
def start(self):
"""See `IJob`."""
- self._set_status(JobStatus.RUNNING)
+ self.status = JobStatus.RUNNING
self.date_started = datetime.datetime.now(UTC)
self.date_finished = None
self.attempt_count += 1
def complete(self):
"""See `IJob`."""
- self._set_status(JobStatus.COMPLETED)
+ self.status = JobStatus.COMPLETED
self.date_finished = datetime.datetime.now(UTC)
def fail(self):
"""See `IJob`."""
- self._set_status(JobStatus.FAILED)
+ self.status = JobStatus.FAILED
self.date_finished = datetime.datetime.now(UTC)
def queue(self):
"""See `IJob`."""
- self._set_status(JobStatus.WAITING)
+ self.status = JobStatus.WAITING
self.date_finished = datetime.datetime.now(UTC)
def suspend(self):
"""See `IJob`."""
- self._set_status(JobStatus.SUSPENDED)
+ self.status = JobStatus.SUSPENDED
def resume(self):
"""See `IJob`."""
if self.status is not JobStatus.SUSPENDED:
- raise InvalidTransition(self._status, JobStatus.WAITING)
- self._set_status(JobStatus.WAITING)
+ raise InvalidTransition(self.status, JobStatus.WAITING)
+ self.status = JobStatus.WAITING
Job.ready_jobs = Select(
Job.id,
And(
- Job._status == JobStatus.WAITING,
+ Job.status == JobStatus.WAITING,
Or(Job.lease_expires == None, Job.lease_expires < UTC_NOW),
Or(Job.scheduled_start == None, Job.scheduled_start <= UTC_NOW),
))
=== modified file 'lib/lp/services/job/tests/test_job.py'
--- lib/lp/services/job/tests/test_job.py 2011-03-23 15:30:02 +0000
+++ lib/lp/services/job/tests/test_job.py 2011-05-26 16:08:28 +0000
@@ -30,6 +30,15 @@
from lp.testing import TestCase
+def prepare_job(*status_transitions):
+ """Creates a new job and moves it through the given status transitions."""
+ job = Job()
+ for status_transition in status_transitions:
+ if job.status != status_transition:
+ job.status = status_transition
+ return job
+
+
class TestJob(TestCase):
"""Ensure Job behaves as intended."""
@@ -69,17 +78,17 @@
def test_start_when_completed_is_invalid(self):
"""When a job is completed, attempting to start is invalid."""
- job = Job(_status=JobStatus.COMPLETED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED)
self.assertRaises(InvalidTransition, job.start)
def test_start_when_failed_is_invalid(self):
"""When a job is failed, attempting to start is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.start)
def test_start_when_running_is_invalid(self):
"""When a job is running, attempting to start is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.start)
def test_complete(self):
@@ -87,7 +96,7 @@
It should set date_finished and set the job status to COMPLETED.
"""
- job = Job(_status=JobStatus.RUNNING)
+ job = Job(status=JobStatus.RUNNING)
self.assertEqual(None, job.date_finished)
job.complete()
self.assertNotEqual(None, job.date_finished)
@@ -95,17 +104,17 @@
def test_complete_when_waiting_is_invalid(self):
"""When a job is waiting, attempting to complete is invalid."""
- job = Job(_status=JobStatus.WAITING)
+ job = prepare_job(JobStatus.WAITING)
self.assertRaises(InvalidTransition, job.complete)
def test_complete_when_completed_is_invalid(self):
"""When a job is completed, attempting to complete is invalid."""
- job = Job(_status=JobStatus.COMPLETED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED)
self.assertRaises(InvalidTransition, job.complete)
def test_complete_when_failed_is_invalid(self):
"""When a job is failed, attempting to complete is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.complete)
def test_fail(self):
@@ -113,7 +122,7 @@
It should set date_finished and set the job status to FAILED.
"""
- job = Job(_status=JobStatus.RUNNING)
+ job = Job(status=JobStatus.RUNNING)
self.assertEqual(None, job.date_finished)
job.fail()
self.assertNotEqual(None, job.date_finished)
@@ -121,17 +130,17 @@
def test_fail_when_waiting_is_invalid(self):
"""When a job is waiting, attempting to fail is invalid."""
- job = Job(_status=JobStatus.WAITING)
+ job = prepare_job(JobStatus.WAITING)
self.assertRaises(InvalidTransition, job.fail)
def test_fail_when_completed_is_invalid(self):
"""When a job is completed, attempting to fail is invalid."""
- job = Job(_status=JobStatus.COMPLETED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED)
self.assertRaises(InvalidTransition, job.fail)
def test_fail_when_failed_is_invalid(self):
"""When a job is failed, attempting to fail is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.fail)
def test_queue(self):
@@ -139,7 +148,7 @@
It should set date_finished, and set status to WAITING.
"""
- job = Job(_status=JobStatus.RUNNING)
+ job = Job(status=JobStatus.RUNNING)
self.assertEqual(None, job.date_finished)
job.queue()
self.assertNotEqual(None, job.date_finished)
@@ -147,22 +156,22 @@
def test_queue_when_completed_is_invalid(self):
"""When a job is completed, attempting to queue is invalid."""
- job = Job(_status=JobStatus.COMPLETED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED)
self.assertRaises(InvalidTransition, job.queue)
def test_queue_when_waiting_is_invalid(self):
"""When a job is waiting, attempting to queue is invalid."""
- job = Job(_status=JobStatus.WAITING)
+ job = prepare_job(JobStatus.WAITING)
self.assertRaises(InvalidTransition, job.queue)
def test_queue_when_failed_is_invalid(self):
"""When a job is failed, attempting to queue is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.queue)
def test_suspend(self):
"""A job that is in the WAITING state can be suspended."""
- job = Job(_status=JobStatus.WAITING)
+ job = prepare_job(JobStatus.WAITING)
job.suspend()
self.assertEqual(
job.status,
@@ -170,22 +179,22 @@
def test_suspend_when_running(self):
"""When a job is running, attempting to suspend is invalid."""
- job = Job(_status=JobStatus.RUNNING)
+ job = Job(status=JobStatus.RUNNING)
self.assertRaises(InvalidTransition, job.suspend)
def test_suspend_when_completed(self):
"""When a job is completed, attempting to suspend is invalid."""
- job = Job(_status=JobStatus.COMPLETED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED)
self.assertRaises(InvalidTransition, job.suspend)
def test_suspend_when_failed(self):
"""When a job is failed, attempting to suspend is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.suspend)
def test_resume(self):
"""A job that is suspended can be resumed."""
- job = Job(_status=JobStatus.SUSPENDED)
+ job = Job(status=JobStatus.SUSPENDED)
job.resume()
self.assertEqual(
job.status,
@@ -193,17 +202,17 @@
def test_resume_when_running(self):
"""When a job is running, attempting to resume is invalid."""
- job = Job(_status=JobStatus.RUNNING)
+ job = Job(status=JobStatus.RUNNING)
self.assertRaises(InvalidTransition, job.resume)
def test_resume_when_completed(self):
"""When a job is completed, attempting to resume is invalid."""
- job = Job(_status=JobStatus.COMPLETED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.COMPLETED)
self.assertRaises(InvalidTransition, job.resume)
def test_resume_when_failed(self):
"""When a job is failed, attempting to resume is invalid."""
- job = Job(_status=JobStatus.FAILED)
+ job = prepare_job(JobStatus.RUNNING, JobStatus.FAILED)
self.assertRaises(InvalidTransition, job.resume)
@@ -227,7 +236,7 @@
def test_ready_jobs_started(self):
"""Job.ready_jobs should not jobs that have been started."""
preexisting = self._sampleData()
- job = Job(_status=JobStatus.RUNNING)
+ job = Job(status=JobStatus.RUNNING)
self.assertEqual(
preexisting, list(Store.of(job).execute(Job.ready_jobs)))
=== modified file 'lib/lp/soyuz/model/initialisedistroseriesjob.py'
--- lib/lp/soyuz/model/initialisedistroseriesjob.py 2011-04-13 09:55:48 +0000
+++ lib/lp/soyuz/model/initialisedistroseriesjob.py 2011-05-26 16:08:28 +0000
@@ -60,7 +60,7 @@
DistributionJob.job_type ==
DistributionJobType.INITIALISE_SERIES,
DistributionJob.distroseries_id == distroseries.id,
- Job._status.is_in(Job.PENDING_STATUSES))
+ Job.status.is_in(Job.PENDING_STATUSES))
@property
def parent(self):
