Code review comment for lp:~michael.nelson/launchpad/567922-binarypackagebuild-new-table-2

Hi Michael,

Man I know you're going to be happy to get these changes all wrapped
up. Thanks for doing a very tedious series of branches.

I'm glad to see soyuzvariablenames expanded. thank_you.

--bac

> === modified file 'lib/lp/buildmaster/interfaces/packagebuild.py'
> --- lib/lp/buildmaster/interfaces/packagebuild.py 2010-05-04 12:07:35 +0000
> +++ lib/lp/buildmaster/interfaces/packagebuild.py 2010-05-04 15:29:42 +0000
> @@ -151,6 +151,13 @@
> :param slave_status: A dict as returned by IBuilder.slaveStatus
> """
>
> + def queueBuild(suspended=False):
> + """Create a BuildQueue entry for this build.
> +
> + :param suspended: Whether the associated `Job` instance should be
> + created in a suspended state.
> + """

So interface method specifications leave off the 'self' argument but
as a staticmethod there is no self, so the first argument 'build' is
dropped? That's no good.

>
> class IPackageBuildSource(Interface):
> """A utility of this interface used to create _things_."""

> === modified file 'lib/lp/buildmaster/model/buildfarmjob.py'
> --- lib/lp/buildmaster/model/buildfarmjob.py 2010-05-04 13:42:25 +0000
> +++ lib/lp/buildmaster/model/buildfarmjob.py 2010-05-04 15:29:42 +0000
> @@ -189,7 +189,7 @@
> def __init__(self, job_type, status=BuildStatus.NEEDSBUILD,
> processor=None, virtualized=None):
> super(BuildFarmJob, self).__init__()
> - self.job_type, self.status, self.process, self.virtualized = (
> + self.job_type, self.status, self.processor, self.virtualized = (
> job_type,
> status,
> processor,

> === modified file 'lib/lp/soyuz/model/binarypackagebuild.py'
> --- lib/lp/soyuz/model/binarypackagebuild.py 2010-05-04 09:45:26 +0000
> +++ lib/lp/soyuz/model/binarypackagebuild.py 2010-05-04 15:29:42 +0000

> @@ -778,7 +791,7 @@
>
> # This code MUST match the logic in the Build security adapter,
> # otherwise users are likely to get 403 errors, or worse.

If it MUST match then perhaps it should be refactored and shared?
(Not a requirement for this branch.)

> === renamed file 'lib/lp/soyuz/tests/test_build.py' => 'lib/lp/soyuz/tests/test_binarypackagebuild.py'
> --- lib/lp/soyuz/tests/test_build.py 2010-04-29 08:28:30 +0000
> +++ lib/lp/soyuz/tests/test_binarypackagebuild.py 2010-05-04 15:29:42 +0000
> @@ -7,6 +7,7 @@
>
> from storm.store import Store
> from zope.component import getUtility
> +from zope.security.proxy import removeSecurityProxy
>
> from canonical.database.constants import UTC_NOW
> from canonical.testing import LaunchpadZopelessLayer
> @@ -81,7 +82,7 @@
>
> [depwait_build] = depwait_source.createMissingBuilds()
> depwait_build.buildstate = BuildStatus.MANUALDEPWAIT
> - depwait_build.dependencies = 'dep-bin'
> + depwait_build.dependencies = u'dep-bin'

Why did you need to convert these strings to unicode?