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

On Fri, Apr 23, 2010 at 12:53 PM, Abel Deuring
<email address hidden> wrote:
> Hi Michael,
>
> a nice branch; just just have a few formal nitpicks.

Thanks Abel, comments below.

>
>> === 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"?

Yep. This was just a copy-n-paste, but you're right. Updated.

>
>
>> +
>> +    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.

Ditto.

>
>> +
>> +    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/

Done.

>
>> +
>> +    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/

Done.

>
> [...]
>
>> @@ -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.

Yes, it will *need* to be moved (as IBuildStatus will be removed
later), and the logical place is actually here with IBuildFarmJob. I'm
not keen to do that move as part of this branch, but funnily, I'm able
to use the default value of BuildStatus.NEEDSBUILD here now. You were
right about why I'd set it to None initially, but I must have resolved
the import issue, as it works fine now. Thanks!

>
> [...]
>
>> === 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?

It is, and the XXX is already included in the docstring (only part of
which you can see here).

Thanks Abel.
>
> --
> https://code.edge.launchpad.net/~michael.nelson/launchpad/db-build-farm-job-model/+merge/23913
> Your team Launchpad code reviewers from Canonical is subscribed to branch lp:launchpad.
>
> --
> launchpad-reviews mailing list
> <email address hidden>
> https://lists.ubuntu.com/mailman/listinfo/launchpad-reviews
>