#launchpad-reviews 2010-04-26
<thumper> hello mister reviewer: https://code.edge.launchpad.net/~thumper/launchpad/really-send-reviewer-email-to-teams/+merge/24094
<thumper> mwhudson: a simple branch that actually sends code review email to team reviewers
<mwhudson> thumper: looking
<thumper> mwhudson: good news
<mwhudson> thumper: oh?
<thumper> just QAed my scanner change
<thumper> and it records the merged revno
<thumper> \o/
<mwhudson> thumper: \o/
<thumper> for merged proposals
<thumper> hows the branch you are preparing for me going?
<mwhudson> thumper: i think i just need to push it up and propose them
<mwhudson> thumper: there's three pipes left :)
<mwhudson> two are small though
<thumper> ok
<thumper> I'm spending time looking into the core publication mechanism
<thumper> I'm trying to make testing breadcrumbs easier
<thumper> gary thinks it is too hard to do
<thumper> but I don't take no for an answer
<thumper> I just haven't found the right place to hook in
<thumper> ha
<thumper> I think I found it
<thumper> nope, not yet
<thumper> ah, now I've found it
<thumper> now to disect
<thumper> or should I say, later to disect
<thumper> mwhudson: thanks for the review
 * thumper afk for a bit
<thumper> mwhudson: I'm going to attempt to use a gym without killing myself or pulling any muscles
<thumper> mwhudson: I'll get to your branches later this afternoon/tonight
<mwhudson> thumper: good luck with that
<mwhudson> thumper: sounds fine
<thumper> mwhudson: I'll make sure they're all reviewed for tomorrow morning
<mwhudson> thumper: awesome
<thumper> mwhudson: will this make everything go? all tests pass?
<mwhudson> thumper: all tests passed on friday night, i guess changes in db-devel may have broken something
<mwhudson> but i doubt it
<thumper> hopefully not
<thumper> I found the code I need to tackle for the breadcrumb work
<thumper> I should have read salgado's comment as he mentions it in the code :)
* noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [adiroiban(UI bug-146178), noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: Hi! Will you be around today for reviews? Otherwise I'll try to organise a swap :)
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: -  || queue: [adiroiban(UI bug-146178), noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: noodles775  || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> noodles775: i think that the description of a boolean should take the form of a question, so instead of "Whether the build should be treated as private." I'd do "Should the build be treated as private?"
<noodles775> intellectronica: ok.
<intellectronica> noodles775: what do you think? i don't care deeply about this, just wanted to mention.
<noodles775> intellectronica: Although I personally prefer the first option, I'm not fussed either way, so would just go with whatever the defacto standard is.
<intellectronica> âânoodles775: on line 395 of the diff there's a trailing , which i think you can remove
<noodles775> Aha.
<intellectronica> noodles775: if you prefer the first option keep it
<intellectronica> noodles775: r=me
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: -  || queue: [adiroiban(UI bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> intellectronica: thanks.
<bac> intellectronica: can i add a very small one to your queue?  https://code.edge.launchpad.net/~bac/launchpad/bug-524778-2/+merge/24060
<intellectronica> bac: if it's very small i'll do it right now
<bac> intellectronica: itty bitty
<intellectronica> bac: r=me
<bac> thanks tom
* intellectronica changed the topic of #launchpad-reviews to: On call: intellectronica || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> hi.  Could I get a review for https://code.edge.launchpad.net/~gary/launchpad/bug562828/+merge/24044 ?
<abentley> gary_poster, sure.
<gary_poster> Thank you abentley
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, gary  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> gary_poster, It seems like this solution would be problematic if the librarian could ever contain symlinks.  That's unlikely, right?
<gary_poster> abentley: you mean, if its purpose were to (occasionally) store symlinks, rather than it being merely organized with symlinks?
<abentley> gary_poster, right.
<gary_poster> Yeah, that's unlikely.  I'll be happy to get a more confident answer from flacoste.
<abentley> gary_poster, when I first read "tree", I thought you were talking about the launchpad source tree, which does have symlinks, and thought we were storing the contents of the source tree in the librarian.
<flacoste> gary_poster, abentley: the librarian doesn't create or manage symlinks
<flacoste> gary_poster, abentley: now if an admin does that...
<flacoste> don't :-)
<gary_poster> :-)
<abentley> gary_poster, approved.
<gary_poster> thank you abentley
* abentley changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> intellectronica, Do you have time to review my branch at https://code.edge.launchpad.net/~matthew.revell/launchpad/bug-heat-help-bug-544799/+merge/24016
<intellectronica> mrevell: sure
<intellectronica> mrevell: r=me. don't forget that the actual heat changes are in db-devel, so you might want to delay landing this branch until the end of the cycle
<intellectronica> mrevell: or even better, make it a db-devel branch (which will require you to only merge in the relevant changes and not all the revisions from devel preceding it).
<mrevell> intellectronica, So, do I just land it on db-devel or do I need to re-branch from db-devel?
<mrevell> Thanks for the r=intellectronica, btw :)
* adiroiban changed the topic of #launchpad-reviews to: On call: intellectronica, abentley || reviewing: -, -  || queue: [adiroiban(code bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<intellectronica> mrevell: you need to re-branch from db-devel and then merge into your new branch only the changes relevant to this bug from the existing branch
<mrevell> thanks intellectronica
<bac> intellectronica, abentley: i have another insanely easy branch:  https://code.launchpad.net/~bac/launchpad/bug-568659/+merge/24131
<intellectronica> bac: i'll take it
<bac> intellectronica: pace yourself!  :)
<intellectronica> bac: r=me
* intellectronica changed the topic of #launchpad-reviews to: On call: abentley || reviewing: -  || queue: [adiroiban(code bug-146178)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* adiroiban changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [sinzui, adiroiban(code,bug-146178),adiroiban(code,bug-525992)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-04-27
* noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [sinzui, adiroiban(code,bug-146178),adiroiban(code,bug-525992), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> henninge: another one for you if/when you've time :)
<henninge> noodles775: um, there is quite a queue and I am not ready yet ...
<henninge> noodles775: you can append it and I will see if I can get to it.
<henninge> I need to finish some urgent QA first, sorry.
<noodles775> henninge: yeah, no worries, I've already got in on the queue for whoever gets there first. Good luck with the QA!
<henninge> another staging restore .... might as well do reviews  ;)
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: sinzui  || queue: [adiroiban(code,bug-146178),adiroiban(code,bug-525992), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: adiroiban(code,bug-525992)  || queue: [adiroiban(code,bug-146178), noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> sinzui's queue entry seems to have been stale.
* bigjools changed the topic of #launchpad-reviews to: On call: henninge || reviewing: adiroiban(code,bug-525992)  || queue: [adiroiban(code,bug-146178), noodles, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: On call: henninge || reviewing: adiroiban(code,bug-525992)  || queue: [adiroiban(code,bug-146178), noodles, bigjools, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: henninge,bac || reviewing: adiroiban(code,bug-525992)  || queue: [adiroiban(code,bug-146178), noodles, bigjools, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: henninge,bac || reviewing: adiroiban(code,bug-525992),adiroiban(code,bug-146178)  || queue: [noodles, bigjools, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> morning henninge.  how's it going?
<henninge> bac: Hi! Good, thanks.
<henninge> bac: but I won't have a great OCR throughput today ....
<bac> henninge: that's ok
* bac changed the topic of #launchpad-reviews to: On call: henninge,bac || reviewing: adiroiban(code,bug-525992),noodles  || queue: [bigjools, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<BjornT> abentley_: ping
<abentley> BjornT, pong.
<BjornT> abentley: about dailybuilds-archive. do i understand correctly, if i think that the archive column will be used only for daily builds?
<abentley_> BjornT, correct.  When we do a daily build, we will use SourcePackageRecipe.archive as value for SourcePackageRecipeBuild.archive
<BjornT> abentley_: how about naming it daily_build_archive to make that clear?
<abentley_> BjornT, we might also like to use it as the default when doing a manual build.
<abentley> BjornT, but if it gets it though review faster, I can change the name.
<BjornT> abentley: well, it's mostly that i'd like us to have an idea of what things will be used for. the name should make this clear, so that it's easier to understand from reading the code.
<cody-somerville> Hi :)
<cody-somerville> Can I get some help with my branch? lp:~cody-somerville/launchpad/bug-444266/
<cody-somerville> I created a IHasBugSupervisorEditSchema class to provide an editable field for bug_supervisor in IHasBugSupervisorEditView since bug_supervisor is defined as readoly in the IHasBugSupervisor interface.
<BjornT> abentley: if you're ok with naming it daily_build_archive, that's ok, though. that shows a clear use, and if you find different use cases, it will be more obvious that you should rename it.
<cody-somerville> Similar to what BranchEditSchema does to make the private field editable.
<cody-somerville> However, I get TypeError: ('Could not adapt', <Distribution 'Ubuntu' (ubuntu)>, <InterfaceClass lp.bugs.browser.bugsupervisor.BugSupervisorEditSchema>)
<cody-somerville> and TypeError: ('Could not adapt', <Product at 0xf7dad50>, <InterfaceClass lp.bugs.browser.bugsupervisor.BugSupervisorEditSchema>)
<cody-somerville> :)
<henninge> noodles775, danilo_: Can you please have a look at the first paragraph of my review for Adi and comment *if you object*? Thanks. ;) https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525992/+merge/23285
* henninge changed the topic of #launchpad-reviews to: On call: bac || reviewing: noodles  || queue: [bigjools, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bac: I am done. ;)
<bac> have a good evening henninge
<henninge> thanks
<noodles775> henninge: I think your suggestion is a good one (stating the mode without "Enter")..
<henninge> noodles775: ok, thanks ;)
<henninge> noodles775: I was just thinking it could even be "You are in ... mode"
<danilo_> henninge, I concur :)
<henninge> cool
<cody-somerville> I'm confused.
<cody-somerville> Who is reviewing? bac or noodles775?
<bac> on call: bac
<bac> what is bac doing?
<bac> reviewing: noodles
<bac> clearer cody-somerville?  :)
<cody-somerville> bac is reviewing noodles?
<bac> indeed
<cody-somerville> noodles775, thats not clear at all :P
<bac> do you need a review cody-somerville?
<cody-somerville> err..
<cody-somerville> (sorry, auto complete)
<cody-somerville> bac, Yes please :)
<cody-somerville>  I created a IHasBugSupervisorEditSchema class to provide an editable field for bug_supervisor in IHasBugSupervisorEditView since bug_supervisor is defined as readoly in the IHasBugSupervisor interface.
<cody-somerville> Similar to what BranchEditSchema does to make the private field editable.
<cody-somerville> However, I get TypeError: ('Could not adapt', <Distribution 'Ubuntu' (ubuntu)>, <InterfaceClass lp.bugs.browser.bugsupervisor.BugSupervisorEditSchema>)
<bac> cody-somerville: tbh you're the first person to say it has confused them in the 2 years we've used this format.
<cody-somerville> and TypeError: ('Could not adapt', <Product at 0xf7dad50>, <InterfaceClass lp.bugs.browser.bugsupervisor.BugSupervisorEditSchema>)
<cody-somerville> lp:~cody-somerville/launchpad/bug-444266/
<cody-somerville> bac, Maybe it would be more clear if it listed a branch instead of a person
<cody-somerville> bac, But to me the current topic suggests that noodles775 is reviewing and you're 'on call' to help if needed.
<bac> cody-somerville: things to be reviewed show up at https://code.edge.launchpad.net/launchpad/+activereviews
<bac> so if noodles only has one branch ready to be reviewed there is no ambiguuity
<bac> cody-somerville: have you created a merge proposal?
<cody-somerville> bac, No. My branch doesn't work so I didn't create one.
<bac> ah, ok
 * bac looks at branch
