Launchpad itself

Merge lp://staging/~wgrant/launchpad/refactor-slavestatus into lp://staging/launchpad

refactor-slavestatus
Merge into devel

Proposed by William Grant on 2010-03-27

Status:

Merged

Approved by:

Brad Crittenden on 2010-04-01

Approved revision:

no longer in the source branch.

Merged at revision:

not available

Proposed branch:

lp://staging/~wgrant/launchpad/refactor-slavestatus

Merge into:

lp://staging/launchpad

Diff against target:

215 lines (+51/-70)

7 files modified

lib/lp/buildmaster/interfaces/buildfarmjobbehavior.py (+6/-4)
lib/lp/buildmaster/model/builder.py (+11/-2)
lib/lp/buildmaster/model/buildfarmjobbehavior.py (+2/-2)
lib/lp/code/model/recipebuilder.py (+11/-23)
lib/lp/soyuz/model/binarypackagebuildbehavior.py (+11/-24)
lib/lp/translations/model/translationtemplatesbuildbehavior.py (+4/-14)
lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py (+6/-1)

To merge this branch:

bzr merge lp://staging/~wgrant/launchpad/refactor-slavestatus

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Brad Crittenden (community)	code	2010-03-27	Approve on 2010-03-30
Review via email: mp+22285@code.staging.launchpad.net

Commit message

Factor out common IBuildFarmJobBehavior.slaveStatus behaviour into Builder.slaveStatus.

Description of the change

This branch shuffles common things from IBuildFarmJobBehavior.slaveStatus implementations up into Builder.slaveStatus. In particular build_id, build_status and logtail (all set by the base class on the slave) are extracted for all job types if present, in addition to builder_status which has always been extracted there. This vastly simplifies the slaveStatus methods on the IBFJBs.

Revision history for this message

Brad Crittenden (bac) wrote on 2010-03-30:

William thanks for the fix. The logic of what you have done looks good.

However, I don't like the idiom you've employed of mutating the status dictionary inside the slaveStatus methods and then returning it. If you didn't want to mutate the original dictionary then making a copy, changing it, and returning that copy would make sense. Otherwise just modifying the status dictionary in place and not returning it is a reasonable thing to do (and preferred barring no solid reason to do the former). But to mutate and then return would lead one to think the dictionary was not modified in place, which we know it was.

Changing the method to mutate will necessitate a small change to your new test code.

I'm marking this approved rather than needs fixing with the expectation you'll make that change. If you would prefer to discuss it further we can and I'd be eager to hear your thoughts.

As usual, I'm happy to land this for you when it is ready.

review: Approve (code)

Revision history for this message

William Grant (wgrant) wrote on 2010-03-31:

> William thanks for the fix. The logic of what you have done looks good.
>
> However, I don't like the idiom you've employed of mutating the status
> dictionary inside the slaveStatus methods and then returning it. If you
> didn't want to mutate the original dictionary then making a copy, changing it,
> and returning that copy would make sense. Otherwise just modifying the status
> dictionary in place and not returning it is a reasonable thing to do (and
> preferred barring no solid reason to do the former). But to mutate and then
> return would lead one to think the dictionary was not modified in place, which
> we know it was.

Thanks for the review, Brad. I was a little concerned about that too, but wasn't
entirely sure of the cleanest solution. I've adopted your suggestion, and also
renamed the method to updateSlaveStatus to more accurately reflect its purpose.

If you approve, please land this once PQM opens.