#launchpad-reviews 2010-08-09
<thumper> review needed: https://code.edge.launchpad.net/~thumper/launchpad/recipe-build-email-fix/+merge/32063
<thumper> ^^ critical bug
<thumper> lifeless: do you think it is worthwhile changing the mail to be a symbolic name?
<lifeless> thumper: not a blocker, and I'm sure there are many places that would need touched.
<lifeless> thumper: it was just reading it I went 'erk' - its tech debt waiting for use in future.
<thumper> lifeless: actually it looks like it could be a config variable
<lifeless> ahha! in which case that makes it rather more important to use that
<thumper> config.canonical.noreply_from_address
<thumper> changed
<lifeless> sweet, thanks
<thumper> I'll just run through ec2 to make sure there are no surprising breakages
<lifeless> naturally ;)
* henninge changed the topic of #launchpad-reviews to:  On call: henninge || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to:  On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * henninge lunches
<salgado> bigjools, LP seems to be having trouble to generate a diff of https://code.edge.launchpad.net/~salgado/launchpad/bug-614923/+merge/32089, but I think it's worth of an RC so here's the diff: http://bazaar.launchpad.net/~salgado/launchpad/bug-614923/revision/9639 :)
<bigjools> salgado: approved :)
<salgado> thanks bigjools!
<salgado> bigjools, should I submit it to devel or db-devel?
* abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || reviewing: jtv, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> Hi abentley! ;)
<abentley> henninge, Hi!
<bigjools> salgado: devel is open
<salgado> bigjools, I actually submitted it to db-devel before asking.  should I submit it to devel as well?
<bigjools> salgado: might be a good idea
<salgado> ok, I'll do that
<henninge> jtv: the reason for putting translation_side_traits in the interface is so that it can be accessed on security proxied instances?
<jtv> henninge: no, it's just for convenience.
<jtv> As the cover letter says: potemplate.translation_side_traits is easier to spell than getUtility(ITranslationSideTraitsSet).getForTemplate(potemplate)
<henninge> yes, I understand taht.
<bac> bigjools:  rc ping
<bigjools> bac: rc pong
<bac> bigjools:  i have a db branch that i tried to get in on friday but due to some hardware problems i missed by 45 minutes
<bac> https://code.edge.launchpad.net/~bac/launchpad/db-bug-613442/+merge/31955
<bac> bigjools:  the patch modifies the db but does nothing operational.  it is prep for work scheduled for the next cycle.
<bac> bigjools:  so i am requesting an RC
<bigjools> bac: what are the risks?
<bac> bigjools:  there are no call sites yet using the new properties.  i cannot see any risks.  i will land it through ec2 to ensure, again, that nothing breaks.
<bac> bigjools:  there is ONE risk.  i have a self-assigned db-patch number as stub was not available to give me one
<bigjools> ah
<bac> bigjools:  after talking with jml i just picked the next one , 81
<bigjools> did you let him know?
<bac> bigjools:  that is a risk that you as the gatekeeper may need to manage.
<bac> bigjools:  no i haven't but will right now
<bigjools> FWIW I often ask him for numbers before a schema review
<bigjools> bac: ok so rc=me.  It's probably the least risky time to pick the next number since there's not many branches going in.
<bac> bigjools:  email just sent to stub and lp-dev
<bigjools> bac: great, thankds
* henninge changed the topic of #launchpad-reviews to: On call: henninge, abentley || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: reviewed
<jtv> henninge: yayyy!  Erâ¦ sneak preview?
<henninge> r=me ;)
<jtv> yayyy!
<jtv> :-)
<jtv> Thanks.
<henninge> np but we will have to discuss the traits some more another time.
<jtv> henninge: OK.  By the way, I'm not saying you did anything wrong w.r.t. the parenthesesâit's just that the new lint doesn't like it that way and I try to keep it quiet.
<henninge> jtv: all good. ;)
<jtv> henninge: you're right that we might as well not expose the traits in the interfaceâ¦ I'll see if I can change that.
<henninge> cool
* henninge changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> abentley, I have a rc candidate branch coming up.
<rockstar> abentley, https://code.edge.launchpad.net/~rockstar/launchpad/fix-cancel-rescore-permissions/+merge/32126
<abentley> rockstar, it's nice to block access to the pages, but AFAIK, this only prevents people from viewing the pages, not sending POST requests to rescore/cancel builds.
<rockstar> abentley, I don't think that's true, because it's a request to the page, and the access SHOULD be on a request level.
<rockstar> abentley, I guess I don't know, but I'm assuming it would DTRT.
<abentley> rockstar, possibly.  Testing required.  wgrant has demonstrated that the model permissions are wrong, and I think that's the key thing to fix.
<abentley> rockstar, but for these pages, shouldn't it be Admin, not Edit?  You can imagine that in the future, we'd want users to be able to cancel their own builds.
<rockstar> abentley, I thought about that, but wanted to make a minimal change for rc.
<abentley> rockstar, thought about model permissions or Admin vs Edit?
<rockstar> abentley, the latter.
<abentley> rockstar, why would Admin be a larger change?
<rockstar> abentley, it's not a much larger change, but the current permissions are launchpad.Edit, so I'd have to change the links, etc.
<rockstar> launchpad.Admin isn't currently defined.  We can make the current launchpad.Edit be launchpad.Admin, but it seems 6 of 1, half a dozen of the other.
<abentley> rockstar, that's fine then.
<abentley> rockstar, I'm thinking that eventually we will want it defined, and that it's clearer if lp.Edit isn't admin-only, but we can chalk that up as tech debt.
<rockstar> abentley, okay.
<abentley> r=me
<rockstar> abentley, please to have a review: https://code.edge.launchpad.net/~rockstar/launchpad/fix-spr-builds-perms/+merge/32139
<abentley> rockstar, r=me.  Thanks.
<rockstar> abentley, no problem.  I'm fixing the model issues now.
<abentley> rock!
<rockstar> (because otherwise we'll expose it in the API one day without even thinking about it)
<bac> sinzui:  https://code.launchpad.net/~bac/launchpad/bug-32618/+merge/32145
<abentley> rockstar, I can has review?
<abentley> rockstar, https://code.edge.launchpad.net/~abentley/launchpad/fix-builder-history/+merge/32150
* abentley changed the topic of #launchpad-reviews to: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> abentley, r=me
<abentley> rockstar, thanks.
<wgrant> abentley: Code seems to prefer model permissions and ignore view permissions. But then when the model permissions fail, a hole is left. There are a few other places like this in Code as well.
<wgrant> Everyone else seems to use correct permissions for the views as well.
#launchpad-reviews 2010-08-10
<jtv> say henninge, aren't you ocr today?
<StevenK> No, it's gmb
 * StevenK already checked
<jtv> henninge: I just put that API simplification branch up for review.
<henninge> lemme look
<henninge> jtv: looks good. Shouldn't it have a bug?
<jtv> henninge: well, it's a feature-branch task so I'm not sure we need to go to those lengths.  And it's just split off from, depending on your point of view, yesterday's branch or the job I'm working on now.
<henninge> ok
<StevenK> gmb: O hai, Mr OCR!
<gmb> StevenK, Morning. If you've got something that needs review bung it in the queue and I'll take a look in about an hour's time. Still catching up on emails.
<lifeless> gmb: hey
<lifeless> gmb: I have an _easy_ fix for you
<lifeless> https://bugs.edge.launchpad.net/malone/+bug/615644
<_mup_> Bug #615644: BugTask:+distrotask timeout on HEAT lookup <timeout> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/615644>
<gmb> lifeless, Easy fix for me to create a branch for, or is there already a branch that needs reviewing?
<lifeless> gmb: easy fix for a sparkling young fella like you to make a branch for
<gmb> lifeless, Righto, I shall take care of it presently.
<StevenK> gmb: Maybe you should suggest lifeless reviews the branch :-)
<lifeless> gmb: just needs a ordered.first() turned into a .max()
<gmb> lifeless, Ah, even better.
<gmb> lifeless, Do you know, or can you point me to a way of finding out, whereabouts that query is getting issued? I'm struggling to find it (though this may be a case of E_NOT_ENOUGH_CAFFEINE).
<lifeless> gmb: well, have a look at the linked oops
<lifeless> gmb: as for trapping it, uhm, I've been strugglying with that too
<lifeless> putting a hook in the thing tha tappaends to get_request_statements might be best
<gmb> lifeless, Ah, I think I may have found it. I was looking for Storm but maybe the query is pure SQL.
<gmb> lifeless, Yep, it's HasBugHeatMixin.recalculateBugHeatCache() that's the culprit. lib/lp/bugs/model/bugtarget.py:239
<gmb> lifeless, https://code.edge.launchpad.net/~gmb/launchpad/bug-615644/+merge/32175 if you're interested.
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* StevenK changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> gmb: *cough* :-)
<gmb> StevenK, Righto. Good job you put it in the queue otherwise (/me looks at +activereviews) jelmer was first up :)
<gmb> StevenK, That could have done with a more detailed cover letter. See https://dev.launchpad.net/CoverLetters for templates. So, expect questions.
<gmb> (Although I'm kidding a bit because of the nature of the branch)
<gmb> \0/ for killing doctests
<StevenK> gmb: I seriously tried three times to write a better cover letter and come up with writers block.
<gmb> StevenK, Understandable.
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> gmb: I'm starting to cook dinner, so I'll be afk and such like
<gmb> StevenK, Okidoke.
<henninge> jtv1: you mp is not emitting emails again. Could you investigate that or have it investigated, please?
<jtv1> henninge: grrr.  OK.
<StevenK> gmb: Are you open to bribes of pasta bake? :-)
<jtv1> henninge: btw I did get the email where I requested the review
<henninge> jtv1: but there is none on the list.
<henninge> jtv1: also, when I claim it, I should get one... let me check.
<gmb> StevenK, Pasta bake may or may not be sufficient for your branch to land without oversight (see extended reviewing guidlines, subsection 1, "Bribes of food, money or sundry electronic items")
<StevenK> Bwahaha
<jtv1> henninge: do you normally get them when you propose for merging into the recife branch?  I mean, the emails to the list aren't tied to specific LP branches?
<henninge> oh, I did not think about that ...
<henninge> jtv1: I just remeber OCRs picking up on recife branches so I always thought they had been notified.
<henninge> but maybe it's just because they appear in +activereviews.
<jtv1> henninge: sounds more likelyâ¦ I don't think anyone follows the lp-reviews list for this anymore.
<henninge> jtv1: just replying to it by makes it much more easier to comment on the diff, though ...
<jtv1> henninge: yes, it doesâ¦ but for now I think I'll just say that I've investigated and found this as the most likely cause.  :-)
<henninge> jtv1: well done.
<henninge> ;-)
 * jtv1 salutes to the sound of heroic trumpets