<bac> hi noodles775
<bac> cody-somerville: how do i reproduce those errors?
<cody-somerville> ./bin/test -vvvc -tbug-release-management
* mrevell changed the topic of #launchpad-reviews to: On call: bac || reviewing: noodles  || queue: [bigjools, StevenK, mrevell] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* salgado changed the topic of #launchpad-reviews to: On call: bac || reviewing: noodles  || queue: [bigjools, StevenK, mrevell,salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi bac
<bac> cody-somerville: you need an adapters property on the view.  look at how it was done for BranchEditSchema
<cody-somerville> bac, okay, I must have missed that
<cody-somerville> bac, I'll take another look
<bac> noodles775: ping
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: stevenk  || queue: [bigjools, salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> hi bac
<bac> hi noodles775.  bad timing -- i thought you'd gone so i just marked your MP 'abstain'
* salgado changed the topic of #launchpad-reviews to: On call: bac || reviewing: stevenk  || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<leonardr> bac, can i add https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/retry_on_502/+merge/24252 to the queue?
<bac> leonardr: yes
<leonardr> cool
* leonardr changed the topic of #launchpad-reviews to: On call: bac || reviewing: stevenk  || queue: [bigjools,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: leonardr  || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> leonardr: inexplicably your MP is private again.
<leonardr> bac: fixing
<leonardr> bac: i don't see any 'public' flag for mps. is it possible you don't belong to lazr-developers??
<leonardr> i've asked you specifically for a review
<leonardr> so you should be able to see it now
<bac> actually i could see it before but noticed it was private
<bac> now i cannot see it at all
<leonardr> aaaargh
<leonardr> if only i knew how to use this launchpad thing
<bac> and i am in ~lazr-developers as are all of ~launchpad
<leonardr> brad: ah, the _branch_ is private
<bac> odd, the MP moved URLs when you changed it.  it now lives at https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/retry_on_502/+merge/24257
<leonardr> bac: try again, i made itp ublic
<bac> how'd you do that?
<bac> leonardr: what is the purpose of the BrokenApplication class in your test?
<leonardr> bac: i think that might not be necessary anymore
<bac> leonardr: me too
<leonardr> yeah, i tried to do it as a class and decided it was easier as a function
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: -  || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<cody-somerville> bac, woot. ty.
<bac> np
<cody-somerville> bac, Now, where would you recommend I add tests to set security contact and bug supervisor via API?
<bac> cody-somerville: we cannot yet do automated tests using launchpadlib but will be able to very soon now
<bac> until then, we use the web service directly, without the lplib wrapper
<bac> cody-somerville: look in lp/bugs/stories/webservices
<cody-somerville> bac, right right
<cody-somerville> bac, but there is no doctest in there yet for this sort of thing
<cody-somerville> bac, I'm looking for a good name to use for new doctest
<bac> cody-somerville: i'm not familar with how the bug tests are organized right now. if they don't fit in one of the existing tests then just use your best judgment
<adiroiban> bac: hi. I have create a new MP for bug 146178 and this one is targeted to devel (not db-deve). https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/24265
<mup> Bug #146178: Add links to latest full and delta language pack exported <ui> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/146178>
<adiroiban> Should I get a new UI review?
#launchpad-reviews 2010-04-28
<cody-somerville> bac, https://code.edge.launchpad.net/~cody-somerville/launchpad/bug-444266/+merge/24273
<mwhudson> thumper: https://code.edge.launchpad.net/~mwhudson/launchpad/unregister-bzr-git/+merge/24279 ?
<mwhudson> thumper: um, please insert "can you review" in there
<thumper> :)
<thumper> done
* wgrant changed the topic of #launchpad-reviews to: On call: bac || reviewing: -  || queue: [bigjools,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [bigjools,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mwhudson> thumper: another one? https://code.edge.launchpad.net/~mwhudson/launchpad/code-import-workers-copy-too-much/+merge/24282
<thumper> mwhudson: I'm almost done with breadcrumbs
<thumper> mwhudson: want to review
<thumper> ?
<mwhudson> thumper: ok
<thumper> mwhudson: why does reading other teams code make me want to cry?
<mwhudson> thumper: i don't knpw
<thumper> distroseries.hide_all_translations = BoolCol...
<mwhudson> !!
<mwhudson> thumper: hang on, why is that so bad?
<mwhudson> apart from being sqlobject-y
<thumper> they weren't setting it to False in their tests
<mwhudson> oh
<thumper> just awaiting diff
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/code-import-machine-breadcrumb/+merge/24286
<thumper> mwhudson: the generated diff is missing the last commit
<thumper> mwhudson: which deletes an obsolete method
<mwhudson> thumper: i was grabbing the branch to look at in meld anyway
<thumper> mwhudson: ok
<thumper> the branch has been pushed
<thumper> I have my guitar lesson soon, so I'm afk
<mwhudson>             ok
<thumper> stub: review poke for abentley
<stub> k
* noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: -  || queue: [bigjools,wgrant,noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gmb> stub, Thanks for the review; I've replied and updated the patch.
<stub> k
<stub> gmb: have you pushed?
<gmb> stub, Hahahaaha. Durr.
<gmb> stub, Have pushed now.
<gmb> This may be a sign that I have too much blood in my caffeine system.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: -  || queue: [bigjools,wgrant,noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: ready for review at last... would you mind?
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-569108/+merge/24297
<noodles775> jtv: not at all, can we swap?
<noodles775> (I'd appretiate your thoughts on the refactoring branch I've got. It's a bit confusing as its one in a pipeline).
<jtv> noodles775: sure...  I think I'll relocate away from this road my embassy told me to stay away from, and I can read your branch on the way
<noodles775> jtv: thanks. It doesn't need to be today (given the time etc.)
<jtv> noodles775: actually I was _supposed_ to OCR today, so might as well make a decent effort
<noodles775> :)
<jtv> (Actually it's a bit insane for my embassy to warn me away from this road; it goes all the way to the next city, and the battle happened miles away & hours ago)
<noodles775> sheesh
<jtv> They keep doing that: "Bombs went off in X and Y sometime before your family halfway around the world asked you about them.  We recommend that you stay away from bombs."
<jtv> There's just no keeping up with the electronic grapevine.  :)
<noodles775> heh
<jtv> Speaking of grapevine: word is only one dead, soldier, friendly fire.
<jtv> Confusion because of the rain.  Maybe they got a little trigger-happy with those flashy new Israeli assault rifles I saw them with... bullpup, so easy to point the wrong way.
<jtv> And in other news: my girlfriend managed to get out of there.  :-)
<noodles775> Some good news then!
<jml> hello
<jml> https://code.edge.launchpad.net/~jml/launchpad/more-tests-for-ec2/+merge/24309 -- easy peasy review
* jml changed the topic of #launchpad-reviews to: On call: allenap || reviewing: -  || queue: [bigjools,wgrant,noodles,jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: bigjools || queue: [bigjools,wgrant,noodles,jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: wgrant || queue: [wgrant,noodles,jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: noodles || queue: [jml] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> BjornT, I made the name change you requested to the database patch.  If that's sufficient, could you mark it approved, please?
<BjornT> abentley: yes of course, it was already conditionally approved, even though it wasn't marked approved
<abentley> BjornT, Ah.  I thought so, but I wanted to make sure.
<allenap> noodles775: I'm looking at 567922-binarypackagebuild-packagebuild-2. Is it worth getting this reviewed by someone who knows this area? I'm happy to continue, but I'm probably not going to be able to give you more than a syntax and style check.
<noodles775> allenap: yeah, I chatted with jtv earlier and he said he'd be happy to take a look, so feel free to leave it with him.
<allenap> noodles775: Okay, cool.
* allenap changed the topic of #launchpad-reviews to: On call: allenap || reviewing: jml || queue: [noodles] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* noodles775 changed the topic of #launchpad-reviews to: On call: allenap || reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: allenap, Edwin || reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* allenap changed the topic of #launchpad-reviews to: On call: allenap, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<cody-somerville> allenap, Would you be willing to review https://code.edge.launchpad.net/~cody-somerville/launchpad/bug-444266/+merge/24273 ?
<allenap> cody-somerville: Yeah, sure :)
* allenap changed the topic of #launchpad-reviews to: On call: allenap, Edwin || reviewing: cody-somerville, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> rockstar, could you please review https://code.launchpad.net/~abentley/launchpad/fix-build-time-display/+merge/24263 ?
<rockstar> abentley, on it
<abentley> rockstar, thanks.
<adiroiban> danilos: hi, whan you have time, can you take a quick look over the changes from the branch attached to bug 548375
<mup> Bug #548375: Rosetta import empty strings (e.g. "") from .po files <rosetta-imports> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/548375>
<rockstar> abentley, I'm not sure TestSourcePackageRecipeBuildView should inherit from BrowserTestCase - TestCaseWithFactory is probably fine.
<rockstar> I say this because it doesn't actually use a browser, but just instantiates a raw view.
<abentley> rockstar, Okay.
<rockstar> abentley, r=me
<danilos> heh, a thought occurred to me: shall we consider moving x-es for closing pop-ups in LP to the left side? :)
<rockstar> danilos, I wondered that while working on the overlay yesterday.  :)
<maxb> eww
<maxb> No, leave the Xes in the right place :-)
<maxb> pun intentional
<danilos> rockstar, abentley: hi, do you guys know if it'd be safe to upgrade bzr-git to the latest lp:~launchpad-pqm/bzr-git/devel revision? If so, can you give me an ok on https://code.edge.launchpad.net/~danilo/launchpad/bug-570728/+merge/24340?
<rockstar> danilos, I saw that bug you commented on.
<danilos> rockstar, cool, can you let me know if it's ok to merge the change?
<rockstar> danilos, do you know if this will actually fix your issue?  I don't see jelmer confirm that in the bug.
<danilos> rockstar, I don't know how to check but jelmer did say on IRC that this is a fix for that bug
<rockstar> danilos, okay.
<danilos> rockstar, well, that "he committed a fix on bzr-git tree"
<danilos> jelmer, is there a way I can check latest bzr-git fixes it for the branch from the bug 570728?
<mup> Bug #570728: Importing git repo from http://git.participatoryculture.org/miro/ fails <Bazaar Git Plugin:Fix Released by jelmer> <Launchpad Bazaar Integration:In Progress by danilo> <https://launchpad.net/bugs/570728>
<rockstar> danilos, r=me
* allenap changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<danilos> rockstar, thanks
<danilos> rockstar, just one more question before I head home: is it possible to QA this on staging later? (i.e. are there imports happening on staging, or is it just a simple script run once I set it up in the UI)
<rockstar> danilos, yes, it can be QA'd on staging. I remember last time I needed to do it, it took some LOSA help, but that was a while ago.
<danilos> rockstar, cool, thanks
<jelmer> danilos: sorry, was off to the shops. are you still there?
<danilos> jelmer, yeah, otp though
<jelmer> danilos: other than QA'ing you can use bzr-git locally to test if it works
<danilos> jelmer, how to best test it?
<danilos> jelmer, (i.e. I see bzr-receive-pack and bzr-upload-pack and have no idea what to do with them)
<jelmer> danilos: bzr branch lp:~launchpad-pqm/bzr-git/devel ~/.bazaar/plugins/git; bzr branch http://git.participatoryculture.org/miro/ miro
<danilos> jelmer, ah :)
<jelmer> danilos: assuming you have the right version of dulwich installed
<bac> hi EdwinGrubbs, could you review https://code.edge.launchpad.net/~bac/launchpad/bug-561586/+merge/24355
<EdwinGrubbs> bac: The bug doesn't mention this, but at some point "lp." needs to be prepended to the argument to YUI.add() and Y.namespace(). Of course, that is a lot more work, because you have to change all the imports. If you don't want to do that now, we should make sure there is a bug about that.
<bac> EdwinGrubbs: it seems like not that much work.  i'll do it now and add a note to the current bug
<EdwinGrubbs> bac: I don't have any other comments about your current diff.
<bac> thanks EdwinGrubbs.  i'll get back to you shortly with the incremental
<bac> EdwinGrubbs: i've made the namespace changes to https://code.edge.launchpad.net/~bac/launchpad/bug-561586/+merge/24355 and included an incremental diff
<bac> EdwinGrubbs: could you review it please?
<EdwinGrubbs> sure
<bac> hi sinzui
<bac> sinzui: you reviewed and approved the ui for adi for this MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/23760
<bac> sinzui: i pointed out to him it was incorrectly targeted to db-devel so he resubmitted to https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/24265
<bac> sinzui: could you mark the latter one UI approved?
<EdwinGrubbs> bac: r=me
<bac> thanks EdwinGrubbs
<sinzui> done
<flacoste> EdwinGrubbs: care to review https://code.edge.launchpad.net/~flacoste/launchpad/cleanup/+merge/24367
<flacoste> or anyone else
<flacoste> it's a pure-removal branch
<EdwinGrubbs> flacoste: sure
<EdwinGrubbs> flacoste: r=me
#launchpad-reviews 2010-04-29
<jpds> EdwinGrubbs: I have https://code.launchpad.net/~jpds/launchpad/fix-distromirrorstatus-colours/+merge/24378 if you're still around.
<EdwinGrubbs> jpds: what's a url where I can see the color?
<jpds> EdwinGrubbs: Compare: https://edge.launchpad.net/ubuntu/+archivemirrors and https://launchpad.net/ubuntu/+archivemirrors
<EdwinGrubbs> jpds: r=me
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jpds> EdwinGrubbs: Cool, can you please send it to PQM? :)
<EdwinGrubbs> jpds: sure
<thumper> hello friendly reviewer: https://code.edge.launchpad.net/~thumper/launchpad/code-email-permissions/+merge/24390
<thumper> mwhudson: ^^ it is very small
<mwhudson> thumper: looking
<mwhudson> thumper: done
<thumper> ta
<thumper> mwhudson: how about:
<thumper>         Permission to mark a merge proposal as approved checks launchpad.Edit
<thumper>         of the target branch, or membership of the review team on the target
<thumper>         branch.  For import branches launchpad.Edit also checks the registrant
<thumper>         of the code import if there is one, and membership of vcs-imports.  So
<thumper>         if someone is attempting to review something on an import branch, but
<thumper>         they don't have launchpad.Edit but are a member of the review team,
<thumper>         then a check against the code import is done.
<mwhudson> thumper: it almost feels like it needs a diagram :-)
<mwhudson> thumper: but that'll do
<thumper> it probably does
* noodles775 changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [noodles*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jelmer: when you've time, I've got 3 boring refacting branches that could benefit from your eyes :)
<jelmer> noodles775: sure, np :-)
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [noodles*3] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> jtv: so regarding makeJob(), I'm not sure what the issue is? (Given that you can provide your own implementation on TranslationTemplatesBuildJob)
<noodles775> Or you mean the issue is that I haven't done that as part of the branch.
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> noodles775: no, just wanted to let you know that the direction could lead you to clashes with the existing code, is all
<jtv> noodles775: I hadn't realized that it was the intention to allow it to be a no-op
<noodles775> It wasn't the intention to allow it to be a no-op, but derived classes *must* implement it either way.
<noodles775> jtv: all the other derived classes are *Build objects, and so SPRecipeBuild.makeJob() returns an SPRBuildJob.
<noodles775> In your case, you've got a BuildJob object implementing makeJob() (which is unexpected from my end), but simply returning self probably makes the most sense?
<jtv> noodles775: I must say I'm still getting my head around the boundary between builds and buildfarmjobs in your branch
<noodles775> jtv: did you look at http://people.ubuntu.com/~wgrant/launchpad/buildfarm/new-build-model-again.png
<jtv> noodles775: only an older version I think
<noodles775> Basically, BuildFarmJob is just the base information common to all things that run on the buildfarm. But yeah, it is confusing. It'll be helped later when we get to clean up the queue side of things (bug 570939)
<mup> Bug #570939: Replace BPJ and SPRBJ with IBuildFarmJob.job <Soyuz:Triaged> <https://launchpad.net/bugs/570939>
<noodles775> jelmer: Just to be sure, you're looking at this one right? https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-packagebuild-3/+merge/24327
<noodles775> jtv: let me know if you've got other questions or things I can fix up in that branch. Thanks!
<jelmer> noodles775: no, I was looking at -2
<jtv> noodles775: got some waiting on my other machine, hang on
<noodles775> jelmer: That's the one that jtv has already started. Sorry for the confusion... there are still 3 branches left, and the first one is -3.
<jelmer> noodles775: ah, ok - I'll have a look at that one then
<jelmer> noodles775: I noticed that your branches touches on some code that I've recently changed
<jelmer> noodles775: I've reordered some methods in buildbase.py
<noodles775> jelmer: yeah, I expect to be resolving conflicts for the next while :)
<jtv> noodles775: sorry for delays... some nasty distractions
<noodles775> jtv: no problem (just glad you're still there :)).
<jtv> noodles775: l.61 of the diff, lib/lp/buildmaster/interfaces/packagebuild.py: getUploaderCommand... I know this is not your fault but what does "the command to run as the uploader" mean?  That the class is all about running one particular command, and it will be run under the uploader's identity?
<noodles775> Yes, personally i think it might be better named getUploadCommand().
<jelmer> jtv: fwiw I'm landing a branch that removes it (we're importing the uploadprocessor directly now rather than through subprocess)
<noodles775> Even better :)
<jtv> yep!
<jtv> Same for getUploadLeaf?
<jtv> noodles775: also, note that the slave build id has been replaced with the slave build cookie, which has (I hope) no discernible internal structure
<jelmer> jtv: no, just getUploaderCommand and getUploadLogContent
<noodles775> jtv: also, you're right about the makeJob - in that the docstring implies that its sole purpose is to create the services job, rather than return the 'specific_job' which links the services job. I'll update it and add a :return:.
<jtv> noodles775: thanks...  yes, and since the implementation wasn't in the diff...
<jtv> jelmer: shame, because its interface entry could do with a healthy dose of violent renewal as well
<jelmer> jtv: agreed
<jtv> noodles775: the docstring for getUploadLeaf I find confusing tooâstarting with the "Upload" part of the name.  "Build things" will be stored there...  for the purpose of upload?
<jtv> Discuss.
<noodles775> jtv: yeah, as noted, I didn't even read the docstrings - I was just replicating the interface. But getUploadDirName (cf. with getUploadDir)?
<noodles775> "Return the name of the directory where the uploaded files will be stored."?
<jtv> noodles775: step in the right direction, though I do think that Leaf is good for explaining the assumed directory structure.
<jtv> noodles775: uploaded files, or files to be uploaded?  I'm not familiar with this stuff to be sure.
<noodles775> Right, good point. I assume it's the files to be uploaded.
<jtv> noodles775: might as well get some clarity into these murky depths.  :)
<jtv> noodles775: ready for more?
<noodles775> jtv: yep... I'm just renaming/updating the docstrings, but can do that while we chat :)
<jtv> noodles775: ok... did you get the note about slave build ids not existing any more?
<jtv> One of the docstrings describes their usual structure, but that no longer applies
<jtv> noodles775: (also, that diagram you sent me still has arrows pointing the wrong way afaics)
<noodles775> BTW: just checked, it is actually the location where the uploaded files will be stored (ie. /incoming/...)
<noodles775> jtv: I did, but I wasn't updating the interface... but will, did you decide on a var name? buildd_cookie?
<jtv> noodles775: I'd say just replace "id" with "cookie."
<jtv> It's basically like free cookies.
<noodles775> Done.
<jtv> noodles775: sorry, more urgent stuff elsewhere.  :)  Where was I?  Oh yes.  In the getUploadLeaf docstring, there's a :param now: that says "now" is the time to use.  But for what?
<jtv> noodles785: In the getUploadLeaf docstring, there's a :param now: that says "now" is the time to use.  But for what?
<noodles785> jtv: "The datetime to use when constructing the leaf directory name."?
<jtv> noodles775: assuming that's actually true, perfect, thanks.  (And that goes to show that this has actual information value :-)
<noodles775> jtv: should I push these changes: http://pastebin.ubuntu.com/424513/
 * jtv looks
