Code review comment for lp:~al-maisan/launchpad/ibuilder-api-cleanup-505647

On Mon, Jan 11, 2010 at 03:08:17AM -0000, Muharem Hrnjadovic wrote:
> === modified file 'lib/lp/buildmaster/manager.py'
> --- lib/lp/buildmaster/manager.py 2009-07-26 14:19:49 +0000
> +++ lib/lp/buildmaster/manager.py 2010-01-11 03:08:16 +0000
> @@ -340,17 +340,9 @@
> transaction.commit()
> continue
>
> - candidate = builder.findBuildCandidate()
> - if candidate is None:
> - self.logger.debug(
> - "No build candidates available for builder.")
> - continue
> -
> slave = RecordingSlave(builder.name, builder.url, builder.vm_host)
> - builder.setSlaveForTesting(slave)
> -
> - builder.dispatchBuildCandidate(candidate)
> - if builder.currentjob is not None:
> + candidate = builder.findAndStartJob(buildd_slave=slave)
> + if candidate is not None:
> recording_slaves.append(slave)
> transaction.commit()

Here you changed it to commit when no candidates are found...

> === modified file 'lib/lp/soyuz/scripts/buildd.py'
> --- lib/lp/soyuz/scripts/buildd.py 2009-10-26 18:40:04 +0000
> +++ lib/lp/soyuz/scripts/buildd.py 2010-01-11 03:08:16 +0000
> @@ -201,12 +201,10 @@
> if not builder.is_available:
> self.logger.warn('builder is not available. Ignored.')
> continue
> - candidate = builder.findBuildCandidate()
> +
> + candidate = builder.findAndStartJob()
> if candidate is None:
> - self.logger.debug(
> - "No candidates available for builder.")
> continue
> - builder.dispatchBuildCandidate(candidate)
> self.txn.commit()

... but here you retain the behaviour of not commiting when no candidate
is found. Why was one place changed, but not the other?

> === modified file 'lib/lp/soyuz/tests/test_builder.py'
> --- lib/lp/soyuz/tests/test_builder.py 2009-12-02 15:18:46 +0000
> +++ lib/lp/soyuz/tests/test_builder.py 2010-01-11 03:08:16 +0000

> @@ -34,13 +35,15 @@
> # Create some i386 builders ready to build PPA builds. Two
> # already exist in sampledata so we'll use those first.
> self.builder1 = getUtility(IBuilderSet)['bob']
> - self.builder2 = getUtility(IBuilderSet)['frog']
> + self.frog_builder = removeSecurityProxy(
> + getUtility(IBuilderSet)['frog'])

Why doesn't frog_builder have a security proxy? If it's just to call
private methods, it's better to remove the proxy when you call the
method. If you start passing frog_builder to methods in your tests, you
might get things passing in the tests, while failing in real code.
Better to be safe and only unwrap the objects when you know it's safe.

> self.builder3 = self.factory.makeBuilder(name='builder3')
> - self.builder4 = self.factory.makeBuilder(name='builder4')
> + self.builder4 = removeSecurityProxy(
> + self.factory.makeBuilder(name='builder4'))

Same comment here.

> self.builder5 = self.factory.makeBuilder(name='builder5')
> self.builders = [
> self.builder1,
> - self.builder2,
> + self.frog_builder,
> self.builder3,
> self.builder4,
> self.builder5,
> @@ -62,7 +65,8 @@
> self.publisher.prepareBreezyAutotest()
>
> self.bob_builder = getUtility(IBuilderSet)['bob']
> - self.frog_builder = getUtility(IBuilderSet)['frog']
> + self.frog_builder = removeSecurityProxy(
> + getUtility(IBuilderSet)['frog'])

And here.

    review needsinformation

--
Björn Tillenius | https://launchpad.net/~bjornt