Merge lp:~james-w/launchpad/copy-archive-job into lp:launchpad/db-devel

Please ignore the schema/sampledata changes, they are in the pre-requisite branch.

Thanks,

James

I haven't done a through review as I'm not familiar with the job system, so wouldn't be comfortable doing it. I have a few comments, though.

I assume the new tables created to describe a new job are standardised, but we need to have the new one reviewed by either stub or jml.

It's weird to see new code raising SQLObjectNotFound. Does the job system expect that?

I think your tests can do with just the DatabaseFunctionalLayer, which might be faster than LPZopelessLayer.

I'll be happy to do a thorough review tomorrow, if you think it's worth it, but I'd like someone more familiar with the job system to review it as well.

Hi,

I adjusted the layer, and the db tables have been approved.

The SQLObjectNotFound is what the code jobs use, and no-one was
able to suggest anything better when I asked on IRC just now.

Thanks,

James

Hi James,

This branch looks very good. I have minior comments below.

merge-conditional

-Edwin

>=== added file 'lib/lp/soyuz/interfaces/archivejob.py'
>--- lib/lp/soyuz/interfaces/archivejob.py 1970-01-01 00:00:00 +0000
>+++ lib/lp/soyuz/interfaces/archivejob.py 2010-07-28 19:38:05 +0000
>@@ -0,0 +1,55 @@
>+from zope.interface import Attribute, Interface
>+from zope.schema import Int, Object
>+
>+from canonical.launchpad import _
>+
>+from lazr.enum import DBEnumeratedType, DBItem
>+
>+from lp.services.job.interfaces.job import IJob, IJobSource, IRunnableJob
>+from lp.soyuz.interfaces.archive import IArchive
>+
>+
>+class ArchiveJobType(DBEnumeratedType):
>+ """Values that IArchiveJob.job_type can take."""
>+
>+ COPY_ARCHIVE = DBItem(0, """
>+ Create a copy archive.
>+
>+ This job creates a copy archive from the current state of
>+ the archive.
>+ """)
>+
>+
>+class IArchiveJob(Interface):
>+ """A Job related to an Archive."""
>+
>+ id = Int(
>+ title=_('DB ID'), required=True, readonly=True,
>+ description=_("The tracking number for this job."))
>+
>+ archive = Object(
>+ title=_('The Archive this job is about.'), schema=IArchive,

It looks peculiar to occasionally capitalize the noun. For example,
"job" isn't capitalized here, but it is capitalized in the following title.
I think capitalizing to match the class names is better left to doc
strings, since field titles are often exposed to the user, even if that
would never happen for this interface.

>+ required=True)
>+
>+ job = Object(
>+ title=_('The common Job attributes'), schema=IJob, required=True)
>+
>+ metadata = Attribute('A dict of data about the job.')
>+
>+ def destroySelf():
>+ """Destroy this object."""
>+
>+
>+class IArchiveJobSource(IJobSource):
>+ """An interface for acquiring IArchiveJobs."""
>+
>+ def create(archive):
>+ """Create a new IArchiveJobs for an archive."""
>+
>+
>+class ICopyArchiveJob(IRunnableJob):
>+ """A Job to copy archives."""
>+
>+
>+class ICopyArchiveJobSource(IArchiveJobSource):
>+ """Interface for acquiring CopyArchiveJobs."""
>
>=== added file 'lib/lp/soyuz/model/archivejob.py'
>--- lib/lp/soyuz/model/archivejob.py 1970-01-01 00:00:00 +0000
>+++ lib/lp/soyuz/model/archivejob.py 2010-07-28 19:38:05 +0000
>@@ -0,0 +1,130 @@
>+
>+__metaclass__ = object
>+
>+import simplejson
>+
>+from sqlobject import SQLObjectNotFound
>+from storm.base import Storm
>+from storm.expr import And
>+from storm.locals import Int, Reference, Unicode
>+
>+from zope.component import getUtility
>+from zope.interface import classProvides, implements
>+
>+from canonical.database.enumcol import EnumCol
>+from canonical.launchpad.webapp.interfaces import (
>+ DEFAULT_FLAVOR, IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
>+
>+from lazr.delegates import delegates
>+
>+from lp.services.job.model.job import Job
>+from lp.services.job.runner import BaseRunnableJob
>+from lp.soyuz.interfaces.archivejob import (
>+ ArchiveJobType, IArchiveJob, IArchiveJobSource)
>+from lp.soyuz.model.archive import Archive
>+
>+
>+class ArchiveJob(Storm):
>+ """Base class for jobs r...

> Here you are creating a SPPH record for the source_archive, and
> makeSourceAndTarget() creates a SPPH record for the source_archive. To
> test the merge, shouldn't the to packages be in different archives?

It's not the best test, as the setup runs a job.

What we do is create a source archive, and then clone it to a target archive,
we then modify the source archive and request a "merge" operation, which should
copy the new publication we create in the source.

I'm not sure of a better way to write test to make that clear, so I added
comments.

All other changes made as suggested.

Thanks,

James