<jtv> noodles775: is there a more formal way of saying "specific job" in the makeJob docstring?
<noodles775> Hmm... "The object linking the services job with this build farm job."?
 * noodles775 looks forward to 'specific_job' dying a slow death.
<jtv> :)
<jtv> noodles775: but that's saying "object," which makes it seem even vaguer somehow...
<jtv> (Also, line 102 of that pastebin still mentions "build id."  Apart from that, it looks good to me)
<noodles775> jtv: the XXX explains the awkward situation with the specific job (you can only see the first line in the diff).
<jtv> noodles775: ok
<jtv> noodles775: then those changes are good afaic, and I've only got two more notes about the branch as a whole.
<noodles775> OK, those changes pushed. Shoot.
<jtv> You're creating class hierarchies with TestCase-derived base classes as helpers.
<jtv> It'd be better not to derive those from TestCase, but use them as mixins instead.
 * noodles775 looks
<jtv> Where they have setUp methods, you'll want to rename those and call them explicitly from the subclass setUp methods so as not to confuse super(), but it saves you awkwardness like deriving classes from TestCase twice (once through your own base class, once through TestClassWithFactory)
<jtv> It also avoids the impression that those base classes are themselves test cases.
<noodles775> Right... doing so now.
<jtv> It's in a bunch of places.
<jtv> noodles775: client crash
<jtv> noodles775: turns out the test-class inheritance was my last note (the other one I had has just evaporated), so once that's cleared up, you're good to land.
<noodles775> jtv: unfortunately, landing will be a while off yet (it's one of currently 7 branches in a pipe, dependent on a db-schema change that can't land until it's all finished). But approval is a step forward :)
<jtv> noodles775: whoops, I typed that on autopilot.
<jelmer> how is flush_database_updates() different from the other flush/commit methods?
<noodles775> jtv: http://pastebin.ubuntu.com/424531/
<noodles775> jelmer: it's faster than committing (just going on what I've heard), but it could be that I should be using flush instead of flush_database_updates... I can try using store.flush() instead.
<noodles775> jelmer: store.flush() works, and since it is only needed in one test, I moved it there instead of in the setUp: http://pastebin.ubuntu.com/424538/
<jelmer> ah, great
 * noodles775 gets rid of the other instance of flush_database_updates in the same file.