<henninge> jtv1: review about to be sent, r=me with a few suggestions.
<jtv1> henninge: great, thanks!
 * henninge lunches
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> gmb: All of the checks in test_initialise() are ripped straight from the doctest, and are checking that source and binary publications were copied correctly.
<gmb> StevenK, Okay, that's fine. Please add comments explaining that (you don't need to comment the individual asserts, though)
<StevenK> gmb: Yup, was going to do so tomorrow morning
<gmb> Cool.
* noodles775 changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi gmb, when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/561586-move-js-to-lp-app/+merge/32180
<noodles775> I'll take a look at StevenK's branch now too.
<gmb> noodles775, Thanks muchly. Looking at your MP now.
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> gmb: Hi. https://code.edge.launchpad.net/~wgrant/launchpad/bug-590708-getBuildsByArchIds-timeout/+merge/32181 is a nice simple RC branch, when you have time.
* wgrant changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: noodles775 || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> ^^^ can take priority over mine which is not RC.
<gmb> noodles775, r=me.
<gmb> wgrant, Looking now
<noodles775> Thanks gmb
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: wgrant || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> gmb, noodles775: Thanks.
<gmb> wgrant, Nice solution :)
<gmb> r=me
 * gmb lunches before anything else comes up
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> gmb: Ahem.
<wgrant> Thanks.
<wgrant> Do I request an 'rc', or a 'release-critical'?
<StevenK> The latter
<wgrant> D: There are two Julian Edwardses, both with hidden email addresses.
<bigjools> I am julian-edwards
<bigjools> I guess I should expose my canonical one :)
<wgrant> bigjools: I know, but the UI doesn't tell me the username.
<wgrant> It just lists two 'Julian Edwards <email address hidden>'
<wgrant> So I have to use Firebug to work out who is who, I guess.
<bigjools> hmmm that's bad
<wgrant> It should probably show the username, yeah.
<bigjools> +editemails doesn't let me change privacy. WTF do I go
<wgrant> The might be on +edit, IIRC. But you can't just expose your primary; you have to expose all :(
<bigjools> :/
<wgrant> Yes.
 * StevenK subsribes bigjools to 1,000,000 mailing lists
