Hi Michael.  These look like nice fixes.

I asked some questions on IRC to which you replied to my satisfaction.  I'll include the log beneath for reference.

I did ask you to consider whether the duplicated code for private setup in xx-source-package-publishing.txt and xx-binary-package-publishing.txt (">>> from lp.registry.interfaces.distribution import IDistributionSet...") should be factored into a testing helper.  You thought this was a reasonable idea, which is great by me. :-)

Thank you!

Gary

Edited IRC log:

gary_poster: noodles775: why did you remove test_login.py? 
noodles775: gary_poster: I didn't... a local diff between this pipe and the prev shows everything but the test_login.py.
noodles775: I was just trying to figure out what was going on with the MP.
gary_poster: noodles775: ok, I'll just clarify in the review that merging that removal is not ok, and leave you to handle the mechanism 
noodles775: gary_poster: I'll merge RF into the initial pipe (there are 7 in total) and push it through and see if it's still there when the diff regenerates on the MP.
gary_poster: noodles775: ack, ok
salgado: gary_poster, noodles775, in my openid branch I added a test_new_login.py file and later removed test_login.py.  then on a subsequent branch I renamed test_new_login.py to test_login.py.  that, together with pipes, might have confused  bzr?
noodles775: salgado: thanks for the info - I thought it may have been related to your branch I reviewed yesterday. If that's the case, pumping a fresh RF through my pipes should resolve the issue. (I hope)
gary_poster: noodles775: in xx-private-builds.txt, why is it OK to remove diff line 375ff? ("Now visit the same page as an admin we can see that there are four more...")
gary_poster: thanks salgado
gary_poster: noodles775: also, in the previous section of the same file, why did the result change (5 of 17 results, and different values)?  If explaining it takes too long, I'll accept "because of changes that have previously passed review"
gary_poster: but I'd like reassurance that you are confident of the change, at least 
gary_poster: hm, I could understand an increase of 1, since we added the private p3a archive, but I don't understand 14 -> 17
noodles775: gary_poster: so, regarding your first question, 375ff, the test for authorised viewers seeing the builds in the history was moved to just below with the Frog builder.
noodles775: It looks like a lot less test-code because the Frog builder only has the one build in its history (it's not polluted with sample data :)).
gary_poster: noodles775: ah ok cool thanks
noodles775: gary_poster: and your second question is related, because previously cprov's ppa was switched to private (which is no longer allowed), and had a bunch of publishings.
noodles775: So the 14 -> 17 is because there are 3 packages in cprov's public archive (when it was private ,they did not show up).
Topic changed to "on call: adeuring,gary_poster || reviewing: ?,noodles775, sinzui || queue [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews" by sinzui.
noodles775: s/packages/builds of course.
gary_poster: noodles775: we dupe the private setup in xx-source-package-publishing.txt and xx-binary-package-publishing.txt (">>> from lp.registry.interfaces.distribution import IDistributionSet...").  You considered and rejected a helper function?  Two copies is arguable, maybe.  Three, much less so. 
gary_poster: So if you considered and rejected then I'm OK, but otherwise, please consider 
noodles775: gary_poster: no I didn't consider it - sounds good.
gary_poster: ok cool noodles775
gary_poster: noodles775: I'm finishing the review, but merge-conditional on not landing the deletion of test_login.py
gary_poster: I mean, I'm writing it up
gary_poster: but it is r=gary with that
noodles775: gary_poster: thanks. Actually, if you re-load the MP you'll see that it's no longer there (pumping a fresh RF through seems to have helped)
gary_poster: noodles775: confirmed, awesome