<mrevell> Hi jelmer, may I add a text change branch to the review queue please?
* mrevell changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2, mrevell] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> jelmer, I've assumed "yes" :) The branch is here: https://code.edge.launchpad.net/~matthew.revell/launchpad/whats-new-10-04/+merge/24408
<mrevell> I'm off for lunch for a bit
<jelmer> mrevell-lunch: sorry, I was away for lunch
<jelmer> mrevell-lunch: yeah, adding something to the queue isn't a problem
<henninge> adiroiban: did you see the test failure?
<adiroiban> henninge: :) yes. I fixed the one from the translations, but I have no idea why the registry windmill tests have failed
<adiroiban> some for the other translation windmill tests
<adiroiban> but I'm working on solving them
<adiroiban> just that I can not reproduce them on my computer
<danilos> adiroiban, hi, any reason you haven't gotten https://code.edge.launchpad.net/~adiroiban/launchpad/bug-402235/+merge/23973 landed?
<danilos> adiroiban, or https://bugs.edge.launchpad.net/rosetta/+bug/487137 for that matter?
<mup> Bug #487137: Allow Rosetta admins to create custom language codes <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/487137>
<adiroiban> danilos: yes. the permission problem is haunting me again and I don't know how should I map 3 roles to 2 zope permissions ...
<adiroiban> I know this is not possible
<adiroiban> and one suggestion was to create an adapter and delegate the custom language code part of IProduct
<adiroiban> but thinks are not that simple
<adiroiban> a simple solution would be to add launchpad.CustomLanguageCodeAdmin permission
<danilos> adiroiban, ok, just wondering because today is the last day to land stuff for 10.04, but I won't be able to help much with these things
<danilos> adiroiban, (i.e. we are very busy with many things today trying to get generation of templates in proper shape)
<adiroiban> danilos: don't worrr
<adiroiban> danilos: don't worry
<adiroiban> I can imagine that people are busy now
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adiroiban> also for that bug, I think I will have to talk with sizui and discuss what is the best option
<adiroiban> in the same time, I was also busy with other problems.
<adiroiban> btw. sorry for leaving yesterday without a notice :)
<danilos> adiroiban, anyway, I think using TranslationsAdmin should be fine, but I might be missing something :)
<danilos> adiroiban, also, I've sent you a comment on bug 548375 if you haven't seen it
<mup> Bug #548375: Rosetta import empty strings (e.g. "") from .po files <rosetta-imports> <Launchpad Translations:In Progress by adiroiban> <https://launchpad.net/bugs/548375>
<danilos> adiroiban, heh, no worries :)
<adiroiban> danilos: translationAdmin is not ok, since product owners are also TranslationAdmin
<adiroiban> and the should not be allowed to change custom language codes (as far as I understoond from the bug report)
<danilos> adiroiban, right, it's probably something for us to reconsider :)
<adiroiban> allowing product owners to change custom language codes will make things ... plain :)
<adiroiban> henninge: can you please try to resubmit that branch again? I think those windmill failers were transient as I can no longer reproduce them
<adiroiban> danilos: You still don't have the Belgium visa?
<danilos> adiroiban, I don't need it anymore since Dec 2009
<adiroiban> ah. nice. and will you be at UDS?
<danilos> adiroiban, no, sorry
<henninge> adiroiban: I can do that, sure.
<adiroiban> henninge: do you think you can also push my other branches that are approved (ie. https://code.edge.launchpad.net/~adiroiban/launchpad/bug-146178/+merge/24265 ) or should I talk with bac ?
<henninge> adiroiban: if they are approved, I can land them.
<bac> adiroiban: i attempted to land your branch last night but, again, ec2 ate it
<bac> adiroiban, henninge: i will attempt again shortly.
<henninge> bac: ok, cool
<adiroiban> adeuring: is there anything I need to do for this branch https://code.edge.launchpad.net/~adiroiban/launchpad/bug-402235/+merge/23973 ?
<adeuring> adiroiban: I think  I found a few spelling mistakes ;) Once you fix them, I can run the ec2
<adiroiban> adeuring: they should be fixed. I have also merged with devel and fixed the conflicts
<adeuring> adiroiban: ah, sorry I did not notice this. I'll start the ec test right now
<adiroiban> but I think it is my fault for only pushing those changes without adding a new comment to the MP
<adeuring> adiroiban: would have been better to ping me about the pushes ;)
<adeuring> adiroiban: and sorry, I should have recommended earier to ping me...
* StevenK changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* bigjools changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles*2,StevenK, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bigjools> soyuz review overload
* jelmer changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,StevenK,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * noodles775 takes a look at the other branches
<leonardr> gary, if you have time i'd like you to look into the mystery of https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-remove-recipe/+merge/24423
<leonardr> why do you think the lazr.restful tests pass but the lazr.restfulclient tests fail?
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<gary_poster> leonardr: I'll take a glance, though I'm kind of swamped today.  If I have any immediate ideas I'll share them though.  I'll be able to take a look within the next hour (which is also when we are scheduled to have our call).
<leonardr> gary: the reason i'm picking on you is it's a very zope-security problem. if you can think of someone else who might know, i'll ask them
<gary_poster> I'm your best bet, I think.  You know who the other suspects are.
<leonardr> jelmer, i'd like to add https://code.edge.launchpad.net/~leonardr/lazr.restfulclient/delete-entry/+merge/24421 to the queue
<leonardr> bac, want to solve a zope mystery?
<jelmer> leonardr: sure, np
<bac> leonardr: ?
* leonardr changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,bigjools,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> StevenK: did you check that the comment about binary expiry also applies for sources (ie. that just that fact that expires is set is enough to deny the copy)?
<leonardr> bac: https://code.edge.launchpad.net/~leonardr/lazr.restful/fix-remove-recipe/+merge/24423
<jelmer> noodles775: what's the policy wrt updating import ordering in existing files?
<leonardr> the zope security proxy is protecting objects when i run lazr.restfulclient tests, but not when i run lazr.restful tests on the same objects
<bac> leonardr: interesting.  unfortunately i don't have time ATM to look at it.
<noodles775> jelmer: Generally if it's not going to blow the diff out, usually we request people to order them as a drive-by, but it's not policy afaik.
<leonardr> bac: ok, np
<jelmer> noodles775: ok, thanks
<StevenK> noodles775: I was playing since-wgrant-says card, but I'm having a dig now
<wgrant> I wondered that myself a few weeks ago, but played the soyuz-code-says card and decided not to think about it any more.
<wgrant> The mechanics of LFA expiry still confuse me slightly.
<gary_poster> leonardr: almost certainly the reason for a problem like that is that, in one case, the source of the objects is security wrapped, and in the other, it is not.  Since it is "viral," a simple change at the beginning of the process will result in this.
<gary_poster> leonardr: I will try to look at details with you a little later this hour
<leonardr> gary: great
<StevenK> noodles775: Right, so death row processing will certainly expire out sources
<noodles775> StevenK: r=me... I added a small comment on the MP (just about updating the comment and perhaps the test to demo this).
<StevenK> noodles775: Source and binary expiration is slightly seperate. Just because a binary is expired doesn't mean a source is
<StevenK> noodles775: The comment in the code, or the test?
<noodles775> StevenK: Yep, I just meant that it should be obvious when looking at the source check *why* we are only checking that expires is set, rather than that it's in the past (ie. "See above comment regarding expires.").
<StevenK> noodles775: TBH, I thought that was nicely covered already by the binary comment.
<noodles775> StevenK: and I think the MP has more details about the test change I was suggesting (just not using a 1970 date, so that the test also shows that the expiry just has to be set to raise the exception).
<noodles775> StevenK: yes, it is, if it's right above it then fine (I didn't notice in the diff, just looked locally at trunk).
<StevenK> noodles775: Yeah, the comments are about 6 lines apart
<noodles775> Perfect, forget that then :)
<StevenK> And I was using 1970 since the test for expired binaries did the same
<noodles775> StevenK: yep, but looking at the test on its own, you'd expect it to fail for an expires in the future (well, I would). But up to you, I'm not fussed about it.
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles || queue: [noodles,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
 * StevenK gives ec2 another chance
<wgrant> Can someone please ec2 lp:~wgrant/launchpad/delete-more-stuff? Two attempts to land it vanished yesterday.
<wgrant> (my others landed after two or three attempts... yay reliability)
<noodles775> bigjools: given that you're change will also require updating lp-production-configs, should you be wrapping the actual config.buildmaster.bzr_builder_sources_list in a try/except?
<noodles775> wgrant: yep, trying now.
<wgrant> noodles775: Thanks.
<noodles775> bigjools: also, why not catch KeyError rather than StandardError when substituting the series?
<bigjools> noodles775: it returns None if it doesn't exist
<bigjools> I think!
<noodles775> bigjools: I just tried locally - AttributeError: No section key named bzr_builder_sources_list.
<bigjools> re. second point, I am shamefully copy & wasting the code in adapters/archivedependencies.py that does a similar thing
<bigjools> noodles775: crap, I'll fix that, thanks for noticing
<noodles775> np. Great.
<noodles775> bigjools: you could rework the second test (or add a separate test) so it shows the failure when the config option is not set?
<bigjools> noodles775: yep, I can add another test
<bigjools> noodles775: KeyError doesn't catch the failure
<noodles775> bigjools: really? I was just doing "hey %(expected_key)s" % {'actual_key': 'blah'}, but maybe I obviously missed something?
<bigjools> noodles775: I think there's a sufficiently broad range of possible errors that it makes sense to catch a base exception doesn't it?
<noodles775> bigjools: yep.
<bigjools> at least that's why it's in the other code IIRC
<noodles775> Especially if you include getting the config option in the same try/except, or are you adding a separate one?
<bigjools> it's separate
<bigjools> noodles775: http://pastebin.ubuntu.com/424644/
<noodles775> bigjools: isn't it an error if the config option is not set? (ie. do you want to log.error?)
<bigjools> noodles775: no, it's fine, since it will be unset in production for a while
<noodles775> bigjools: ok, great.
<henninge> danilos: where's the branch/mp
<henninge> ?
<danilos> henninge, email's coming up
<danilos> henninge, I've CCed you directly
<henninge> danilos: got it
<danilos> henninge, cool!
* rockstar changed the topic of #launchpad-reviews to: On call: jelmer, rockstar || reviewing: noodles || queue: [noodles,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> danilos: a lot of copy-and-paste in the test ;-)
<henninge> danilos: The class docstring, the class attributes
<henninge> danilos: and the comment on each test method is identical ...
<danilos> henninge, heh, I don't know about those things :)
<henninge> danilos: Can you give that some love, please?
<danilos> henninge, sure
<henninge> danilos: don't feign ignorance, won't work on me! :-P
<bac> adiroiban: i resent your branch to ec2 a good while ago.  let's hope this one works
<henninge> danilos: the other thing I wonder about is the indention of the "warning". The previous code did it write, your version is not in harmony with our style. ;)
<henninge> (i.e. it's wrong ;)
<henninge> s/write/right/
<danilos> henninge, lint was complaining about it
<henninge> what did it complain about?
<henninge> danilos: anyway, it's not a strong point. Basically this branch is good.
<henninge> r=me
<danilos> henninge, you are going to like this: "519: [W6501, TranslationImportQueueEntry.getGuessedPOFile] Specify string format arguments as logging function parameters"
<henninge> ;-)
<henninge> danilos: but I was just talking about the multiline-handling.
<danilos> henninge, I know what are you talking about, but why do you say it's not according to style guide?
<henninge> https://dev.launchpad.net/PythonStyleGuide#Multiline%20braces
<henninge> danilos: I'd think it should be like this:
<henninge>                warning = (
<henninge>                    "%s: can't approve entry %d ('%s') "
<henninge>                    "because entry %d is in the way." % (
<henninge>                        potemplate.title, self.id, self.path,
<henninge>                        existing_entry.id))
<henninge> but as I said, not a strong point for me ...
<danilos> henninge, well, not really
<danilos> henninge, according to that, only stuff after % should be reformatted for each bit to be on a separate line
<danilos> henninge, the initial brace is an alignment parenthesis, it's not a multi-element one :)
<henninge> danilos: right, I am just trying to apply the style.
<danilos> henninge, only stuff after % (string formatting) is a tuple
<henninge> true
<danilos> henninge, well, I am applying the multiline function definitions style myself so I think it's good :P
 * danilos goes check multiline fundefs style now ;)