<StevenK> subscribes, even
 * wgrant gives up and uses the non-AJAX form.
<bigjools> so "Hide my email addresses from other Launchpad users" really ought to be on the +editemails form
<bigjools> sigh
<bigjools> bug 387774 it seems
<_mup_> Bug #387774: Hiding email addresses should be done from the email settings page <Launchpad Registry:Triaged> <https://launchpad.net/bugs/387774>
<wgrant> Hah, I reported it.
<bigjools> lol
<StevenK> Then fix it?
 * StevenK hides
<bigjools> Curtis Hovey  wrote on 2009-06-16:  "We can fix this in July and August."
<bigjools> lmao
<wgrant> It was more than a month before the code was released! I am in the clear.
<wgrant> bigjools: He didn't state a year...
<bigjools> exactly :)
<StevenK> Bwahaha
<StevenK> wgrant: Fix it now?
<wgrant> Silence!
<StevenK> You have the code ...
 * StevenK baits wgrant via phone
<wgrant> Uhoh.
<bigjools> I filed bug 615809
<_mup_> Bug #615809: I should be able to selectively hide email addresses <Launchpad Registry:New> <https://launchpad.net/bugs/615809>
 * wgrant me-toos.
<bigjools> I don't expect it to get fixed soon, Curtis has even more bugs than me
<wgrant> bigjools: Anyway, can you RC-stamp that branch?
<bigjools> wgrant: done.  let's ask gmb nicely to land it :)
<wgrant> bigjools: Thanks.
<wgrant> gmb: Can you please land it?
<gmb> wgrant, Yes, sure.
<wgrant> gmb: Thanks.
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> bigjools, On a similar note, can I get an RC for this: https://code.edge.launchpad.net/~gmb/launchpad/bug-615644/+merge/32175
<gmb> Timeout fix.
 * bigjools looks
<bigjools> gmb: nice change, I'm glad we have stub :)
<gmb> bigjools, Aren't we all :)
<bigjools> gmb: FWIW your bug is unassigned and not in-progress
<bigjools> rc=me anyway
<gmb> bigjools, Ooops. And there's no Kanban card for it either. This is what I get for accepting on-the-fly assignments. Thanks.
<bigjools> :)
<leonardr> gmb, can you take a look at https://code.edge.launchpad.net/~leonardr/lazr.restful/benji-updated-size-link/+merge/32187 ?
<gmb> Sure
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> leonardr, Woah, 1233 lines? Oh, wait. I see. nm
<gmb> leonardr, r=me.
<leonardr> gmb, thanks
<gmb> np
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> james_w, Is https://code.edge.launchpad.net/~james-w/launchpad/archive-collection/+merge/31499 in a reviewable state?
<gmb> Or is it WIP after your conversation with jml?
<james_w> gmb: WIP, sorry, just changed the status
<gmb> james_w, Cool, thanks.
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* leonardr changed the topic of #launchpad-reviews to: Today's trained review ape: gmb, leonardr || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> gmb: Thanks!
* gmb changed the topic of #launchpad-reviews to: Today's trained review ape: gmb || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<gmb> jelmer, np
 * gmb -> bbiab
