Code review comment for lp:~wgrant/launchpad/ddeb-domination

Revision history for this message
William Grant (wgrant) wrote :

Hi Māris,

On Tue, 2010-07-27 at 20:26 +0000, Māris Fogels wrote:
> Review: Approve code
> Hi William,

Thanks for the review.

> This is a very good change! At first I was caught thinking
> testJudgeAndDominateWithDDEBs() needed refactoring, but was pleasantly
> surprised to find that you fully tested both DDEB policies in the
> test_publishing module. I guess testJudgeAndDominate is just checking
> that the components are integrated correctly.

Right. I refactored the related code in earlier branches to make this
style of testing possible. I think it worked pretty well.

> The only other question is about _getCorrespondingDDEBPublications():
> can it use the ISlaveStore instead of the IMasterStore? Check
> lib/canonical/launchpad/doc/db-policy.txt for the difference.

This is the sort of thing where it's absolutely critical that we have
up-to-the-second data. Using the slave store would be dangerous.

> Otherwise this looks good to me: r=mars

Thanks!

« Back to merge proposal