<henninge> danilos: they allow both, I jut wrote them the other week ... ;)
<danilos> henninge, I know :)
<henninge> anyway, do it in whatever way you like. I have to go now ... ;-)
<henninge> Launchpad is sooooo slooooow today ...
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-04-30
<thumper> mwhudson: testfix review ? http://pastebin.ubuntu.com/424977/
<thumper> although I'd just rs it
<mwhudson> thumper: r=me
<thumper> ta
<mwhudson> with added stabbing
<mwhudson> thumper: lib/lp/code/doc/branch-karma.txt seems to have a similar issue btw
<thumper> is it failing?
<mwhudson> no
<mwhudson> but it has a factory name in expected output
<mwhudson> lib/lp/bugs/doc/bugnotification-sending.txt too
<mwhudson> i guess i can make a quick branch for this
<thumper> bzr pqm-submit -m "[testfix][r=mwhudson] Fix archive-deletion test - NEVER PRINT OUT THE NAME OF A FACTORY GENERATED ANONYMOUS PERSON (stabby-stabby)."
<mwhudson> hah
<thumper> you grepped for person-name ?
<mwhudson> "PPA named nightly for Person-name2 owned by Person-name2" :(
<mwhudson> thumper: grepping for '-name[0-9]
<mwhudson> thumper: grepping for '-name[0-9]' finds some stuff
<thumper> oh dear god!
 * mwhudson changes the starting value for getUniqueInteger, and shoves that into ec2
<thumper> :)
<thumper> mwhudson: up for a review?
<mwhudson> thumper: sure
<mwhudson> four tests have failed in that ec2 test run so far btw
<thumper> only four?
<thumper> https://code.edge.launchpad.net/~thumper/launchpad/more-careful-network-service-usage/+merge/24469
<mwhudson> they've only been going for about 20 minutes :)
<thumper> :)
<thumper> oh
<thumper> has it taken that long to set up the instance?
<thumper> didn't you start it at 3:30?
<mwhudson> um
<mwhudson> i can't tell the progress of time any more clearly :-)
<mwhudson> the first instance didn't start though
<thumper> diff is up
<mwhudson> thumper: do you think repr(self) would be a better default getOperationDescription ?
<mwhudson> it's unfriendly, but maybe less useless
<thumper> mwhudson: it is supposed to be in a nice email to a user
<thumper> so a repr wouldn't be so good
<thumper> saying: something went wrong doing xxx
<thumper> roughly
<mwhudson> thumper: 'unspecified operation' is not friendly!
<thumper> better than oops
<thumper> that line isn't necessary
<thumper> but really a fall back
<mwhudson> ok
<mwhudson> self.logger.error(e) <- i think you want self.logger.exception there
<thumper> oh
<thumper> ok
<thumper> line 162 of runner.py is where I copied it from
<thumper> I should fix that too I guess
<mwhudson> yeah, i reckon
<thumper> done
<mwhudson> tracebacks are good when things go ker-blooey
 * thumper nods