* gmb changed the topic of #launchpad-reviews to: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* leonardr changed the topic of #launchpad-reviews to: leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> Hi leonardr
<jelmer> Could I add a merge proposal to your queue?
<leonardr> jelmer, sure
<jelmer> leonardr: Thanks! The mp is https://code.edge.launchpad.net/~jelmer/launchpad/55288-publisher-unknown-distroseries/+merge/32232
* leonardr changed the topic of #launchpad-reviews to: leonardr || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, us Tarmac guys wants yous to look at https://code.edge.launchpad.net/~jkakar/launchpadlib/fake-launchpad/+merge/26391 please.
<leonardr> rockstar, ok
* leonardr changed the topic of #launchpad-reviews to: leonardr || reviewing: jelmer, rockstar || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, I realize that it's a HUGE diff.
<rockstar> leonardr, technically it's jkakar's branch, but I'm willing to help facilitate where needed.
<jelmer> rockstar: Ooh, a fakelaunchpad, nice!
<mars> leonardr, +1 from Ursinha, lifeless, Diogo and I on having that as well.  The QA tools use launchpadlib a lot
<leonardr> rockstar: this branch has not had any code revisions since i made some sweeping comments back in may
<leonardr> i can take another look but a lot of it is going to boil down to "you should really make those changes"
<rockstar> leonardr, okay, great.
<rockstar> What's more, I think jkakar is on holiday for a bit.
<EdwinGrubbs> jelmer: do you want to finish reviewing this feature? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page-part2/+merge/32234
<EdwinGrubbs> jelmer: oops, I saw your name at the top, and I thought you were oncall. I'll ask leonardr.
<EdwinGrubbs> leonardr: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-602385-register-project-from-sourcepackage-page-part2/+merge/32234
* EdwinGrubbs changed the topic of #launchpad-reviews to: leonardr || reviewing: jelmer, rockstar || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* leonardr changed the topic of #launchpad-reviews to: leonardr || reviewing: jelmer  || queue: [Edwin] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> leonardr, can I add myself to the queue with https://code.edge.launchpad.net/~salgado/lazr.restful/fix-exporting-empty-entries/+merge/32239 ?
<leonardr> salgado: yeah, but i think you're the limit
* salgado changed the topic of #launchpad-reviews to: leonardr || reviewing: jelmer  || queue: [Edwin,salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> jelmer, r=me
* leonardr changed the topic of #launchpad-reviews to: leonardr || reviewing: Edwin  || queue: [salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> leonardr: Thanks!
<leonardr> edwin, some stupid questions
<leonardr> what does the 'count' attribute do in the img tag within the copyright-expand link?
<leonardr> edwin: is source_package_name really a name? it looks like a source_package object that _has_ a name
<EdwinGrubbs> leonardr: the count can be removed. I copied the link from elsewhere in the widget template, although I don't understand what it does for those links. I'll investigate and see if it is just cruft for those links also.
<leonardr> ok
<EdwinGrubbs> leonardr: confusingly, the sourcepackage object is just a combination of a distroseries and a sourcepackagename object, and the sourcepackagename table in the db just has a name column.
<leonardr> ok, so it is a sourcepackagename object that has a name
<EdwinGrubbs> leonardr: yes, the sourcepackage.name actually just returns the sourcepackagename.name
<leonardr> edwingrubbs, does link_source_package need to check that self.product is not None, or is that a given?
<EdwinGrubbs> leonardr: hmm, it won't be None because of where it is called, but I should turn that into an argument to link_source_package() so that it does surprise someone when they reorganize the main_action().
<leonardr> ok
<bac> leonardr:  do you have time for another branch to review?
<jelmer> I was about to ask the same :-)
<bac> jelmer:  trade?
<bac> jelmer:  meaning, i'll review your branch if you'll do mine.
<leonardr> edwingrubbs: "If that is your situation we u= rge" really? is that an email thing?
<leonardr> if so, it might make more sense to change it back, to make it clear where the line break is in the email
<EdwinGrubbs> leonardr: oh, I was fixing a lint error. I'll try to make it look less odd without making lint complain.
<jelmer> bac: Sure! What's your branch?
<bac> jelmer:  https://code.edge.launchpad.net/~bac/launchpad/bug-590813/+merge/32248
<leonardr> edwin: r=me with the changes we discussed
* leonardr changed the topic of #launchpad-reviews to: leonardr || reviewing: salgado || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> bac: Mine is at https://code.edge.launchpad.net/~jelmer/launchpad/publisher-use-debian-1/+merge/32245
<leonardr> salgado, your changes to test_webservice, are they just delinting?
<salgado> leonardr, yes, removal of unused imports
<leonardr> ok
<jelmer> bac: Thanks for the review.
<leonardr> salgado, r=me
<salgado> leonardr, cool, thanks!
<bac> jelmer:  np
<salgado> leonardr, no need to bump the version or add anything to NEWS.txt as there was no release where that bug was present, right?
<leonardr> salgado: right, i'll do a release tomorrow
<jelmer> bac: I'm still reading up on snapshots, but a review of your branch is in progress as well.
<bac> jelmer:  take your time, no rush
* leonardr changed the topic of #launchpad-reviews to: On call: - || reviewing: salgado || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* leonardr changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> bac: It seems like this could be an issue in a lot of situations - every sql based collection field I guess?
<bac> jelmer:  yes, that is true.
<jelmer> bac: r=me, though I wonder if it would make sense to have a more structural solution for this kind of thing as well.
<jelmer> bac: Something like a shortlist() implementation that would give up on trying to collect the full sequence contents rather than errorring out perhaps.
<jelmer> It seems like a pity to give up on snapshotting product series (which would be access quite often I imagine) just because of a very rare case.
<bac> jelmer:  my understanding of 'shortlist' is that we want it to complain loudly b/c it indicates our original assumptions were wrong
<bac> jelmer:  lifeless started an email thread about changing Snapshot to ignore CollectionFields by default.  it seems like a good idea which may get implemented in the future.
<jelmer> bac: Ah, that would make sense indeed.
#launchpad-reviews 2010-08-11
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> thumper: "ICanHasLinkedBranch"â¦?  All it needs is s/s/z/
* jtv changed the topic of #launchpad-reviews to: On call: jtv || reviewing: thumper || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> thumper, you there?
* jtv changed the topic of #launchpad-reviews to:  On call: jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<thumper> jtv: kinda
<jtv> thumper: nm it's reviewed now
<jtv> thumper: bikeshed effectâI can't judge the merits of the branch without a lot more research (decadent I-don't-do-manual-labour grunt here) but I've got lots of little whiny pedantic points about what the code looks like.  :)
<thumper> is this for my branches?
 * thumper reads mail
