Code review comment for lp:~michael.nelson/launchpad/db-build-farm-job-model

Hi Michael,

a nice branch; just just have a few formal nitpicks.

> === modified file 'lib/lp/buildmaster/interfaces/buildfarmjob.py'
> --- lib/lp/buildmaster/interfaces/buildfarmjob.py 2010-04-21 07:15:40 +0000
> +++ lib/lp/buildmaster/interfaces/buildfarmjob.py 2010-04-22 07:42:23 +0000
> @@ -9,15 +9,20 @@
>
> __all__ = [
> 'IBuildFarmJob',
> + 'IBuildFarmJobSource',
> 'IBuildFarmJobDerived',
> 'BuildFarmJobType',
> ]
>
> from zope.interface import Interface, Attribute
> -
> -from canonical.launchpad import _
> +from zope.schema import Bool, Choice, Datetime
> from lazr.enum import DBEnumeratedType, DBItem
> from lazr.restful.fields import Reference
> +
> +from canonical.launchpad import _
> +from canonical.launchpad.interfaces.librarian import ILibraryFileAlias
> +
> +from lp.buildmaster.interfaces.builder import IBuilder
> from lp.soyuz.interfaces.processor import IProcessor
>
>
> @@ -56,6 +61,66 @@
> class IBuildFarmJob(Interface):
> """Operations that jobs for the build farm must implement."""
>
> + id = Attribute('The build farm job ID.')
> +
> + processor = Reference(
> + IProcessor, title=_("Processor"), required=False, readonly=True,
> + description=_(
> + "The Processor required by this build farm job. "
> + "For processor-independent job types please return None."))

It is perhaps my limited English knowledge, but this sounds to me
like a polite request to the implementation class to do the right
thing ;) What about "should|must be None for processor-independent jobs"?

> +
> + virtualized = Bool(
> + title=_('Virtualized'), required=False, readonly=True,
> + description=_(
> + "The virtualization setting required by this build farm job. "
> + "For job types that do not care about virtualization please "
> + "return None."))

Same here.

> +
> + date_created = Datetime(
> + title=_("Date created"), required=True, readonly=True,
> + description=_("The timestamp when the build farm job was created."))
> +
> + date_started = Datetime(
> + title=_("Date started"), required=False, readonly=True,
> + description=_("The timestamp when the build farm job was started."))
> +
> + date_finished = Datetime(
> + title=_("Date finished"), required=False, readonly=True,
> + description=_("The timestamp when the build farm job was finished."))
> +
> + date_first_dispatched = Datetime(
> + title=_("Date finished"), required=False, readonly=True,
> + description=_("The timestamp when the build farm job was finished."))

s/finished/dispatched/

> +
> + builder = Reference(
> + title=_("Builder"), schema=IBuilder, required=False, readonly=True,
> + description=_("The builder assigned to this job."))
> +
> + status = Choice(
> + title=_('Status'), required=True,
> + # Really PackagePublishingPocket, patched in

s/PackagePublishingPocket/BuildStatus/

[...]

> @@ -149,3 +202,17 @@
> accurately based on this job's properties.
> """
>
> +
> +class IBuildFarmJobSource(Interface):
> + """A utility of BuildFarmJob used to create _things_."""
> +
> + def new(job_type, status=None, processor=None,
> + virtualized=None):
> + """Create a new `IBuildFarmJob`.
> +
> + :param job_type: A `BuildFarmJobType` item.
> + :param status: A `BuildStatus` item, defaulting to PENDING.
> + :param processor: An optional processor for this job.
> + :param virtualized: An optional boolean indicating whether
> + this job should be run virtualized.
> + """

I understand that you use status=None because you can't import BuildStatus
here. Seems that interfaces.buildbase isn't that basic ;) I'm wondering
if it is worth to move the definition of BuildStatus into another module
like "realbuildbase" which does not import anything "build related" --
I simply prefer to see a default value status=BuildStatus.PENDING
in the method definition. Even more, because you have this pattern "None
means pending" in two method (new and __init__) in ther implementation.

[...]

> === modified file 'lib/lp/buildmaster/model/packagebuildfarmjob.py'
> --- lib/lp/buildmaster/model/packagebuildfarmjob.py 2010-04-20 14:38:20 +0000
> +++ lib/lp/buildmaster/model/packagebuildfarmjob.py 2010-04-22 07:42:23 +0000
> @@ -26,7 +26,9 @@
> itself a concrete class. This class (PackageBuildFarmJob)
> will also be renamed PackageBuild and turned into a concrete class.
> """
> - super(PackageBuildFarmJob, self).__init__()
> + # Classes that initialise with a build are not yet using
> + # the concrete class, so we don't call the superclass'
> + # initialisation.

Is this a candidate for an XXX, i.e., are you planning to call super()
once the "transition period" is over?