<mwhudson> thumper: you don't ned to pass the exception to exception(), did you know that?
<mwhudson> i didn't
<thumper> no
<mwhudson> so it should be self.logger.exception("something went wrong")
<thumper> I didn't know that
<thumper> mwhudson: it checks the exec stack?
<mwhudson> it calls sys.exc_info() i guess
<mwhudson> seems a bit lame
 * thumper shrugs
<thumper> I guess I should add a message of some form then
<thumper>                 self.logger.exception(
<thumper>                     "Failed to notify users about a failure.")
<thumper> ?
<mwhudson> thumper: reviewed
<mwhudson> +1
<thumper> thanks
<thumper> is BjornT our release manager?
<mwhudson> i think so
 * thumper whacks it into ec2 test while I await the RC
<BjornT> thumper: yes, i am
<thumper> BjornT: hi
<thumper> BjornT: https://code.edge.launchpad.net/~thumper/launchpad/more-careful-network-service-usage/+merge/24469
<thumper> BjornT: fallout from a screwed staging environment
<thumper> we weren't handling some errors in a nice enough manner
<thumper> especially since we can't send email right now
<thumper> so our email about your failing job was failing causing more issues
<thumper> this branch adds an extra except block that just logs
<thumper> BjornT: also be aware that in a conflict resolution branch I disabled a soyuz test, so they should land a fix for that
 * thumper fetches a drink