<jtv> thumper: no, it was for your kite
<StevenK> gmb: O hai!
<StevenK> gmb: Mind looking at my MP from yesterday again?
<gmb> StevenK, Is it RC? If not, I'll do it later on today as I've got a few tasks that need to be done this morning.
<StevenK> gmb: It isn't, that's cool
<gmb> StevenK, Cool, thanks.
* EdwinGrubbs changed the topic of #launchpad-reviews to:  On call: jtv, Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* jtv changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> EdwinGrubbs, would you like to review https://code.edge.launchpad.net/~salgado/launchpad/request-to-base-template-adapter/+merge/31982 for me?
<EdwinGrubbs> salgado: it looks like mwhudson already reviewed it. do you need a ui review?
<salgado> EdwinGrubbs, just code. I wanted somebody else to have a look as he suggested, but I could ask sinzui if you'd prefer
<EdwinGrubbs> salgado: I can look at it, but will definitely have some questions about the intent. Is vostok for setting up onsite instances for Linaro?
<salgado> EdwinGrubbs, yep, that's it
<salgado> let's see if sinzui would like to take it...
<salgado> sinzui, would you like to review that branch above?
<salgado> back soon
<EdwinGrubbs> salgado-lunch: shouldn't there be a test for IMainTemplateFile() when the layer is not Vostok?
<sinzui> EdwinGrubbs, https://bugs.edge.launchpad.net/launchpad-registry/+bug/326384
<_mup_> Bug #326384: Milestone dateexpected and release datereleased wrong type in db <tech-debt> <Launchpad Registry:Triaged> <https://launchpad.net/bugs/326384>
<leonardr> Edwin, i seek a review of https://code.edge.launchpad.net/~leonardr/launchpadlib/616055/+merge/32352
* noodles775 changed the topic of #launchpad-reviews to: On call: Edwin || reviewing: - || queue: [leonardr, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> Hi EdwinGrubbs, I've also got a critical LP branch ready, but won't be around. I'll check back later before going to bed though to fix any thing you find (and send it off for testing):
<noodles775> https://code.edge.launchpad.net/~michael.nelson/launchpad/616331-private-builds-in-builder-history/+merge/32355
<EdwinGrubbs> noodles775: I'll work on that. How soon do you need it? 2 hours?
<noodles775> EdwinGrubbs: thanks. 2 or 3 would be great, so I can have it tested for Julian in the morning.
<EdwinGrubbs> noodles775: review sent
<noodles775> flacoste: do you have a minute to evaluate an RC MP - basically whether I should land it on db-devel, or just test in and include it as part of the re-roll. There are comments from Julian in the irc log on the attached bug: https://code.edge.launchpad.net/~michael.nelson/launchpad/616331-private-builds-in-builder-history/+merge/32355
<noodles775> (the branch name is misleading... it's just a query improvement for a timeout)
<flacoste> noodles775: we don't do re-roll anymore
<flacoste> noodles775: we'll cherry-pick after the release
<noodles775> flacoste: OK, so whether I should ec2 land it on db-devel, or do it as a cp...
<noodles775> OK.
#launchpad-reviews 2010-08-12
<stub> lifeless: So my branch had timeout as the timeout, and your branch has timeout as the goal time for the 99% calculations, and histogram_width as the timeout
<lifeless> yeah, my first branhc, as you said, was in poor shape
<lifeless> the incremental changes are twofold
<lifeless> firstly, there is an existing bug in the report: the stddev is being mangled by the clipping of the data
<lifeless> likewise the variange
<lifeless> so I changed that, and having fixed that I then had the abilitity to do the histogram columns independently, and I figures that seeing the shape beyond the timeout would be useful
<stub> The logic was that if a page takes longer than the hard timeout, that is irrelevant. It doesn't render so ends up in the 'fail' column at the hard timeout, giving a nice 'kick' at the end.
<stub> I would have labelled the last column 'timeout' if the graphing tool let me.
<stub> I suspect the only correct answer though would be to report a timeout%, and have the stats calculated non-timeout pages.
<lifeless> stub: well, what I've done willso have a kick
<lifeless> and - we could do a kick at the exact timeout column if you wanted
<lifeless> but I wanted the stddev to be accurate, because that tells us the 99% figure
<lifeless> stub: perhaps you could give it a spin and we can see what it looks like ?
<stub> lifeless: I don't like that we are growing a separate list for capped request times. I think we want to create this on the fly using a generator, and use numpy.fromiter instead of numpy.asarray to construct the array.
<lifeless> stub: ok
<lifeless> stub: is that a memory overhead concern ?
<stub> We have nearly 500 of these things in ram containing over 10 million 32bit objects, so yeah
<stub> But I pulled out the spool-to-disk version, as it seemed to be better to hit swap than open 500 temporary files. That was implemented as I thought I was crashing devpad, but it was just hardware issues.
<stub> (monthly reports will get lots of requests, when devpad stays up long enough to actually generate one...)
<stub> I'll give it a run as is - memory shouldn't be an issue for a daily run.
<lifeless> I'm happy to change that for you
<lifeless> I considered doing it on the fly but it seemed slower ;P
<stub> It is slower, but that is a minor part of our runtime. Parsing the logs takes time. Generating the stats is pretty much instantaneous.
<lifeless> ok
<lifeless> so - lets see how it looks, and I'll look up fromiter
<stub> fromiter(iterable, numpy.float32, len(self.request_times)) - we know the length so can use the third parameter
<lifeless> and iterable is an adapter to call min ?
<stub> https://devpad.canonical.com/~stub/ppr/edge/latest-daily-timeout-candidates.html is done. lpnet still generating
<stub> lifeless: I imagine it is a generator that returns the time capped to the timeout
<lifeless> RIGHT
<lifeless> bah
<lifeless> Right
<lifeless> thats what I thought too
<stub> Bwaha... LibraryFileAlias:StreamOrRedirectLibraryFileAliasView is a mess without the cap ;)
<stub> But I guess that is the truth.
<lifeless> yup
<lifeless> do think these changes are ok then ?
<lifeless> I've pushed up a (hopefully working) fromiter version
<stub> That all looks good. I've kicked off another run with the new version
<lifeless> thanks
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: - || queue: [leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> noodles775: Could I add a branch to your queue?
<noodles775> jelmer: please do :)
<jelmer> noodles775: The MP is https://code.edge.launchpad.net/~jelmer/launchpad/613468-xb-ppa-db/+merge/32307
* jelmer changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: - || queue: [leonardr,jelmer]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> noodles775, thanks!
<StevenK> noodles775: O hai, can you re-re-review https://code.edge.launchpad.net/~stevenk/launchpad/switch-ifp-to-unittests/+merge/32073 ?
<noodles775> StevenK: indeed :)
<noodles775> StevenK: I'm not trying to make your life hard, and hate dragging this review on, but I don't understand why test_create_builds does so much more than what the name implies... do you have time to mumble and discuss it briefly?
* noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jelmer || queue: [leonardr]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<StevenK> noodles775: Yes, doing so in a sec
<StevenK> noodles775: But it doesn't? It asserts the *source* for 0.1-2 exists, and then creates a build for it?
<StevenK> noodles775: The diff does not appear to have been updated yet, but the MP is ready for your re-re-re-review.
<noodles775> Thanks StevenK
<StevenK> noodles775: No news is good news, or I'm in the queue?
<noodles775> StevenK: looking now (had to merge as the MP hadn't updated)
<StevenK> Yes, I'm not sure what is going on there
<jelmer> noodles775: Thanks for the review!
<noodles775> np
<noodles775> StevenK: looks good. I've just highlighted what seem to be a few inconsistencies in the comments, but otherwise r=me: http://pastebin.ubuntu.com/476903/
<wgrant> jelmer: Hm, why are the extra fields JSON?
<wgrant> They're known to be string pairs.
<wgrant> It seems like an extra table would do fine, leave the table smaller, and make it possible to query them in future.
<jelmer> wgrant: consistency with other fields in the database, where we also use JSON.
<wgrant> Are there any, apart from on Job?
<StevenK> noodles775: Planning to commit http://pastebin.ubuntu.com/476907/
<jelmer> wgrant: No, just job actually.
<wgrant> jelmer: And it's like that so that custom job types can store different data there.
<wgrant> I don't see why JSON should be used here, and not a separate table.
<jelmer> wgrant: simplicity; I don't see why we would need to query that field. if we would go the way of a separate table we might want to use it for some of the other control fields as well
<jelmer> which might not be a bad idea, just out of scope for me at the moment.
<StevenK> gmb: O hai! Given noodles775's has Approved the MP at https://code.edge.launchpad.net/~stevenk/launchpad/switch-ifp-to-unittests/+merge/32073; can haz moar review?
<gmb> StevenK, If you stop asking in lolspeak, yes. ;)
<gmb> In fact, I think noodles775's review is far more comprehensive than mine (hurrah for context)
<gmb> So I'll just mark it approved.
<StevenK> gmb: Okay, cool
* StevenK changed the topic of #launchpad-reviews to: On call: noodles775 || reviewing: jelmer || queue: [leonardr,StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<james_w> jml: hi, would you be willing to do an incremental review of https://code.edge.launchpad.net/~james-w/launchpad/no-more-sampledata-2/+merge/31891 ?
<jml> james_w, yes. I'll look at it shortly
<james_w> thanks
<leonardr> can i get someone to review my really simple launchpadlib branch? i've been in the queue since yesterday
<bac> hey salgado, can you do a shipit review for me?
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpadlib/616055/+merge/32352
<salgado> bac, sure!
<bac> salgado:  https://code.edge.launchpad.net/~bac/shipit/bug-616059/+merge/32457
<salgado> bac, not allowed
<bac> salgado:  try now
<salgado> bac, while you subscribe me to your branch, subscribe launchpad-pqm as well, or else you won't be able to land it later
<bac> salgado:  thanks for the tip!
<leonardr> bac, while you wait, will you review my branch please?
<bac> leonardr:  sure
 * bac looks
