launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #04289
[Merge] lp:~julian-edwards/launchpad/pcj-requester-bug-810957 into lp:launchpad
Julian Edwards has proposed merging lp:~julian-edwards/launchpad/pcj-requester-bug-810957 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~julian-edwards/launchpad/pcj-requester-bug-810957/+merge/68081
= Summary =
Enable PackageCopyJob to store the Person who requested it.
== Pre-implementation notes ==
Chatted to Gavin who first wrote PCJ. IJob has a database column for requester
which is not declared in the model code, so we can just use that with some
changes to IJob.
== Implementation details ==
Fix IJob's model implementation, its createMultiple() and PackageCopyJob's
create() and createMutliple().
Note that I've had to add requestor=None to create() even though it's requred,
since I can't be sure of all the places it's called from and they would all need fixing to match the new positional parameters. The assertion matches the one
done for package_version for the same reason.
== Tests ==
bin/test -cvv test_packagecopyjob
== Demo and Q/A ==
n/a until something uses it.
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
lib/lp/services/job/interfaces/job.py
lib/lp/soyuz/model/packagecopyjob.py
lib/lp/soyuz/interfaces/packagecopyjob.py
lib/lp/testing/factory.py
lib/lp/soyuz/tests/test_packagecopyjob.py
lib/lp/services/job/tests/test_job.py
lib/lp/services/job/model/job.py
lib/lp/registry/browser/distroseries.py
--
https://code.launchpad.net/~julian-edwards/launchpad/pcj-requester-bug-810957/+merge/68081
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/launchpad/pcj-requester-bug-810957 into lp:launchpad.
=== modified file 'lib/lp/registry/browser/distroseries.py'
--- lib/lp/registry/browser/distroseries.py 2011-07-08 09:06:24 +0000
+++ lib/lp/registry/browser/distroseries.py 2011-07-15 12:13:27 +0000
@@ -1161,7 +1161,7 @@
)
for dsd in self.getUpgrades()]
getUtility(IPlainPackageCopyJobSource).createMultiple(
- target_distroseries, copies,
+ target_distroseries, copies, self.user,
copy_policy=PackageCopyPolicy.MASS_SYNC)
self.request.response.addInfoNotification(
=== modified file 'lib/lp/services/job/interfaces/job.py'
--- lib/lp/services/job/interfaces/job.py 2011-07-06 14:02:54 +0000
+++ lib/lp/services/job/interfaces/job.py 2011-07-15 12:13:27 +0000
@@ -22,6 +22,7 @@
DBEnumeratedType,
DBItem,
)
+from lazr.restful.fields import Reference
from zope.interface import (
Attribute,
Interface,
@@ -35,6 +36,7 @@
)
from canonical.launchpad import _
+from lp.registry.interfaces.person import IPerson
class SuspendJobException(Exception):
@@ -109,6 +111,11 @@
max_retries = Int(title=_(
'The number of retries permitted before this job permanently fails.'))
+ requester = Reference(
+ IPerson, title=_("The person who requested the job"),
+ required=False, readonly=True
+ )
+
is_pending = Bool(
title=_("Whether or not this job's status is such that it "
"could eventually complete."))
=== modified file 'lib/lp/services/job/model/job.py'
--- lib/lp/services/job/model/job.py 2011-07-06 14:02:54 +0000
+++ lib/lp/services/job/model/job.py 2011-07-15 12:13:27 +0000
@@ -21,6 +21,10 @@
Or,
Select,
)
+from storm.locals import (
+ Int,
+ Reference,
+ )
from zope.interface import implements
from canonical.database.constants import UTC_NOW
@@ -73,6 +77,9 @@
max_retries = IntCol(default=0)
+ requester_id = Int(name='requester', allow_none=True)
+ requester = Reference(requester_id, 'Person.id')
+
# Mapping of valid target states from a given state.
_valid_transitions = {
JobStatus.WAITING:
@@ -108,16 +115,19 @@
return self.status in self.PENDING_STATUSES
@classmethod
- def createMultiple(self, store, num_jobs):
+ def createMultiple(self, store, num_jobs, requester=None):
"""Create multiple `Job`s at once.
:param store: `Store` to ceate the jobs in.
:param num_jobs: Number of `Job`s to create.
+ :param request: The `IPerson` requesting the jobs.
:return: An iterable of `Job.id` values for the new jobs.
"""
- job_contents = ["(%s)" % quote(JobStatus.WAITING)] * num_jobs
+ job_contents = [
+ "(%s, %s)" % (
+ quote(JobStatus.WAITING), quote(requester))] * num_jobs
result = store.execute("""
- INSERT INTO Job (status)
+ INSERT INTO Job (status, requester)
VALUES %s
RETURNING id
""" % ", ".join(job_contents))
=== modified file 'lib/lp/services/job/tests/test_job.py'
--- lib/lp/services/job/tests/test_job.py 2011-07-06 14:02:54 +0000
+++ lib/lp/services/job/tests/test_job.py 2011-07-15 12:13:27 +0000
@@ -22,10 +22,13 @@
Job,
LeaseHeld,
)
-from lp.testing import TestCase
-
-
-class TestJob(TestCase):
+from lp.testing import (
+ TestCase,
+ TestCaseWithFactory,
+ )
+
+
+class TestJob(TestCaseWithFactory):
"""Ensure Job behaves as intended."""
layer = ZopelessDatabaseLayer
@@ -39,6 +42,12 @@
job = Job()
self.assertEqual(job.status, JobStatus.WAITING)
+ def test_stores_requester(self):
+ job = Job()
+ random_joe = self.factory.makePerson()
+ job.requester = random_joe
+ self.assertEqual(random_joe, job.requester)
+
def test_createMultiple_creates_requested_number_of_jobs(self):
job_ids = list(Job.createMultiple(IStore(Job), 3))
self.assertEqual(3, len(job_ids))
@@ -55,6 +64,17 @@
job = store.get(Job, Job.createMultiple(store, 1)[0])
self.assertEqual(JobStatus.WAITING, job.status)
+ def test_createMultiple_sets_requester(self):
+ store = IStore(Job)
+ requester = self.factory.makePerson()
+ job = store.get(Job, Job.createMultiple(store, 1, requester)[0])
+ self.assertEqual(requester, job.requester)
+
+ def test_createMultiple_defaults_requester_to_None(self):
+ store = IStore(Job)
+ job = store.get(Job, Job.createMultiple(store, 1)[0])
+ self.assertEqual(None, job.requester)
+
def test_start(self):
"""Job.start should update the object appropriately.
=== modified file 'lib/lp/soyuz/interfaces/packagecopyjob.py'
--- lib/lp/soyuz/interfaces/packagecopyjob.py 2011-07-12 14:23:40 +0000
+++ lib/lp/soyuz/interfaces/packagecopyjob.py 2011-07-15 12:13:27 +0000
@@ -126,7 +126,7 @@
def create(package_name, source_archive,
target_archive, target_distroseries, target_pocket,
include_binaries=False, package_version=None,
- copy_policy=PackageCopyPolicy.INSECURE):
+ copy_policy=PackageCopyPolicy.INSECURE, requester=None):
"""Create a new `IPlainPackageCopyJob`.
:param package_name: The name of the source package to copy.
@@ -141,9 +141,10 @@
:param package_version: The version string for the package version
that is to be copied.
:param copy_policy: Applicable `PackageCopyPolicy`.
+ :param requester: The user requesting the copy.
"""
- def createMultiple(target_distroseries, copy_tasks,
+ def createMultiple(target_distroseries, copy_tasks, requester,
copy_policy=PackageCopyPolicy.INSECURE,
include_binaries=False):
"""Create multiple new `IPlainPackageCopyJob`s at once.
@@ -153,6 +154,7 @@
:param copy_tasks: A list of tuples describing the copies to be
performed: (package name, package version, source archive,
target archive, target pocket).
+ :param requester: The user requesting the copy.
:param copy_policy: Applicable `PackageCopyPolicy`.
:param include_binaries: As in `do_copy`.
:return: An iterable of `PackageCopyJob` ids.
=== modified file 'lib/lp/soyuz/model/packagecopyjob.py'
--- lib/lp/soyuz/model/packagecopyjob.py 2011-07-08 09:06:24 +0000
+++ lib/lp/soyuz/model/packagecopyjob.py 2011-07-15 12:13:27 +0000
@@ -131,9 +131,11 @@
return cls.wrap(IStore(PackageCopyJob).get(PackageCopyJob, pcj_id))
def __init__(self, source_archive, target_archive, target_distroseries,
- job_type, metadata, package_name=None, copy_policy=None):
+ job_type, metadata, requester, package_name=None,
+ copy_policy=None):
super(PackageCopyJob, self).__init__()
self.job = Job()
+ self.job.requester = requester
self.job_type = job_type
self.source_archive = source_archive
self.target_archive = target_archive
@@ -250,9 +252,10 @@
def create(cls, package_name, source_archive,
target_archive, target_distroseries, target_pocket,
include_binaries=False, package_version=None,
- copy_policy=PackageCopyPolicy.INSECURE):
+ copy_policy=PackageCopyPolicy.INSECURE, requester=None):
"""See `IPlainPackageCopyJobSource`."""
assert package_version is not None, "No package version specified."
+ assert requester is not None, "No requester specified."
metadata = cls._makeMetadata(
target_pocket, package_version, include_binaries)
job = PackageCopyJob(
@@ -262,7 +265,8 @@
target_distroseries=target_distroseries,
package_name=package_name,
copy_policy=copy_policy,
- metadata=metadata)
+ metadata=metadata,
+ requester=requester)
IMasterStore(PackageCopyJob).add(job)
return cls(job)
@@ -292,12 +296,12 @@
return format_string % sqlvalues(*data)
@classmethod
- def createMultiple(cls, target_distroseries, copy_tasks,
+ def createMultiple(cls, target_distroseries, copy_tasks, requester,
copy_policy=PackageCopyPolicy.INSECURE,
include_binaries=False):
"""See `IPlainPackageCopyJobSource`."""
store = IMasterStore(Job)
- job_ids = Job.createMultiple(store, len(copy_tasks))
+ job_ids = Job.createMultiple(store, len(copy_tasks), requester)
job_contents = [
cls._composeJobInsertionTuple(
target_distroseries, copy_policy, include_binaries, job_id,
=== modified file 'lib/lp/soyuz/tests/test_packagecopyjob.py'
--- lib/lp/soyuz/tests/test_packagecopyjob.py 2011-07-12 14:52:52 +0000
+++ lib/lp/soyuz/tests/test_packagecopyjob.py 2011-07-15 12:13:27 +0000
@@ -92,9 +92,10 @@
target_archive = dsd.derived_series.main_archive
target_distroseries = dsd.derived_series
target_pocket = self.factory.getAnyPocket()
+ requester = self.factory.makePerson()
return getUtility(IPlainPackageCopyJobSource).create(
dsd.source_package_name.name, source_archive, target_archive,
- target_distroseries, target_pocket,
+ target_distroseries, target_pocket, requester=requester,
package_version=dsd.parent_source_version, **kwargs)
def runJob(self, job):
@@ -122,13 +123,15 @@
distroseries = self.factory.makeDistroSeries()
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
+ requester = self.factory.makePerson()
source = getUtility(IPlainPackageCopyJobSource)
job = source.create(
package_name="foo", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
package_version="1.0-1", include_binaries=False,
- copy_policy=PackageCopyPolicy.MASS_SYNC)
+ copy_policy=PackageCopyPolicy.MASS_SYNC,
+ requester=requester)
self.assertProvides(job, IPackageCopyJob)
self.assertEquals(archive1.id, job.source_archive_id)
self.assertEquals(archive1, job.source_archive)
@@ -140,6 +143,7 @@
self.assertEqual("1.0-1", job.package_version)
self.assertEquals(False, job.include_binaries)
self.assertEquals(PackageCopyPolicy.MASS_SYNC, job.copy_policy)
+ self.assertEqual(requester, job.requester)
def test_createMultiple_creates_one_job_per_copy(self):
mother = self.factory.makeDistroSeriesParent()
@@ -148,6 +152,7 @@
derived_series=derived_series)
mother_package = self.factory.makeSourcePackageName()
father_package = self.factory.makeSourcePackageName()
+ requester = self.factory.makePerson()
job_source = getUtility(IPlainPackageCopyJobSource)
copy_tasks = [
(
@@ -166,7 +171,8 @@
),
]
job_ids = list(
- job_source.createMultiple(mother.derived_series, copy_tasks))
+ job_source.createMultiple(mother.derived_series, copy_tasks,
+ requester))
jobs = list(job_source.getActiveJobs(derived_series.main_archive))
self.assertContentEqual(job_ids, [job.id for job in jobs])
self.assertEqual(len(copy_tasks), len(set([job.job for job in jobs])))
@@ -185,17 +191,24 @@
for job in jobs]
self.assertEqual(copy_tasks, requested_copies)
+ # The passed requester should be the same on all jobs.
+ actual_requester = set(job.requester for job in jobs)
+ self.assertEqual(1, len(actual_requester))
+ self.assertEqual(requester, jobs[0].requester)
+
def test_getActiveJobs(self):
# getActiveJobs() can retrieve all active jobs for an archive.
distroseries = self.factory.makeDistroSeries()
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="foo", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="1.0-1", include_binaries=False)
+ package_version="1.0-1", include_binaries=False,
+ requester=requester)
self.assertContentEqual([job], source.getActiveJobs(archive2))
def test_getActiveJobs_gets_oldest_first(self):
@@ -249,12 +262,14 @@
distroseries = self.factory.makeDistroSeries()
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
+ requester = self.factory.makePerson()
job_source = getUtility(IPlainPackageCopyJobSource)
job = job_source.create(
package_name="foo", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="1.0-1", include_binaries=False)
+ package_version="1.0-1", include_binaries=False,
+ requester=requester)
naked_job = removeSecurityProxy(job)
naked_job.reportFailure = FakeMethod()
@@ -268,12 +283,14 @@
package = self.factory.makeSourcePackageName()
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
+ requester = self.factory.makePerson()
source = getUtility(IPlainPackageCopyJobSource)
job = source.create(
package_name=package.name, source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.UPDATES,
- include_binaries=False, package_version='1.0')
+ include_binaries=False, package_version='1.0',
+ requester=requester)
naked_job = removeSecurityProxy(job)
naked_job.reportFailure = FakeMethod()
@@ -315,12 +332,14 @@
archive=target_archive)
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="libc",
source_archive=breezy_archive, target_archive=target_archive,
target_distroseries=target_series,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="2.8-1", include_binaries=False)
+ package_version="2.8-1", include_binaries=False,
+ requester=requester)
self.assertEqual("libc", job.package_name)
self.assertEqual("2.8-1", job.package_version)
@@ -348,12 +367,14 @@
distroseries = self.factory.makeDistroSeries()
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
+ requester = self.factory.makePerson()
source = getUtility(IPlainPackageCopyJobSource)
job = source.create(
package_name="foo", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="1.0-1", include_binaries=False)
+ package_version="1.0-1", include_binaries=False,
+ requester=requester)
oops_vars = job.getOopsVars()
naked_job = removeSecurityProxy(job)
self.assertIn(
@@ -374,6 +395,7 @@
distroseries = publisher.breezy_autotest
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
+ requester = self.factory.makePerson()
publisher.getPubSource(
distroseries=distroseries, sourcename="libc",
version="2.8-1", status=PackagePublishingStatus.PUBLISHED,
@@ -382,7 +404,8 @@
package_name="libc", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="2.8-1", include_binaries=False)
+ package_version="2.8-1", include_binaries=False,
+ requester=requester)
transaction.commit()
out, err, exit_code = run_script(
@@ -401,12 +424,14 @@
distroseries = self.factory.makeDistroSeries()
archive1 = self.factory.makeArchive(distroseries.distribution)
archive2 = self.factory.makeArchive(distroseries.distribution)
+ requester = self.factory.makePerson()
source = getUtility(IPlainPackageCopyJobSource)
job = source.create(
package_name="foo", source_archive=archive1,
target_archive=archive2, target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- package_version="1.0-1", include_binaries=True)
+ package_version="1.0-1", include_binaries=True,
+ requester=requester)
self.assertEqual(
("<PlainPackageCopyJob to copy package foo from "
"{distroseries.distribution.name}/{archive1.name} to "
@@ -519,6 +544,7 @@
# target_archive.
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="libc",
package_version="2.8-1",
@@ -526,7 +552,8 @@
target_archive=target_archive,
target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False)
+ include_binaries=False,
+ requester=requester)
self.runJob(job)
@@ -556,6 +583,7 @@
# Now, run the copy job, which should raise an error because
# there's no ancestry.
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="copyme",
package_version="2.8-1",
@@ -563,7 +591,8 @@
target_archive=target_archive,
target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False)
+ include_binaries=False,
+ requester=requester)
self.assertRaises(SuspendJobException, self.runJob, job)
# Simulate the job runner suspending after getting a
@@ -619,6 +648,7 @@
# Now, run the copy job.
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="copyme",
package_version="2.8-1",
@@ -626,7 +656,8 @@
target_archive=target_archive,
target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False)
+ include_binaries=False,
+ requester=requester)
# The job should be suspended and there's a PackageUpload with
# its package_copy_job set.
@@ -672,6 +703,7 @@
# Now, run the copy job.
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="copyme",
package_version="2.8-1",
@@ -679,7 +711,8 @@
target_archive=target_archive,
target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False)
+ include_binaries=False,
+ requester=requester)
# The job should be suspended and there's a PackageUpload with
# its package_copy_job set in the UNAPPROVED queue.
@@ -715,6 +748,7 @@
archive=source_archive)
source = getUtility(IPlainPackageCopyJobSource)
+ requester = self.factory.makePerson()
job = source.create(
package_name="copyme",
package_version="2.8-1",
@@ -722,7 +756,8 @@
target_archive=target_archive,
target_distroseries=distroseries,
target_pocket=PackagePublishingPocket.RELEASE,
- include_binaries=False)
+ include_binaries=False,
+ requester=requester)
# Run the job so it gains a PackageUpload.
self.assertRaises(SuspendJobException, self.runJob, job)
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2011-07-13 20:55:34 +0000
+++ lib/lp/testing/factory.py 2011-07-15 12:13:27 +0000
@@ -4165,7 +4165,8 @@
def makePlainPackageCopyJob(
self, package_name=None, package_version=None, source_archive=None,
- target_archive=None, target_distroseries=None, target_pocket=None):
+ target_archive=None, target_distroseries=None, target_pocket=None,
+ requester=None):
"""Create a new `PlainPackageCopyJob`."""
if package_name is None and package_version is None:
package_name = self.makeSourcePackageName().name
@@ -4178,10 +4179,12 @@
target_distroseries = self.makeDistroSeries()
if target_pocket is None:
target_pocket = self.getAnyPocket()
+ if requester is None:
+ requester = self.makePerson()
return getUtility(IPlainPackageCopyJobSource).create(
package_name, source_archive, target_archive,
target_distroseries, target_pocket,
- package_version=package_version)
+ package_version=package_version, requester=requester)
# Some factory methods return simple Python types. We don't add