<StevenK> thumper: Which test, and how does it fail?
 * StevenK fetches the mail, while waiting for dogfood's ppa publisher to die again
<BjornT> thumper: ok, in general it looks ok for rc. however, it seems like the new exception handling you added is untested, no?
<thumper> BjornT: it is
<thumper> BjornT: untested that is
<thumper> BjornT: I'd have to cause a failure, then monkey patch the sending to fail again
<thumper> all to add something to the log
<thumper> is it worth it?
<thumper> StevenK: https://bugs.edge.launchpad.net/soyuz/+bug/572005
<mup> Bug #572005: Disabled test buildd-slavescanner.txt <Soyuz:Triaged> <https://launchpad.net/bugs/572005>
<BjornT> thumper: it's not hard to test without monkey patching. all you have to do is to create a custom Job class that fails in the place you expect it to fail, and maybe sub-classing BaseJobRunner to not do anything in runJob(). giving it's so simple, i'd say it's worth it.
<thumper> BjornT: ok, I'll take a look
<BjornT> thumper: and you're not only logging, you also report an oops.
<thumper> BjornT: do you think I should just log?
<thumper> I'm pretty sure that generating an oops is guaranteed not to oops
<BjornT> thumper: i do think you should log an oops, since that makes the error visible to us. so maybe two tests are needed. one where notifyUserError errors out, and one where notifyOops errors out, to show that you will still log an oops in that case.
<thumper> ok
<thumper> I'm failing to generate the enthusiasm needed to do it at 6pm on a Friday
<BjornT> thumper: without such a tests, it's tempting for someone to refactor that outer oops reporting to use self._doOops()
<thumper> ok
<noodles775> Morgan
<BjornT> thumper: i'd be willing to let you land that branch as it is, though, if you promise to file bugs about the tests, and write them on monday (so that you can more easily qa this on monday)
<thumper> BjornT: sounds fair
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing:  || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> Hi adeuring, when you're up for it: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-enable-buildd-slavescanner.txt/+merge/24478
<adeuring> noodles775: sure
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: r=me
<noodles775> Thanks adeuring
<noodles775> Hi BjornT, can I get an RC to re-enable two soyuz tests (just to be sure that any later RC's can't break things tested in the two tests).
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/db-enable-buildd-slavescanner.txt/+merge/24478
<BjornT> noodles775: of course
<noodles775> Thanks BjornT
<noodles775> adeuring: another one when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/567922-binarypackagebuild-packagebuild-4/+merge/24356
<noodles775> Sorry for the verbose MP.
<adeuring> noodles775: nah, i like verbose MPs :)
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Fancy a couple of reviews?
<adeuring> allenap: sure; I'm just finishing one for noodles775.
* allenap changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: noodles775 || queue: [allenap, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> adeuring: Thanks. https://code.edge.launchpad.net/~allenap/launchpad/storm-bulk-reload-bug-572211/+merge/24492 then https://code.edge.launchpad.net/~allenap/launchpad/checkwatches-bulk-reload-bug-572211/+merge/24493
* noodles775 changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: noodles775 || queue: [allenap, allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: r=me
<noodles775> Thanks adeuring.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: allenap || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> allenap: a very nice branch! just one suggestion: What about renaming "identity" to "object_is_key" so the intended usage is a bit more obvious
<EdwinGrubbs> adeuring: I have a small branch that I'll want to try to get an RC for after it is reviewed. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-567065-empty-branch/+merge/24497
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: allenap || queue: [allenap, noodles775, Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> EdwinGrubbs: add it to the queue.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: allenap || queue: [noodles775, Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: my branch isn't an RC, so pls give EdwinGrubbs's branch priority.
<adeuring> noodles775: sure
<adeuring> EdwinGrubbs: sorry, did not notice that it is an RC branch....
<allenap> adeuring: Okay, good idea. Thanks!
<adeuring> allenap: approved your other branch too
<allenap> adeuring: Brilliant, thanks.
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: Edwin || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<adeuring> allenap: I'll miss things like I saw today when your on "bugs holidays"
<adeuring> ...you are on...
<allenap> adeuring: Hehe, I won't be gone long :)
<adeuring> EdwinGrubbs: r=me
<EdwinGrubbs> adeuring: thanks
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> deryck: I assume you are the backup RM since you were the release manager last month. Can I get an RC for https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-567065-empty-branch/+merge/24497
<adeuring> noodles775: has your branch 567922-binarypackagebuild-new-table-1 really a 1500 lines diff or should I use a diff against some other branch?
<noodles775> adeuring: no it is not.
 * noodles775 looks
<deryck> EdwinGrubbs, sure, I can look at it.  Though I would feel more comfortable if flacoste looked, since he's probably already thought about release more than me.
<EdwinGrubbs> deryck: ok, I'll ask him
<EdwinGrubbs> flacoste: can I get an RC for https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-567065-empty-branch/+merge/24497
<deryck> EdwinGrubbs, if he is not able, then I can certainly.
<noodles775> adeuring: here's the actual diff that I get locally...http://pastebin.ubuntu.com/425292/ I'm guessing I haven't pushed the previous branch.
<adeuring> noodles775: thanks!
<noodles775> adeuring: I'll need to merge a new db-devel on that branch and resolve all the conflicts :/
<noodles775> It might be best to leave it for now.
<adeuring> noodles775: ok, noproblem
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> deryck: I can't find flacoste, can you look at the branch?
<flacoste> EdwinGrubbs: ???
<flacoste> EdwinGrubbs: i approved it
<deryck> no need for me then :-)
<EdwinGrubbs> flacoste: oh, I'm still not used to that being part of the MP
* adeuring changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