<leonardr> thanks
<salgado> bac, did you consider doing that for canonical.* instead of canonical.launchpad.*?
<bac> salgado:  yeah, sort of.  i thought lp was sufficient.  you think i chose wrong?
<salgado> bac, I'd have done for canonical.* but lp.* and canonical.launchpad.* is certainly where we do most of our changes/refactorings, so it's no big deal
<bac> salgado:  yeah, you would've...but you're smarter than me!
<salgado> bac, If that was the case I'd have done this change long ago. ;)
<bac> salgado:  yeah, if you weren't such a slacker
<salgado> that is certainly the case
<bac> leonardr:  r=bac.
<salgado> bac, did you get my review?  you might want to get someone from ISD to have a look as well, as they own shipit now
<bac> salgado:  i see it now.  did you mean to have it just be a comment?
<salgado> no, just approved using the web UI, but it'd be good to at least let them know about the change before it lands
<bac> salgado:  i'll talk to achuni or someone else
<bac> salgado:  thanks for catching those issues
<bac> i renamed to sd_setUp b/c there is another setUp being imported in that file
<salgado> np.  thank you for doing this change
<salgado> fair enough
<bac> sd is cryptic for 'systemdocs'
<leonardr> benji-lunch, it looks like we're missing a review for lazr.restfulclient/total_size_link
<leonardr> noodles775, the branch i'd like reviewed is https://code.edge.launchpad.net/~benji/lazr.restfulclient/total_size_link/+merge/32472
<leonardr> it's benji's branch but most of the code is mine
<leonardr> just fyi, when you get to me
<jml> james_w, done
<james_w> thanks jml
<jml> np
<leonardr> noodles775, i'd also like to add this to the queue: https://code.edge.launchpad.net/~leonardr/launchpad/new-lazr.restful-and-friends/+merge/32474
<jelmer> leonardr: I think Michael has signed off for the day.
<leonardr> well, damn
<leonardr> bac, are you in the mood to do more reviews?
<bac> leonardr:  i can do one.
<jelmer> rockstar should be around later, or is he on leave?
 * bac has a full day of reviews tomorrow
<rockstar> Oh snap, forgot it was Thursday.
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: leonardr || queue: [StevenK]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> bac: ok, rockstar will take care of these
* leonardr changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: leonardr || queue: [StevenK, leonardr]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> thanks leonardr and rockstar
<rockstar> leonardr, apparently there are two review to do for you.  Where's the other one?
<rockstar> (I already did new-lazr.restful-and-friends)
<leonardr> rockstar: ok, the other one is https://code.edge.launchpad.net/~benji/lazr.restfulclient/total_size_link/+merge/32472
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: StevenK || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> rockstar may i add a small branch to your queue?
<rockstar> bac, sure.
<rockstar> bac, ftr, I'm going to clear out all the +activereviews today, so you won't have a huge queue tomorrow.
<bac> rockstar:  that would be swell.  do leave me some, though!
<bac> leonardr:  what is the status of this branch: https://code.edge.launchpad.net/~leonardr/launchpad/grant-permissions-oauth/+merge/29425
<leonardr> bac: i got sidetracked by an emergency project before i could land it, and i don't want to land it on its own because it's part of another large project
<leonardr> so, the status is "wait and see"
<bac> leonardr:  ok
* bac changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: StevenK || queue: [bac]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bryceh changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: StevenK || queue: [bac bryce]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> thanks for the review rockstar
<jelmer> rockstar: I've fixed the conflicts in my branch; any chance I could add it back into your queue?
<rockstar> jelmer, if it's in +activereviews, I'll get to it today.
 * rockstar is firefighting currently.
