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

Björn Tillenius wrote:
> Review: Needs Information
> 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...
Good catch! I have indented the commit() line above to preserve the old
behaviour.

>> === 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?
The issue above is fixed now.

>> === 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.
Good point! Revised the code accordingly.

>> 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.
Yup. Fixed.

>> 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.
Fixed as well.

> review needsinformation
Please see the enclosed incremental diff.

Best regards

--
Muharem Hrnjadovic <email address hidden>
Public key id : B2BBFCFC
Key fingerprint : A5A3 CC67 2B87 D641 103F 5602 219F 6B60 B2BB FCFC