<jelmer> rockstar: great, thanks
<jelmer> rockstar: oh, ok
<thumper> jelmer: are you going to propose the bzr-git update branch?
<jelmer> thumper: yes, it should fix quite a few of the broken imports
<thumper> cool
<thumper> there is someone trying the chicken import every few days
<jelmer> thumper: yeah, he pinged me on IRC today about it.
<jelmer> thumper: How hard it is to get the new bzr-git deployed without waiting for the next lp release?
<thumper> jelmer: very trivial
<thumper> jelmer: we land it, test it on staging
<thumper> jelmer: then CP it to the production branch
<thumper> jelmer: and get it rolled out
<thumper> so lag time is 3 x ec2 test run + staging rollout lag + production rollout lag
<thumper> add in test time, and getting approval, both of which should be pretty short
<jelmer> ok
<jelmer> thumper: https://code.edge.launchpad.net/~jelmer/launchpad/update-bzr-git/+merge/32527
<thumper> jelmer: done
<jelmer> thumper: bug 497428 appears to be fixed, scilab's import is working. Can I mark it fixreleased in launchpad-code?
<_mup_> Bug #497428: Import on a large git repository is failing <code-import> <Bazaar Git Plugin:Fix Released> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/497428>
<thumper> jelmer: yep
<jelmer> thumper: perhaps we should increase the number of revisions imported per step with bzr-svn
<thumper> jelmer: I'm up for that
<thumper> jelmer: it is just a config change I think
<jelmer> thumper: the KDE branches appear to be working now, but they're spending 15 minutes trying to figure out what 1k revisions to fetch and then 2 minutes doing the actual work.
<thumper> jelmer: 10k seem reasonable?
<jelmer> thumper: Yeah, definitely.
<jelmer> thumper: Could the git batch size also be increased perhaps?
<thumper> jelmer: yep
<jelmer> I think it's at 10k now but we should be able to handle 30k at least
<thumper> jelmer: sure, sounds reasonable
<mwhudson> i still wonder if it should be time-limited not rev-count limited
<jelmer> mwhudson: time and memory limited in that case, but yeah, that sounds reasonable.
#launchpad-reviews 2010-08-13
<wgrant> jelmer: 15 minutes? kdebase too almost 9 hours...
<wgrant> s/too/took/
<jelmer> wgrant: it depends on the machine. pear importing kdelibs4 seems to take 15 minutes to figure out what revisions to fetch, 3 minutes to fetch the actual 1k revisions and then some time importing tags and moving branches around
<mwhudson> each machine has to build the cache separately
<wgrant> Ah, I see.
<mwhudson> which is lame, but...
<wgrant> The cache should be shared across the whole repo, though, right?
<jelmer> wgrant, yes
<StevenK> thumper, lifeless: Do either of you have a few moments to look over a quick MP I put up?
<thumper> StevenK: sure
<StevenK> thumper: https://code.edge.launchpad.net/~stevenk/launchpad/fix-ssh-keys-make-lp-user/+merge/32543
<thumper> StevenK: which soyuz test uses it?
<StevenK> thumper: Sorry, was distracted by mumble. lib/lp/soyuz/doc/sampledata-setup.txt
<StevenK> Which calls utilities/soyuz-sampledata-setup.py
<thumper> this is starting to make me hurl
<thumper> what does that do?
<StevenK> Haha, I knew I should have warned you
<StevenK> utilities/soyuz-sampledata-setup.py sets up the database so developers can run Soyuz locally
<thumper> so it isn't part of the actual test suite?
<thumper> let me put this another way:
<thumper> we should NOT be calling utility scripts from the test suite
<StevenK> Well, the way I found this bug is via the test suite
<StevenK> And I think the bug should be fixed
<StevenK> But I'm happy to propose another branch that just removes that whole doctest
<thumper> is the purpose of the doc test to make sure that the script doesn't break?
<StevenK> Yes
<thumper> what should be done then, is to move the guts of the scripts into something like lp.soyuz.tests or lp.soyuz.scripts.tests
<thumper> and test that
<thumper> and have the utility scripts call into those tested functions
<thumper> you have two choices
<thumper> 1) keep pulling on the thread and move the buts that need to be tested into lp.soyuz.<wherever>.tests
<thumper> (or scripts)
<thumper> and have the tests in tests
<thumper> or
<thumper> 2) take the easy way out, file a bug and add an XXX comment referencing the bug, and tag it tech-debt
<StevenK> I'll do 2, and have a discussion about it when Julian isn't sick
<thumper> ok
<StevenK> thumper: But in either case, this is a real bug in make-lp-user, and should be fixed.
<thumper> ack
<thumper> file the bug, add the comment, and I'll approve it
<StevenK> Yup, doing so now
<StevenK> thumper: Bug filed, comment added, branch pushed.
* rockstar changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [bryce]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<wgrant> thumper: Why shouldn't it be called?
<wgrant> The point is to test the script.
<wgrant> And running the script is probably a reasonable way to do that.
<thumper> if it is a tested script it should be in lp.soyuz.scripts
<thumper> it is all about expectation
<thumper> utilities are extra bits
<thumper> the test suite should test lib/lp
<thumper> not utilities
<thumper> although it does test cronscripts and other scripts
<thumper> but I think utilities is too much
<wgrant> Why?
<wgrant> Why not test it? What does it cost?
<thumper> I'm not saying the code should be untested
<thumper> it is about expectation
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [bryce]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> On call: adeuring || reviewing: bryce || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring1 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> Hi Abel, can I add a branch to your queue?
<adeuring> jelmer: sure
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jelmer || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jelmer> adeuring: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/128259-async-1/+merge/32439
<adeuring> jelmer: the last assertTrue() in testSuccess() needs something like "in log_lines"
<jelmer> adeuring: Nice catch. I wonder if the matchers that James and lifeless have been working on has is useful here.
<adeuring> jelmer: maybe; haven't used them
<adeuring> yet
<adeuring> jelmer: r=me, with a few more nitpicks
<jelmer> adeuring: Thanks!
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> lifeless: can you look at the db changes in this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout/+merge/32555
<bac> hi adeuring
<adeuring> hi bac!
<bac> adeuring:  i'm sprinting this week and won't be doing reviews full-time today
<adeuring> bac: ok, no problem.
<bac> but i'll be doing reviews for branches coming out of our sprint
<adeuring> bac: even better :)
<bac> adeuring:  didn't want you to think i'd abandoned you
<adeuring> bac: so, do you want to review sinzui's branch or shall i do it?
<sinzui> bac said he would since he cares about the feature
<sinzui> adeuring, ^
<adeuring> sinzui: thanks;
<sinzui> adeuring, bac will review all my branches because we are sprinting together.
<adeuring> sinzui: thanks for the clarification
<leonardr> adeuring, i would appreciate a review of https://code.edge.launchpad.net/~leonardr/lazr.restful/check_total_size_active_on_call/+merge/32590
<adeuring> leonardr: sure
* adeuring changed the topic of #launchpad-reviews to:  On call: adeuring || reviewing: leonardr || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<salgado> rockstar, yesterday I had a chat with flacoste and he suggested a change to https://code.edge.launchpad.net/~salgado/launchpad/layer-specific-navigation/+merge/32358, which you approved yesterday.  care to have a look at the incremental changes: http://bazaar.launchpad.net/~salgado/launchpad/layer-specific-navigation/revision/9640 and http://bazaar.launchpad.net/~salgado/launchpad/layer-specific-navigation/revision/9642 ?
<adeuring> leonardr: r=me
<leonardr> adeuring, thanks
* adeuring changed the topic of #launchpad-reviews to:   On call: - || reviewing: - || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> salgado, those changes look fine to me.
<salgado> rockstar, cool, thanks!
<salgado> is there anybody willing to do a small review for me? https://code.edge.launchpad.net/~salgado/launchpad/vostok-traverse-package/+merge/32599
<leonardr> salgado, let's trade
<leonardr> i need a follow-up
<leonardr> https://code.edge.launchpad.net/~leonardr/launchpad/new-lazr.restful-and-friends/+merge/32474
<salgado> leonardr, deal!
<salgado> leonardr, approved.  out of curiosity, have you changed total_size into a link because it may be expensive to compute and not always necessary?
<leonardr> salgado: exactly
<salgado> cool
<leonardr> salgado, why did it become necessary to remove the security proxy on the bug?
<leonardr> and what does the .. syntax in your zcml mean?
<leonardr> does it make sense to add a test that shows you can't traverse to a source package on some site other than vostok?
<leonardr> answer me those questions three and i will approve your branch
<salgado> leonardr, I think the makeBugTask() method was not used anywhere, or it's used by tests which log in as admins before calling it
<leonardr> ah
<salgado> the .. syntax means relative paths; '..' is the parent directory
<leonardr> ok
<salgado> and you can traverse to source packages on the launchpad.dev vhost
<leonardr> but your mp says "it's only possible to traverse to distro series and source packages on the vostok.dev vhost"
<leonardr> did you mean that it's not possible to traverse to anything else on vostok.dev?
<salgado> what we want is to make sure that in vostok.dev you can only traverse to a subset of those you can traverse to on launchpad.dev
<leonardr> ok, the language was ambiguous
<leonardr> r=me
<salgado> yeah, I guess I should change that to "it's possible to traverse only to ..."
<salgado> on vostok.dev it's possible to traverse only to ...
<salgado> I'll do that change
<salgado> thanks leonardr
<salgado> oh, that's in the mp.  I thought it was in the code
<salgado> duh
<lifeless> EdwinGrubbs: hi
<EdwinGrubbs> lifeless: hi
<lifeless> I've replied on the MP
<lifeless> but I'd be very happy to chat realtime as well if thats useful
<mars> salgado, have a minute to review https://code.edge.launchpad.net/~mars/launchpad/fix-ec2-shutdown-617598/+merge/32633 ?
<salgado> mars, care to add a comment explaining why we use 'at' instead of shutdown +X?
<mars> salgado, good idea, will do
<salgado> r=me with that
<mars> great, thanks!
<jelmer> Could somebody perhaps re-review https://code.edge.launchpad.net/~jelmer/launchpad/update-bzr-git/+merge/32527 ? Tim already approved the changes itself but I had to fix two tests as well.
<mars> jelmer, why is the 'from dulwich.repo import Repo as GitRepo' on line 36 right beside the test code?
<mars> jelmer, and should you be using .getUniqueString() as the message parameter?
<jelmer> mars: This isn't a valid excuse, but the reason I did it that way is because that's what the old code was doing to import run_git as well.
<mars> hehe, yep, saw that :)
<mars> jelmer, up to you.  I'll stamp it r=mars.  And about the import statement?
<mars> oh, "It did that before" for /both/ the string and the import statement
<jelmer> mars: Yep. But I agree the imports should be at the top of the file and using getUniqueString() is preferable to using "dsadsa".
<mars> ok, I would at least change the import statement - it will stand out when others read the code :)
<jelmer> mars: Thanks for the review.
<mars> and if .getUniqueString() is already there, just takes a few ^W to fix it
<jelmer> I have no idea what ^W does, but I think I understand what you mean :-)
<mars> readline delete word left
<mars> dangerous to learn, because it is also Gnome/CUA 'Close Window and lose everything you were doing because you were typing too fast and hit Enter instead of Cancel on the Save dialog'
<jelmer> Heh, that's my first association with ^W as well.
<jelmer> Anyway, I'm off.
<jelmer> mars: Thanks again for the review, and have a nice weekend!
