#launchpad-reviews 2010-06-28
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> henninge, can I trouble you for a review?
<jtv> https://code.edge.launchpad.net/~jtv/launchpad/bug-599254/+merge/28618
<henninge> jtv: I am onto it!
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jtv> great, thanks
<henninge> jtv: I don't understand why paragraph 3 is there. You don't seem to be doing anything about this situation. Or am I misreading it?
<jtv> henninge: you mean the one about the validators?
<henninge> jtv: That's "These validators only ..."
<henninge> yes,
<jtv> That's what causes the problem.
<jtv> Changes to the potemplate field are combined with changes to the flag, confusing the validators.
<henninge> I don't understand. When are they combined?
<henninge> jtv: I understand the paragraph (without the last sentence) to say: "The validator only looks at diverged messages."
<jtv> Ah right, I missed something I had wanted to keep distinct: the validators only look at messages _with the same value in the potemplate field_, not for the same template as I said.
<henninge> ah, ok
<henninge> so they either search all shared messages or all diverged ones.
<jtv> Well, "all" diverged ones for the same template.
<jtv> (and language and potmsgset, of course)
<bigjools> henninge: hi, I can haz review plz
* bigjools changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: jtv || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<henninge> bigjools: sure
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: bigjools || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<lifeless> I has a couple of reviews up from the weekend
<lifeless> can I tempt anyone
<jml> sorry, I've neglected my prior obligations too much already today.
<jml> lifeless, fwiw, you won't be able to land them until Monday anyway.
<lifeless> jml: I'm not setup to land these days anyway
<lifeless> jml: my hope is to have someone review them overnight
<jml> lifeless, we won't be able to land them until Monday, then.
<lifeless> I can then make any changes, and after that they can queue as needed.
<lifeless> sure
<lifeless> thank you for calling that to my attention.
<lifeless> its a very foreign experience for me ;)
<rinze> henninge: can I add another branch to the queue?
<henninge> rinze: go ahead ;)
* rinze changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: bigjools || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ || h
* rinze changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: bigjools || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> bigjools: does your security adapter really loop over all private PPAs in Launchpad? Couldn't become a performance issue?
<henninge> that
<bigjools> henninge: just private PPAs for the person, not all
<henninge> bigjools: I don't see the person mentioned, though: "getUtility(IArchiveSet).getPrivatePPAs()"
<bigjools> henninge: argh shit you're right
<bigjools> thanks for noticing
<bigjools> I'll see what I can do
<henninge> bigjools: np, I'll push it back into the queue but for now but can approve it anytime you have a fix.
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: jelmer || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> rinze: on to yours, jelmer
<bigjools> henninge: ok thanks
<bigjools> henninge: pushed another revno up, but here's the partial diff: http://pastebin.ubuntu.com/456416/
<henninge> bigjools: Great usage of sets!
<bigjools> it reduces the extra query count to 2 :)
<henninge> bigjools: r=me ;)
<henninge> bigjools: you still need to remove the IArchiveSet import, I assume.
<bigjools> henninge: err yes :)
<bigjools> thanks :)
* henninge changed the topic of #launchpad-reviews to: On call:  henninge || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> jtv: yeah, you can land the oops fix! ;)
<jtv> henninge: yay!
<jtv> henninge: on db-devel, right?
<henninge> err ....
<henninge> jtv: nope, devel.
<jtv> henninge: isn't db-devel normal procedure after pqm closure?
<henninge> no, after devel closure
<jtv> argh
<henninge> jtv: look at the #lp-code topic
<jtv> henninge: right ho... that's easy then
<jtv> (though I had a db-devel branch ready as well)
* abentley changed the topic of #launchpad-reviews to: On call:  henninge, abentley || reviewing: jelmer, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ ||
* henninge changed the topic of #launchpad-reviews to: On call:  abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ ||
<henninge> rinze: done! Have a look at it after the match. ;-)
<rinze> rinze: Thanks!
<henninge> rinze: talking to yourself ;)
 * henninge leaves
<rinze> henninge: Oops! Thank *you*
<rinze> abentley: Hi, can I add a branch to the queue?
<abentley> rinze, sure.
* rinze changed the topic of #launchpad-reviews to:  On call:  abentley || reviewing: - || queue: [jelmer] || This channel is logged: http://irclogs.ubuntu.com/ ||
<rinze> abentley: thanks
* abentley changed the topic of #launchpad-reviews to: On call:  abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ ||
<rockstar> abentley, can I have you look at this diff to fix the build.
<rockstar> http://pastebin.ubuntu.com/456525/
<abentley> rockstar, r=me.
<rockstar> abentley, thanks.
<abentley> rockstar, let me guess: someone removed it and someone added something that needed it.
<rockstar> abentley, nope, just got removed.
<abentley> rockstar, that's crazy.
<rockstar> abentley, I think it appeared to lint like it was an unused import, but it breaks the build entirely...
<abentley> rockstar, yeah, but people are supposed to run the full test suite before landing things...
<rockstar> abentley, especially when it's landed as release-critical.
<rinze> abentley: thanks for the review!
<mars> abentley, ping, could you please review this one line testfix? https://code.edge.launchpad.net/~mars/launchpad/testfix-missing-iarchive-import/+merge/28665
<abentley> mars, that looks just like the one rockstar just got me to review.
<mars> abentley, then good thing I didn't r=jfdi :)
<mars> I'll toss mine then
<rockstar> mars, mine's already in PQM, so I win anyway.
<rockstar> mars, as release manager though, could you please track down why a change like that was landed with the release-critical tag?
<mars> rockstar, three experienced reviewers trusted the original submitter - possibly a failure of impartial judgement on our parts
<rockstar> mars, it breaks the build, which means it didn't get a successful test run.
<abentley> mars, rockstar, I have a fairly good idea: julian was already getting test failures because of bug #599397, so he didn't notice the legitimate failures.
<_mup_> Bug #599397: code imports break when tdb is installed <code-import> <Launchpad Bazaar Integration:Triaged> <https://launchpad.net/bugs/599397>
<rockstar> abentley, this fix I made allowed Launchpad to build itself.  It wouldn't even build to run tests without my change.
<mars> if you tried running 'make', that is
<rockstar> mars, or tried to do anything with zope...
<mars> if you just used bin/test or bin/buildout, as I did, then everything builds fine
<rockstar> mars, bin/test was failing for me.
<rockstar> mars, I suspect it matters what layer you're trying to run your test in though.
<mars> Hmm, bin/test -cvv test_archive_subscriptions failed when that line was missing
<mars> Again, as one of the reviewers, I know there I was in error to assume that ec2 test would be run, and that the missing line was intentional but unmentioned in the cover
<mars> rockstar, I can hit the list with a root cause analysis mail
<rockstar> mars, I don't think as a reviewer you should have to ask for the test suite to have been run, because it was landed with [release-critical]
<rockstar> mars, we make mistakes and land broken code sometimes.  Human error should be allowed.  When things are landed rc, I expect that we've tried to account for as any human errors as possible.
<mars> rockstar, but I am also the release-critical gatekeeper - I should have ensured the run (as I have explicitly asked for in the last two r-c's I gave out.)
 * rockstar nods
<mars> rockstar, I still think a root cause analysis mail is worthwhile here.  We only know one (me) and a half (bigjools via abentley) sides of the story.
#launchpad-reviews 2010-06-29
<bac> hi abentley, can you look at https://code.edge.launchpad.net/~bac/launchpad/bug-597809/+merge/28690 ?
<abentley> bac, sorry, forgot to update the topic.
* bac changed the topic of #launchpad-reviews to: On call:  abentley || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* bac changed the topic of #launchpad-reviews to: On call:  - || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> abentley: ah, right, you were yesterday.  sorry.
* abentley 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
* bac changed the topic of #launchpad-reviews to:  On call:  - || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> leonardr or mars:  are either of you reviewing today?
<leonardr> bac: good question. i guess i'm doing it today--i remember being sick last tuesday
* leonardr changed the topic of #launchpad-reviews to:  On call:  leonardr || reviewing: - || queue: [bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> bac, give me the link to your branch
<bac> thanks leonardr.  i have a trivial branch to start your day.
<bac> https://code.edge.launchpad.net/~bac/launchpad/bug-597809/+merge/28690
<leonardr> bac: why should the privacy sprite not appear on private team pages? is there some other code now that takes care of that?
<bac> leonardr: the privacy sprite is a padlock.  it was never intended to decorate the "This team is private" message.  instead we have two other decorations.  one is a hashed rule that goes down the left side of the page.  the other is the use of the same hash across the top of the privacy portlet with the text "This team is private".  it is consistent with the presentation for bugs and branches.
<bac> leonardr: the <div> i removed that rendered the padlock was leftover from the pre-3.0 design
<leonardr> ok, r=me
* leonardr changed the topic of #launchpad-reviews to:  On call:  leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> thanks
<rinze> leonardr: Hi, can I add another branch to the queue?
<leonardr> rinze, sure
<leonardr> give me the link
<rinze> https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/28750
<rinze> leonardr: Sorry, I just noticed I proposed against lp:launchpad by accident
<rinze> leonardr, I'll get back to you
<leonardr> sure
<rinze> leonardr: https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/28751
* leonardr changed the topic of #launchpad-reviews to:  On call:  leonardr || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> rinze, i must ask some basic questions
<leonardr> what makes an architecture "restricted"
<leonardr> ?
<rinze> leonardr: Some architectures (such as ARM) only have a limited number of slow builders so we don't want to make them available to everybody who is building openoffice in their PPA
<leonardr> ok
<rinze> leonardr: Those architectures are marked as restricted in the database and archives that need to build on those architectures need an explicit entry in the archivearch database table.
<leonardr> rinze:
<leonardr> 	+ terms.append(SimpleTerm(family, token=family.name,
<leonardr> 45	+ title=family.title))
<leonardr> i think our coding style requires putting all the arguments to SimpleTerm on a single new line
<rinze> ah, ok
<jtv> leonardr, I have an oversized branch...  think it's something you could review?  https://code.launchpad.net/~jtv/launchpad/recife-limited-setcurrenttranslation/+merge/28715
<jtv> henninge: maybe you could review the branch we discussed?  It's almost twice as big as it's supposed to be, I'm afraid; those tests are pretty exhaustive.
<henninge> jtv, leonardr: I am taking it.
<jtv> thanks henninge
<leonardr> rinze: what happens if you get the enabled_restricted_families_collection_link. are IProcessorFamily objects actually published in the web service?
<rinze> leonardr: That's a good question, I hadn't actually considered that - just changed the arm_builds_enabled variable which was exposed.
<rinze> leonardr, it looks like it doesn't actually work because of IProcessorFamily not being available so I should probably remove the exported() bit.
<leonardr> rinze: ok, either way you are breaking the backwards compatibility of the web service.
<leonardr> in the unlikely event that someone wrote a web service client that checks .arm_builds_enabled
<leonardr> we have two options
<leonardr> you can reimplement arm_builds_enabled as a property, export in 'beta', and de-export it in 'devel'
<leonardr> (i can show you how to do this)
<leonardr> or, we can decide that it's so unimportant that we can simply remove it retroactively with no worries
<leonardr> flacoste, your opinion? -^
<rinze> leonardr: Sorry, I'm being very clumsy today
<leonardr> rinze: np, it's a complex change
<rinze> It looks like arm_builds_enabled wasn't actually exported, but I made a mistake in making enabled_restricted_families exported.
<leonardr> aha
<leonardr> ok, so simply don't export it, and there will be no backwards compatibility issues
<leonardr> you may want to export it later, but no need to do it now
<leonardr> flacoste, never mind
<flacoste> leonardr: ack
<rinze> leonardr: right - and thanks for pointing that one out
<leonardr> rinze: r=me with changes. i'll summarize in the review
<rinze> leonardr: thanks!
<leonardr> rinze, review written
<leonardr> be sure when you remove exported() to also remove the extra links from your tests
* leonardr changed the topic of #launchpad-reviews to:  On call:  leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rinze> leonardr: will do
<rinze> leonardr: could I add another (simpler) branch to the queue?
<leonardr> rinze, sure
<rinze> leonardr: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/394798-auto-buildd-secret/+merge/28759
* leonardr changed the topic of #launchpad-reviews to:  On call:  leonardr || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* sinzui changed the topic of #launchpad-reviews to:  On call:  leonardr || reviewing: rinze || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, may I hop on your queue?
<leonardr> rockstar, sure add it
* rockstar changed the topic of #launchpad-reviews to: On call:  leonardr || reviewing: rinze || queue: [sinzui, rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> rockstar, give me the mp url as well
<rockstar> leonardr, putting together the mp right now.
<leonardr> great
<leonardr> rinze, what happens if you specify a build secret for a public archive? is that tested in code that you didn't touch?
<rinze> leonardr: yeah, that's already disallowed by code I didn't touch (though I'm not sure if it's tested)
<leonardr> ok
<leonardr> r=me
* leonardr changed the topic of #launchpad-reviews to: On call:  leonardr || reviewing: sinzui || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> sinzui, what's the branch you want reviewed? registry-tales-2?
<rinze> leonardr, thanks
<sinzui> yes please
<leonardr> sinzui, r=me
<sinzui> thanks
* leonardr changed the topic of #launchpad-reviews to: On call:  leonardr || reviewing: - || queue: [rockstar] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, https://code.edge.launchpad.net/~rockstar/launchpad/delete-recipes/+merge/28773
<leonardr> rockstar: i was just about to ask
<leonardr> rockstar, "if self.buildqueue_record:" should be "if self.buildqueue_record is not None:"
<rockstar> leonardr, ah, yeah, that's probably better.
<leonardr> rockstar: do test_destroySelf sets up a recipe build that has no associated build queue record, to test your new conditional?
<rockstar> leonardr, yeah, it does.
<rockstar> leonardr, I actually wrote the test to reproduce the bug first, and then fixed the code to make the test pass properly.
<leonardr> ok, r=me with that minor change
* leonardr changed the topic of #launchpad-reviews to: On call:  leonardr || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rockstar> leonardr, thanks.
<EdwinGrubbs> leonardr: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-576388-proposing-invited-team/+merge/28786
<leonardr> edwingrubbs: sure
* leonardr changed the topic of #launchpad-reviews to: On call:  leonardr || reviewing: Edwin || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> leonardr: I could use a review of https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/28543
<leonardr> bdmurray: ok, you're in the queue
<EdwinGrubbs> leonardr: I need to get some food. Do you have any questions for me about the branch before I go?
<leonardr> edwin: no, sorry, i'm just getting started
<leonardr> i'll put questions in irc if i have them
<leonardr> edwin: i would like to see your test show the string 'Accepted invitation whilst trying to propose the team' somewhere
<leonardr> also, 'while' should be just as good there as 'whilst'
<leonardr> in fact that whole sentence was hard for me to parse. 'Accepted an already pending invitation while trying to propose the team for membership" might be better
<leonardr> other than that, r=me
* leonardr changed the topic of #launchpad-reviews to: On call:  leonardr || reviewing: bdmurray || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<leonardr> bdmurray, can you summarize the old behavior? it sounds like bugs used to say "this bug might expire eventually" and people interpreted this as "it's time for this bug to expire"?
<bdmurray> leonardr: yes, that is what bug 595124 is about
<_mup_> Bug #595124: A bug's can_expire attribute is confusing <dhrb> <easy> <Launchpad Bugs:Triaged> <Ubuntu:Invalid> <https://launchpad.net/bugs/595124>
<leonardr> findExpirableTasks(days_old=60) finds bugs that have not had any activity in 60 days?
<leonardr> bdmurray -^
<leonardr> in that case i'm confused about what isExpirable(days_old=0) means. it means that the bug can expire even if it was just changed?
<bdmurray> leonardr: isExpirable(days_old=0) is what can_expire used to be
<bdmurray> so might expire eventually
<leonardr> the thing i don't understand is where days_old=0 fits into that
<bdmurray> It is for showing on a bug's web page that it will expire in XYZ days
<bdmurray> for example bug 301020
<_mup_> Bug #301020: Video no longer plays on S3 Savage in Intrepid <intrepid> <needs-retested-on-lucid-by-june> <xserver-xorg-video-savage (Ubuntu):Incomplete> <https://launchpad.net/bugs/301020>
<leonardr> ok, it says it will expire in 59 days
<leonardr> so isExpirable(0) must have returned true
<leonardr> but i don't understand the meaning of the zero in there
<leonardr> presumably isExpirable(61) would have returned false?
<bdmurray> well 60 which is the configuration value yes
<leonardr> what is the difference between isExpirable(0) and isExpirable(60)?
<bdmurray>                     AND Bug.date_last_updated < CURRENT_TIMESTAMP
<bdmurray>                         AT TIME ZONE 'UTC' - interval '%s days'
<bdmurray> it is passed to that ^ in findExpirableBugTasks
<leonardr> so isExpirable(0) gets a bug no matter when it was updated -- will this bug eventually expire
<bdmurray> yes
<leonardr> isExpirable(60) only gets bugs that have not been updated in a long time, and 60 days is the de facto point at which expirable bugs that haven't been updated _are_ expired
<bdmurray> correct
<leonardr> ok
<leonardr> in the test, you set a bug to invalid because an invalid bug won't expire?
<bdmurray> right otherwise I would have changed all the other tests
<leonardr> apropos the tests you did change, i don't have enough context to know why all these bugs changed from EXPIRE=True to EXPIRE=False. which change caused that?
<bdmurray> switching from "expire sometime" to "expirable" the 31 is the days old
<leonardr> ok
<leonardr> bdmurray, in incomplete-bugs, why did you switch from a test that adds a comment to a test that directly manipulates a field value?
<bdmurray> because the comment would set date_last_updated to today which is less than 60 days ago
<leonardr> ok
<leonardr> one more question. in places like bugtask-index.pt, i assume you changed can_expire to context.bug.isExpirable(0) for clarity's sake, since the two statements are equivalent?
<bdmurray> they aren't equivalent because can_expire uses 60 days now
<leonardr> ok, in that case you're changing the behavior of the web service in a way that's not backwards compatible
<leonardr> since some bugs that used to have can_expire=True will now have False
<leonardr> however, i don't think it's worth devoting a lot of effort to restoring the old behavior in old versions. flacoste, do you agree?
<bdmurray> okay, is there a policy about that?
<leonardr> the policy is to change behavior only in the 'devel' version of the web service, and maintain the old behavior in all previous versions
<leonardr> and actually, this case might be one i didn't plan for
<leonardr> actually, nm, i have it
<leonardr> i hope flacoste will say this is not a big enough deal to maintain the old behavior, but if he says we have to maintain it, here's what to do
<leonardr> basically you will define two can_expire properties
<leonardr> can_expire and can_expire_beta
<lifeless> hi
<lifeless> whats the protocol to get something reviewed - put your hand up and wait ?
<leonardr> can_expire will be the one you have now, but you will only publish it in version 'devel' of the web service
<leonardr> lifeless: usually, except i'm at eod, so i won't be able to review your branch. i suggest picking a specific person who's around and asking them
<lifeless> ok
<leonardr> bdmurray: can_expire_beta will be implemented as per the old behavior, and you will publish it as 'can_expire' starting in the 'beta' version and remove it as of the 'devel' version
<lifeless> I'll start with a 'is there any one here up for reviewing two small refactoring branches'
<lifeless> :)
<leonardr> bdmurray: for an example, take a look at lazr.restful/src/lazr/restful/example/multiversion/resources.py
<leonardr> you'll see that a_comment first shows up in version 1.0 as 'comment' -- you'll want to make can_expire_beta show up in version beta as 'can_expire', and can_expire show up in version devel
<leonardr> you'll also see that 'deleted' is set to exported=False in version 3.0. you'll want to set can_expire_beta to exported=False in version devel
<leonardr> i will put this information into the review
<bdmurray> leonardr: great, thanks
<lifeless> gary_poster: are you EOD yet?
<gary_poster> on call
<lifeless> do you mean 'on a phone call' ? [conflation with 'on call' in the topic]
* 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
<leonardr> lifeless, he's probably on the phone
<leonardr> flacoste, i'd like your opinion on bdmurray's branch https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/28543
<flacoste> https://lpstats.canonical.com/graphs/LPProjectBugTriage/
<lifeless> trending down
<lifeless> sinzui: perhaps you are around and not EOD ?
<sinzui> I am about
<lifeless> I wrote a couple of patches in the weekend
<lifeless> prompted by our discussion about the milestone page performance
<lifeless> I was following my nose into the code
<lifeless> they seem like improvements to me, and I'd like to get them reviewed.
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/lsprof/+merge/28583
<lifeless> and
<lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/lognamer/+merge/28574
<lifeless> if you have the time, some feedback would be great
<sinzui> I will review them now
<lifeless> thank you!
<gary_poster> lifeless: thanks for changes.  I need to run, but will look more closely later.
<gary_poster> I misunderstood the goal of the lognamer branch (I thought you were tackling making the naming cross-process-safe) but now that I understand, cool.  I like the new explanatory comment, but I don't think an XXX (which we exclusively mean to be associated with an LP bug) is appropriate myself, because I doubt we care enough to change it.  It is more of a warning.
<gary_poster> For the other branch, I also like the changes on first blush.  You do have this comment: "<!-- Would be nice to turn this off in production -->".  It's possible and even easy to do that if you know the right incantation...I don't remember but lemme see if I can find it really fast...
<lifeless> gary_poster: thank you
<lifeless> gary_poster: I'd be delighted to make it cross process safe; the XXX I can downgrade, but it is actually unsafe at the moment, and that should be recorded - theres an expectation mismatch presently.
<gary_poster> lifeless: Apparently there is an expectation mismatch, but I've categorized that in my mind as a documentation/understanding problem, not a code problem.  Arguable, and I won't argue it now. ;-)
<lifeless> gary_poster: :)
<lifeless> gary_poster: no need to argue, as you request I'll down grade it to a non-XXX note; I was trying to be clear is all
<gary_poster> no, understood.  I would have gone along with the XXX if you really wanted too (but with a bug)
<gary_poster> ok
<gary_poster> so, you need zcml:condition
<lifeless> gary_poster: bugs that folk don't have time to work on are a bit close to busywork ;)
<gary_poster> agreed :-)
<gary_poster> in your configure element, or in an include, you say something like zcml:condition="have apidoc" except you test on the devel state...
<gary_poster> trying to remember how to do that
<lifeless> gary_poster: don't worry about it
<gary_poster> ./zopeappsession.zcml:106
<gary_poster> OK
<gary_poster> anyway
<lifeless> gary_poster: its currently on in prod
<gary_poster> heh
<lifeless> removing it is orthogonal to making it event driven
<gary_poster> ok
<gary_poster> true
<gary_poster> ok, running away
<gary_poster> bye
<lifeless> I am likely to keep pulling on this thread to get actual kcachegrind profiles
<lifeless> ciao
<gary_poster> yay!
<lifeless> sinzui: thanks for the reviews, I has replied.
#launchpad-reviews 2010-06-30
* 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 On call:  - || 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:  jtv || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Anyone willing to review a branch for the OCR?  I've been leaning on henninge far too much for my reviews.
<jtv> noodles775, you could do this!
<jtv> Willing to review https://code.edge.launchpad.net/~jtv/launchpad/extract-approval-js ?
<jtv> Ahem... MP is https://code.launchpad.net/~jtv/launchpad/extract-approval-js/+merge/28852
<noodles775> Sorry jtv - I'm working to get an urgent feature done right now...
 * jtv continues his search for prey
<jtv> rockstar, on the off-chance you're working right now: I'm hoping to get a javascript review...  https://code.launchpad.net/~jtv/launchpad/extract-approval-js/+merge/28852
<rinze> jtv: I can code review, but I'm not a UI reviewer
<jtv> rinze: that's fineâthere's no UI change apart from chromium now showing things properly like other browsers.
<jtv> rinze: is javascript a problem?
<rinze> no, javascript should be fine
<jtv> And... change of nick?
 * rinze claims the MP
<jtv> thanks
<rinze> jtv: I switched to two nicks that I can connect/disconnect independently, one for work and one for other FOSS stuff
<jtv> begrijpelijk
<rinze> jtv: I haven't forgotten your branch but it's taking a wee bit longer since my JavaScript is a bit rusty.
<jtv> rinze: in that case, it'd probably be better to go over it with a javascript-specialized reviewerâgets you unrusted and also lets you pick up JS coding guidelines.
<rinze> jtv: Yeah, that'd be useful, though there doesn't appear to be one around at the moment :-(
<jtv> ...and there's the catch.
<rinze> (I've just been looking at https://dev.launchpad.net/JavaScriptReviewNotes)
<jtv> rinze: looking at those myself now... I see that my documentation needs some tweaks.
<bigjools> jtv: hi
<jtv> hi bigjools
<bigjools> jtv: can you review this for me please: https://code.edge.launchpad.net/~julian-edwards/launchpad/copy-archive-fix-component/+merge/28871
<bigjools> it's going to be a CP
<jtv> bigjools: so speed matters... right ho
<bigjools> thanks
<jtv> rinze: I was in the process of updating my documentation; will finish that after julian's review.  BTW I think I forgot to mention that there was no lint.
<rinze> rinze, ok
<bigjools> rinze: how many other nicks have you got? :)
<jtv> bigjools: approved
<bigjools> jtv: cheers
<bigjools> now, I need flacoste to do the same :)
<jtv> quite
<jtv> I suppose you can land on devel in the meantime.
<rinze> bigjools: just these three :-)
<rinze> jtv: I also have another branch up for review
<jtv> rinze: let's have it then
<rinze> https://code.edge.launchpad.net/~jelmer/launchpad/600153-main-archive-builds-on-restricted/+merge/28867
<jtv> rinze: did you run "make lint" btw?
<rinze> jtv: No, I haven't run make lint
<jtv> Make it a habit.  We're supposed to do that before submitting the MP.
<rinze> ok
<jtv> Also, I know it's not your fault (or at least not in this branch), but could you give _get_enabled_restricted_families and _set_enabled_restricted_families docstrings explaining what their purpose is?  If they're methods, give them dromedaryCase names rather than underscore_separated names.
 * rinze nods
<jtv> Also, in _set_enabled_restricted_families please consider avoiding that nasty line break in the "if" condition.  Not very important, but would-be-nice.
<bigjools> jtv: I have another branch up, not urgent though, at your leisure!
 * bigjools needs to eat
<jtv> bigjools: on the queue please
<rinze> jtv: I considered that but I couldn't think of an easy way of doing that without adding a nested if statement
* bigjools changed the topic of #launchpad-reviews to: On call:  jtv || reviewing: - || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bigjools> done, ta
<jtv> Thanks.  My fault for not updating the topic.
<jtv> On call:  jtv || reviewing: rinze || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* rinze changed the topic of #launchpad-reviews to: On call:  jtv || reviewing: rinze || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> rinze: in this case a nested would not be _that_ bad if you combine it with a temporary variable.
<jtv> rinze: thanks  :)
<jtv> rinze: btw I was wondering... what does CannotRestrictArchitectures._fmt do?
<rinze> jtv: I believe it's used for the stringified version of the exception
<jtv> rinze: stringified version?  You mean for __repr__'ing it?
<jtv> rinze: come to think of it I'd prefer it if you just passed the error message to the constructor.
<rinze> I meant __str__, but now that I'm checking it doesn't actually appear to be the case.
<rinze> it's what some of the other exceptions used as well
<rinze> jtv: That makes sense
<jtv> Also avoids changes between python versions... the underscore sort of says "don't mess with this, it looks funny and there's a faint ticking noise coming out of it."
<jtv> rinze: btw lint doesn't complain about it, but try to avoid trailing whitespace as well.
<flacoste> bigjools: what do you need me for?
<jtv> rinze: you have a line overrun in your docstring for test_main_archive_can_use_restricted.  You can fix that by turning it into a commentâwe don't usually write docstrings for test methods.
<jtv> rinze: and have a look at TestCase.assertContentEqual; will save you a bit of listification there.
<rinze> rinze: The style guide is ambiguous in that regard: "Write in the docstring of each test case what is being tested. As a special case for test methods, a comment block at the beginning of the method is considered an acceptable substitute to a docstring."
<rinze> s/rinze/jtv/
<rinze> jtv: Ah, I wasn't aware of that one - thanks
<jtv> rinze: we don't usually write docstrings there... saves us having to follow the docstring format.
<jtv> So don't feel too bad about being merely "considered acceptable."  :-)
<rinze> heh, ok
<jtv> bigjools, line 66 of your diff: s/Lets/Let's/
<bigjools> grar
<jtv> But frankly starting paragraphs with that is usually an indicator that you're trying to pass off setup code as documentation.
<jtv> You may just want to say something like "we have a" rather than "let's make a"
<bigjools> I welcome any recommendation to make it better
<jtv> Do it all in French!
<jtv> no wait
<bigjools> Enc...
<bigjools> no waut
<jtv> ?
<bigjools> typing is clearly not my strong point today
<jtv> bigjools: instead of...
<jtv> https://code.launchpad.net/~julian-edwards/launchpad/api-commercial-ppas-bug-597211/+merge/28877
<jtv> argh
<jtv> Instead of...
<jtv> store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
<bigjools> are we putting the missing U back?
<jtv> ...you can just say...
<jtv> store = IStore(Archive)
<jtv> The missing U back?  I wish.  But now we can say it's a metric U and some people just haven't gotten around to adopting it yet.
<bigjools> does that select a slave or master depending on the operation?
<jtv> Yes, that's the default flavour.
<bigjools> cool
<jtv> There's also IMasterStore and ISlaveStore.  Thank stub for that.
<bigjools> we should do a branch to fix all of those at once
<bigjools> yeah I know, I have a jihad branch that is forcibly choosing ISlaveStore but it breaks a million tests
<jtv> In some places you'll need to commit just to make the slave catch up.
<bigjools> I know -  but it's not as simple as that
<jtv> bigjools: I was hoping for more here :)
<jtv> what complications do you have in mind?
<bigjools> I'm gonna take a new strategy of using the Slave policy in top-level scripts and cast objects to IMasterObject where I need to write them
<jtv> BTW with those changes the branch looks good to me.  Nice work.
<stub> If IStore(Archive) doesn't work because it is not derived from SQLBase (not sure), they can review my branch with the one line fix.
<jtv> bigjools: I just approved your branch
<bigjools> jtv: grassyass
<jtv> rinze, how's yours coming along?
<jtv> bigjools: de nutter
* jtv changed the topic of #launchpad-reviews to: On call:  jtv || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * jtv is taking a break
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call:  jtv, Edwin || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv-afk> hi EdwinGrubbs!
<jtv-afk> Maybe you can review one of mine?
<EdwinGrubbs> jtv-afk: sure
<jtv-afk> EdwinGrubbs: e.g. https://code.launchpad.net/~jtv/launchpad/bug-580337/+merge/28880
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call:  jtv, Edwin || reviewing: rinze, jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> jtv-afk: what does "test_getVariable_smoke" mean? I don't see anything called "smoke".
<jtv> EdwinGrubbs: smoke test :)
<EdwinGrubbs> I kinda guessed, but it seemed a little vague.
<EdwinGrubbs> jtv: review sent
<jtv> EdwinGrubbs: thanks!
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call:  jtv, Edwin || reviewing: rinze || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> jtv: btw, do you have any thoughts on the email I sent about adding code to POTemplate.importFromQueue() to cache the message count?
<jtv> EdwinGrubbs: oh, I swapped that out... hang on
<jtv> EdwinGrubbs: istm this count doesn't have to be time-accurate so an offline job may be more suitable.  Apart from that it made sense.
<jtv> But is the caching necessary?  Summing the template message counts for a package shouldn't be too costly.
<jtv> (As long as you don't go doing it in python)
<EdwinGrubbs> jtv: the problem occurs because the +needs-packaging page sums the message counts for each sourcepackage and then sorts the sourcepackages based on that.
<jtv> EdwinGrubbs: SQL-side or python-side?
<EdwinGrubbs> jtv: it's summed in sql
<jtv> Oh, it's the old "sort before you batch" problem?
<jtv> Where you don't have any performance problems thanks to batching, _except_ determining the order to batch in scales really badly with overall table size?
<EdwinGrubbs> jtv: I'm assuming that there are multiple POTemplates per sourcepackage, since we are using a subquery. If it was one-to-one, we could join instead.
<jtv> Well you can still left-joinâit's just a question of how costly that is.
<EdwinGrubbs> does the message count typically change more than once a day for each sourcepackage? If not, a daily cronjob would actually be more db intensive.
<jtv> Unless we pick out only the templates that _have_ changed.
<jtv> A message queue would be ideal, but timestamps would do it as well.
<EdwinGrubbs> jtv: are you saying it's either 1-to-1 or 1-to-0? I thought it was 1-to-many, which a left-join wouldn't help.
 * EdwinGrubbs feels like I'm talking about world cup scores.
<jtv> It's one-to-many... but you can still "group by"
<jtv> select sourcepackagename.id, sum(messagecount) from sourcepackagename join potemplate on potemplate.sourcepackagename = sourcepackagename.id group by sourcepackagename.id;
<EdwinGrubbs> jtv: how is POTemplate.date_last_updated updated? I don't see a trigger, and the POTemplate class doesn't seem to do it automatically or in importFromQueue().
<rinze> jtv: I've pushed an updated version of my branch
<jtv> EdwinGrubbs: in POTFileImporter
<jtv> rinze: cool, thanks... I'll get on it in a minute
<jtv> rinze: in ll.66â67, could you capitalize and punctuate the error string?
<jtv> EdwinGrubbs: so it is up-to-date, though it may be python-side time rather than database-side time.  But I guess that could be fixed if needed.
<rinze> rinze, yep
<jtv> rinze: you're talking to yourself again :)
<rinze> doh
<jtv> rinze: anyway, with that one remark, I'm approving.  Please give me a moment to document.
<jtv> rinze: done
<rinze> jtv: thanks!
<jtv> np
 * jtv is about done
* 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
<rinze> EdwinGrubbs: Can I add a branch to your queue? It's a simple one to fix the merge of stable into db-devel
<EdwinGrubbs> rinze: sure
<rinze> EdwinGrubbs, https://code.edge.launchpad.net/~jelmer/launchpad/stable-fix/+merge/28912
<EdwinGrubbs> rinze: this branch appears to have a lot more than resolved conflicts. Is there another branch that was already reviewed but never landed?
<rinze> EdwinGrubbs: It merges stable into db-devel so it's got all the other changes from stable as well
<rinze> EdwinGrubbs: I don't have much experience fixing these sorts of merges, I wonder what the best way is to present these changes for review.
<EdwinGrubbs> rinze: it's a good idea to add a comment to the mp which shows just the two files that you had to modify.
<rinze> EdwinGrubbs, the two files are lib/lp/soyuz/{configure.zcml,model/archive.py}
<EdwinGrubbs> rinze: what editor do you use?
<rinze> EdwinGrubbs, vi
<EdwinGrubbs> rinze:  your branch contains an unusual amount of dangling whitespace and tab-characters as opposed to spaces. Most of us have vi or emacs configured to highlight these. See https://dev.launchpad.net/UltimateVimPythonSetup
<EdwinGrubbs> rinze: the most import configs are
<EdwinGrubbs> highlight WhitespaceEOL ctermbg=red guibg=red
<EdwinGrubbs> match WhitespaceEOL /\s\+$/
<EdwinGrubbs> and
<EdwinGrubbs> setlocal list listchars=tab:>~,trail:*
<rinze> EdwinGrubbs, thanks
<EdwinGrubbs> rinze: actually, setlocal should be just 'set' unless it is called from a command that checks the filetype of each newly opened file.
<EdwinGrubbs> rinze: btw, at the very bottom of that wiki page there is a branch that I made that incorporates almost all the configs, so you don't have to copy and paste or download dependencies.
<rinze> EdwinGrubbs, thanks for the review
<rinze> EdwinGrubbs, and thanks for the hint about the vim config branch, I'll have a look at it sometime
<abentley> EdwinGrubbs, could you please review https://code.edge.launchpad.net/~abentley/launchpad/fix-stuck/+merge/28924 ?
<EdwinGrubbs> abentley: sure
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call:  Edwin || reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> abentley: looking at the .bzrignore, it seems odd that the source package files would be placed directly in the lib/canonical/ directory and not in a subdirectory.
<abentley> EdwinGrubbs, IME, building a package places the files in the parent directory.
<EdwinGrubbs> oh, I see, you didn't want to have two levels of temp directories.
<EdwinGrubbs> abentley: it also seems like SourcePackageRecipeBuildManager.initiate() would benefit from adding assert isinstance(self.author_name, unicode) so you get an early warning if None or a str is passed in.
<abentley> EdwinGrubbs, how would that be a benefit?
<EdwinGrubbs> abentley: well, it would only catch tests that initialize the object without running it. I'm not sure if that is even plausible.
<EdwinGrubbs> abentley: r=me
<abentley> EdwinGrubbs, thanks!
<abentley> rockstar, could you please review https://code.edge.launchpad.net/~abentley/launchpad/process-upload-no-cwd/+merge/28682 ?
<rockstar> abentley, ack
* sinzui changed the topic of #launchpad-reviews to: On call:  Edwin || reviewing: abentley || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bdmurray> leonardr: my mp is ready for re-review
<leonardr> bdmurray: ok, i'll get to it tomorrow morning. remind me if i don't get to it
<bdmurray> leonardr: okay thanks
* EdwinGrubbs changed the topic of #launchpad-reviews to: On call:  - || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> sinzui: I didn't see your branch in the queue. I can do it later today.
<sinzui> EdwinGrubbs, okay
#launchpad-reviews 2010-07-01
<lifeless> sinzui: if you're back, I've moved the profile module to lp.services as per your request, but it looks to me like it does not meet the namespace policy about deps. I've mailed the bug
<lifeless> s/bug/mp/
* rinze changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<rinze> sinzui, hi
<noodles775> Hi salgado, if you've got a spare few mins, can you take a look at the issue I've identified at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885
<noodles775> Let me know if I'm not making sense on the MP.
<salgado> noodles775, sure, I have it in my inbox and will get to it shortly
<noodles775> salgado: thanks.
<salgado> noodles775, in which case is the sreg extension not included in the response?
<noodles775> salgado: normal login with existing account. ie. 1) start dev server, 2) click login 3) enter admin@canonical.com:test 4) click login. In this case calling _getEmailAddressAndFullName raises the assert.
<noodles775> It's not seen in the current code as _getEmailAddressAndFullName is only called if the account doesn't exist.
<salgado> noodles775, that's weird because the code that generates the positive openid response is including the sreg extension unconditionally
 * salgado tries to dupe
<salgado> indeed, duped
<noodles775> Yeah, I'd assumed the sreg response should always be there, and was surprised too.
<salgado> noodles775, but this is a bug in the testopenid implementation
<noodles775> Where should I look?
<salgado> testopenid == the OpenID provider we use when developing/testing
<salgado> I'm checking it now
<salgado> noodles775, lib/lp/testopenid/browser/server.py
<noodles775> OK, so we can be confident that the staging/production SSO always include the sreg.
<salgado> createPositiveResponse() adds the sreg extension, but it's empty
<noodles775> Thanks.
<salgado> noodles775, I think I've found the problem
<salgado> noodles775, OpenIDLogin.render() asks for 'email' and 'fullname' in the sreg response
<salgado> bug testopenid ignores that and only includes the nickname
<salgado> s/bug/but
<noodles775> Right... I see.
<noodles775> So should it instead include *only* the requested items (email/fullname) or in addition to the nickname?
<salgado> I think it's fine to include the 3 of them, but the nickname shouldn't be used anywhere
<noodles775> OK, I'll update it... thanks salgado !
<noodles775> salgado: also, I assume you don't have time to review the actual branch, but if you could have a brief look and make sure you think it's a sane change, that would be great.
<salgado> noodles775, sure, I can do that
<noodles775> Ta.
<salgado> noodles775, out of curiosity, why somebody needs a launchpad account to buy stuff from the software center?  wouldn't a Ubuntu SSO account be more appropriate in this case?
<noodles775> salgado: yes, the must already have an SSO account to make the payment, it's after they've paid for the software, we need to give them access to a private PPA (ie. a private ppa subscription), and that's only possible if you've got an LP account.
<salgado> I see
<salgado> and it'd be lots of work to make it possible for someone to have a private ppa subscription without having a Launchpad profile, I assume?
<noodles775> Yes, but that is the long-term plan.
<salgado> cool!
<salgado> 415	+        except LookupError:
<salgado> 416	+            assert email_address is not None and full_name is not None, (
<salgado> 417	+                "An email address and full name are required to "
<salgado> 418	+                "create an account.")
<salgado> noodles775, I'd move that assert out of the except block
<noodles775> That won't be required... I'd only added it because of the above issue.
<noodles775> Hrm, although, it's still possible for other callsites in the future to pass in None (even if processPositive.. doesn't).
<noodles775> So yep, will do.
<salgado> indeed
<salgado> noodles775, I think you can get rid of checked_for_email and the two email_set.getByEmail(email_address) calls by doing "email = email_set.getByEmail(email_address)" at the beginning instead of "email = None"
<noodles775> salgado: yep - that was part of r9514 which I've reverted after our discussion (I couldn't do it at the beginning at that point, because email was None when called via processPositive..)
<salgado> right
<salgado> I guess I should reload the m-p page, then
 * noodles775 hasn't pushed... hangon.
<noodles775> OK, pushed r9515...
<salgado> noodles775, IPersonSet.getOrCreateByOpenIDIdentifier() has a requestor argument but the actual implementation doesn't
<noodles775> salgado: thanks, I'll remove it from the IFace. I've just pushed 9516 which just adds the email/fullname to the sreg response for the testopenid server too.
<salgado> cool, let me reload again
<noodles775> Thanks salgado... I'll paste the irc conversation on the MP too. (And I've pushed the changes you suggested so far). I think that should be enough to get a normal review.
<salgado> noodles775, np
* noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> jelmer-lunch: if you've time when you get back: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885
<jtv> jelmer-lunch: my windmill tests pass now...  I think there was some change in setup that we both needed to synchronize to.
<jelmer_> noodles775: ok
<jelmer_> jtv: I'll have another look, thanks.
<noodles775> Sure.
<jtv> noodles775: anything to add to jelmer's review?
<noodles775> jtv: I pasted our irc conv. on the mp.
<jtv> noodles775: that's all about us being rusty and making dirty jokes afaics, nothing about the branch itself...
<jtv> Ah, apart from the bit about henning's xxx that triggered the jokes
<jtv> rinze: the tests on my JS branch are passing now, repeatedly and consistently... do I get your Approval vote?
<jtv> noodles775: if you approve of the js, maybe you can hit the js review button?
<noodles775> jtv: no, I was just making sure rinze hadn't missed anything obvious... I didn't do a proper review. As you mentioned in your MP, the branch is just cleaning up already working JS (outside of the chromium fix - just a different class)..
<noodles775> Oh, but I can hit the js review button if you need it (sorry).
<jtv> I'd love that, thanks!
<rinze> jtv: Yeahp!
<rinze> jtv: Updating the MP now
<jtv> rinze: great!  Then I can still get it in before the deadline.
<rinze> Also, if somebody here can review a oneliner: https://code.edge.launchpad.net/~jelmer/launchpad/fix-devel/+merge/29011
<rinze> It's the testfix for devel
<noodles775> rinze: done (and jtv too)
<jtv> thanks
<jtv> rinze: least I can do...
<jtv> oh, too late :-)
<rinze> jtv: noodles beat you to it, but thanks anyway :-)
* bigjools changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [sinzui, noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* rinze changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles775 || queue: [bigjools]  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * rinze looks at bigjools branch briefly
<rinze> bigjools: there's a typo in paramter in your angry comment :-)
<bigjools> I was too angry to speel
<rinze> in lib/lp/soyuz/scripts/tests/test_populatearchive.py nonvirtualized seems to be a boolean parameter, yet it defaults to None and checks for "is not None"
<rinze> I.e. if testCopyArchiveVirtualization specified nonvirtualized=False it would still not use virtualization
<bigjools> rinze: yes deliberately, but I suppose it should default to False
<bigjools> I was still too angry to think :)
<bigjools> both fixed
<rinze> bigjools: did you push your changes?
<bigjools> rinze: no, I was waiting for more comments about my angry coding :)
<rinze> hehe
<rinze> bigjools: Tests pass here and I don't see any other issues.
<rinze> perhaps you could add an assert somewhere that self.makeCopyArchive() can also still create archives that require virtualized builds
<bigjools> yeah I saw that problem as you mentioned it too
<bigjools> merde
<rinze> bigjools: in your latest tip, the nonvirtualized argument to makeCopyArchive defaults to None rather than False
<bigjools> rinze: some changes pushed up again
<bigjools> ARGH
<bigjools> missed it
<bigjools> ok pushed up again
<rinze> bigjools: I don't see the diff for that last change yet, but I trust that's correct :-) r=me
<bigjools> thanks rinze :)
<bigjools> saving me from my own stupidity on that branch
* rinze changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles775 || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: review sent (finally)
<jtv> ahhh!
<jtv> henninge: hope it's an approval!
<henninge> jtv: almost
<jtv> grrr
<henninge> jtv: I mean, there are no big issues but I'd like to hear your opinions on some of my comments.
<henninge> ;-)
* rinze 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
<bigjools> is anyone around for a simple but urgent review?
* rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
 * bigjools meets rockstar here
<rockstar> bigjools, I've come to the conclusion that soyuz is generally hard to understand unless you know the whole entire subsystem.
<rockstar> The latter has become a new goal of mine.
<bigjools> attaboy
<bigjools> I still learn stuff 3.5 years after I started on it....
<rockstar> bigjools, why do you "create copy archives for rebuild testing"? That's not a PPA thing, right?
<bigjools> rockstar: no, it's more like a main archive presented like a PPA.  It allows us to copy source from somewhere else and rebuild it
<rockstar> bigjools, presented like a PPA?  What's the use case there?
<bigjools> UI code re-use :)
<rockstar> (Patch looks fine, just trying to squeeze a few user stories out)
<bigjools> we didn't want it looking like the normal distro pages
<bigjools> the main use case is to find failed builds
<rockstar> Why shouldn't it look the same?
<rockstar> Er, why shouldn't it look like the normal distro pages?
<bigjools> they're overkill for this type of archive, mainly
<rockstar> bigjools, so I guess the thing I'm not understanding is what type of archive this is.
<rockstar> Is it a distro archive or a PPA or something else I don't know about.
<james_w> it's a third type
<rockstar> james_w, which is?
<rockstar> Is it like a throwaway archive just for rebuilding copies of existing packages?
<james_w> rockstar: pretty much
<rockstar> james_w, ah, okay.  That parses better now.
<james_w> it's grown some extra features, but the primary use case is to copy sources from Ubuntu in to a new archive, and then rebuild them there
<james_w> so the bug in question is where it copies a bunch of sources, and then fails while creating the build records for them
<rockstar> Yeah, that makes sense.
<james_w> in particular because the new archive doesn't have i386 enabled, and i386 is required to build arch "all" packages, it's the nominatedarchindep
 * bigjools watches james_w explain how my branch works :)
<bigjools> sorry - had to dash off, child-related poo emergency
<james_w> nice
<bigjools> actually it wasn't :(
<rockstar> Yeah, I very much understand nominatedarchindep.  Remember that time that recipe builds were using the arm builder...
<bigjools> unfortunately yes
<james_w> damn you bigjools, I've now started typing poopy instead of poppy
<james_w> rockstar: a small branch of my own if you would: https://code.launchpad.net/~james-w/launchpad/move-file-is-a-directory/+merge/29033
<rockstar> james_w, I live serve...
<rockstar> Er, I live to serve
<james_w> \o/
<rockstar> I apparently don't live to type correctly.
<bigjools> james_w: haha
<bigjools> rockstar: thanks for the review
<rockstar> james_w, your branch is made of awesomeness.  Thanks for doing that.
<bigjools> rockstar: aw jeez you Quoted me!
<rockstar> One day, we'll have all our dependencies nice and neat.
<rockstar> bigjools, yeah, it had to be done.
<rockstar> bigjools, also, you're like, the 6th person today to tell me about poo related mishaps that occurred today.
<bigjools> one day you will have kids and will enthusiastically join in
<james_w> thanks rockstar
#launchpad-reviews 2010-07-02
* noodles775 changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: for which branch do you need a review?
<noodles775> adeuring: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885
<noodles775> Thanks!
 * adeuring is looking
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> I asked salgado to take a brief look yesterday (as it's his area), and he's commented, but it needs a proper review.
<noodles775> adeuring: also, i've got another branch that's not ready for review, but I need some help (related to private XMLRPC authentication - I'm a little unfamiliar) if you've got any input: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-priv-xmlrpc-access-getOrCreate/+merge/29059
<adeuring> noodles775: so, you want to add another celebrity?
<noodles775> adeuring: that's already done (and landed by StevenK). At the moment I'm just trying to write a test that logs in as admin and passes.
<noodles775> The xmlrpc call works fine (I can switch to zope.Public and the tests work), but I can't see how to assign launchpad.Admin to the xmlrpc view (or if I even should be going down that direction - I can't see any other examples in the code)
<noodles775> Hrm, maybe the test should be authenticating differently, or the logged in user needs ssh keys etc.
<adeuring> noodles775: as I understand it, you just need to ensure in checkAuthenitcated that the current user is your celebrity. Or am I missing something?
<noodles775> adeuring: that's what I would have thought, but that breakpoint is never being called (and yet the tests return 401 Unauthorized)
<adeuring> noodles775: seems that some other authentication check is kicking in then?
<noodles775> Yep... I'll check if it's related to ssh keys on the test users.
<adeuring> is there any auth class for the same permission and interface class?
<noodles775> Nope - it's a new class in that branch.
<adeuring> noodles775: after looking closer, I must ammit that I have no clue why this doesn't work...
<noodles775> adeuring: ok, thanks for looking :D
<noodles775> adeuring: found it... I needed to supply the credentials when creating the xmlrpc server proxy... obvious now.
<adeuring> noodles775: uuhhh, yes ;)
<noodles775> But I still need to find out how I can ensure other users have launchpad.Admin on the view...
 * noodles775 keeps looking.
<adeuring> noodles775: overall, your code looks good. Just one question: SHouldn't the "with MasterDBPolicy():" be moved from processPositiveAssrtion() to getOrCreateByOpenidIdentifier()?
<noodles775> adeuring: yes, you're right.
<adeuring> noodles775: OK, so r=me (including aonther nitpick about a typo)
<noodles775> Thanks adeuring
<adeuring> On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<noodles775> adeuring: the other one is ready now if you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-priv-xmlrpc-access-getOrCreate/+merge/29059
<adeuring> noodles775: I'll look
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<adeuring> noodles775: again, a nice branch. But I think it is a bit odd to have a class PersonSetAPIView (ie.e.,a quite generic name) where the method getorCreateByOpenIDIdentifier() unconditionally providesthe rationale "software centerpurchase" as a account creation rationale. Shouldn't we either make the class name more specific or provide the rationale as a parameter of the method?
<adeuring> (or use a method name like getOrCreateForSoftwarePurchase())
<rinze> adeuring, can you review a oneliner (testfix) ?
<adeuring> rinze: sure
<rinze> https://code.edge.launchpad.net/~jelmer/launchpad/fix-db-devel/+merge/29070
<rinze> a warning about the completeness of the dailybuilds feature was added recently, I suspect that broke it
<adeuring> rinze: well, the change looks straightforward, r=me.
<rinze> adeuring: Thanks
<noodles775> adeuring: yes - I thought the same when I pushed the last rev. So I'd be keen to change the application name... most of the other private xmlrpc apps have names like 'ICodehostingApplication' identifying who uses them.
<bac> hello adeuring.  busy review day?
<adeuring> bac: quite intersting branches from noodles775 :)
<noodles775> So perhaps, ISoftwareCenterAgentAPI, http://..../softwarecenteragent etc.?
<adeuring> noodles775: ok, so something like s/Person/Shopper/ ?
<bac> yes, i saw.  looks like fun...
<adeuring> ...in the app name?
 * bac relocates
<noodles775> The app name usually matches the API, so perhaps ISoftwareCenterAgentApplication...
<adeuring> noodles775: ah, right. so, that, combined with a method name like getOrCreateShopCustomer()?
<noodles775> Sounds good.
<adeuring> noodles775: ok, r=me
<noodles775> w 5
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: noodles775 , -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: your reply looks good but you did not push your changes. Did you rename "get_all_important_translations" ?
<jtv> henninge: knew I forgot something!  hang on
<jtv> (Well, I didn't know you'd be this quick so technically I just hadn't gotten around to it yet)
<henninge> jtv: I guess you'd like to be through with this before the game ... ;-)
<henninge> jtv: also, since this branch only contains tests, it should not need a full test suite run, should it?
* adeuring changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -, -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> Thanks for reminding me of the game.  :-)  I started incredibly early today, so don't think I'll do much after.
<jtv> henninge: that's the theory.  Practice doesn't always follow theory.  :)
<henninge> jtv: one more branch after this and we can merge db-stable again?
<jtv> henninge: yes, that's D.'s use-upstream-translations branch.
<jtv> That will also enable us to run the "mirror" tests that I commented out.
<jtv> henninge: oh, and I snuck something extra in: I defined flushing orders when disabling TMs
<jtv> Push is underway.
<henninge> Ah, once bitten ...
<jtv> henninge: push is done, diff updated...
<jtv> bac, got time for this one? https://code.launchpad.net/~jtv/launchpad/bug-181254/+merge/29056
<bac> jtv: yes, please add to topic
<jtv> bac: "reviewing" or "queue"?  :-)
* jtv changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> queue, of course
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,foxxtrot || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> awwww
<henninge> jtv: what?
<henninge> oh, ok
<jtv> bryceh: thanks for going the extra km in cleaning up the branch...  this is to let you know that I'm approving and you should still be able to land
<jtv> henninge: my branch ok now?
<henninge> jtv: most likely ... ;)
<jtv> the suspense is killing me!
<henninge> and
<henninge> the
<henninge> winner
<henninge> is
<henninge> ...
<henninge> jtv!
* bac changed the topic of #launchpad-reviews to: On call: adeuring, bac || reviewing: -,jtv || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> jtv: r=me
<henninge> thanks, looks very good ;-)
<henninge> but now that you changed actual code, you will have to run it through ec2 :-P
<bac> henninge:  which jtv branch did you review?
<henninge> jtv: oh, not the one he asked about today.
<henninge> bac: ^
<henninge> this is three days old.
<henninge> https://code.edge.launchpad.net/~jtv/launchpad/recife-limited-setcurrenttranslation/+merge/28715
<bac> oh, ok. henninge just trying not to duplicate work
<jtv> henninge: thank you for your kind approval...  I'm out of here!
<jtv> bac: I'll keep an eye out for any questions you may have, but be aware that Holland is about to face Brazil and I may be distracted or sobbing uncontrollably.  :)
<bac> jtv:  good luck.  you'll need it.
<jtv> indeed!
<bac> jtv: can you keep us posted here?
 * bac is surprised there has not been more WC chatter
<jtv> bac: ok...  my first time as a commentator
<bac> i watched the ger-eng game on univision.  having spanish play-by-play was much less annoying
<bac> gooooooooooooooooooooooooooool alemania!
<henninge> YEah!
<henninge> ;-)
 * jtv relocates to TV
<henninge> jtv, danilos: Err, the potemplate api branch was never approved.
<henninge> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525371/+merge/25423
<danilos> henninge, isn't there a resubmitted MP which jtv approved?
<jtv> ?
<henninge> danilos: I hope there is
<jtv> (wc commentary: Brazilian players looked nervous and tense... is their national anthem particularly difficult like the Argentinian one?)
<henninge> adiroiban: Hi!
<jtv> (the Dutch players just unfolded a large banner "say no to racism"âa superfluous statement given the diverse composition of the team)
<adiroiban> henninge: hi
<henninge> adiroiban: which is the MP that jtv approved for the POTemplate API?
<henninge> because this one isn't:
<henninge> https://code.edge.launchpad.net/~adiroiban/launchpad/bug-525371/+merge/25423
<adiroiban> henninge: Danilo was the one doing the final review
<henninge> adiroiban: hm, it was landed as [r=jtv], though.
<jtv> I did approve that one, didn't I?
<jtv> bac: so far the pressure went straight onto the brazilians
<bac> jtv: i don't know what that means.
<adiroiban> ah... yep. Danilo did approve it first... but then we decide that it requires more work and the jtv was the one to appove and land it
<bac> jtv: make lint on your branch shows a lot of stuff...mostly valid i think
<bac> assigned but unused variables
<jtv> bac: it means the Dutch have had the upper hand so far.
<bac> jtv: goals only, please.  :)
<jtv> bac: goals only?  The violence may be more interesting... they've deliberately taken down 3 dutch players in 5 minutes and they seem to know the referee will let it go.
<henninge> adiroiban: only the MP does not show that. Wonder why. But if nobody objects to you story ... ;-)
<jtv> bac: those variables were in there before...  they're not mine but they serve an explanatory role.
<adiroiban> henninge: I guess that jtv forget to update the MP
<henninge> adiroiban: yes he does forget things once in a while ... ;-)
<jtv> adiroiban: I guess...  I may have told you by IRC
<henninge> jtv: lucky sods
<jtv> henninge: they wouldn't have gotten anywhere so far without unpunished fouls...  hope the game doesn't turn bilaterally ugly
 * henninge just looked up sods ... probably not the term I meant to choose.
<jtv> bac: Brazil just scored
<henninge> awwwww
<henninge> jtv: come on! We have a date in the final!
<jtv> I hope so...
<bac> jtv: imagine a pick up match between you and stevea vs. ursula and salgado.  you know how i'd bet...
<jtv> bac: are you okay with those explanatory variables or do you want a cleanup round?
<bac> jtv: it's up to you.  i don't think they add much by way of explanation.
<jtv> bac: hell, I don't want the noise... I'll take care of it.
<jtv> bac: the match may not be worth following with the referee already having made up his mind
<jtv> open, nonfunctional violence by the brazilians now... no yellow card
<bigjools> hi bac, can you do a small branch please?
<bac> bigjools:  sure, next.
<bigjools> oh abel is free
<bigjools> I think?
<bigjools> I don't mind who does it :)
 * bigjools files MP
<bac> jtv: the comment and code don't seem to match:
<bac>  # Strip the last column off the result; it's just always True.
<bac> 146	+ return set(result)
<bac> s/146 +//
<jtv> bac: yikes!  that comment is from an older iteration... I'll take it out
<bac> thx
<bac> jtv: r=me
<jtv> bac: great, thanks!
<bac> bigjools:  r=me
<bigjools> bac: easiest soyuz branch evar, thanks )
* bac changed the topic of #launchpad-reviews to: bigOn call: adeuring, bac || reviewing: -,jelmer || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> bigjools:  that's the truth!
<jelmer_> bac_: Hi!
<jelmer_> :-)
<bac> jelmer_:  how are we supposed to find you with a new irc nick every day?  :)
<jelmer_> sorry :-)
<bac> jelmer_:  looking at your MP, i wonder about the import for Dsc
<bac> jelmer_:  on our machines where this script runs do we really find Dsc in both places?
<jelmer_> bac: Ah, I forgot to document that one.
<jelmer_> bac: Newer versions of python-debian have renamed the upstream package from debian_bundle to debian
<jelmer_> it still provides debian_bundle but will raise a DeprecationWarning
<jelmer_> that warning causes various tests to break
<bac> jelmer_:  and our machines are in flux between the two?
<bac> or is it a dev vs production issue?
<jelmer_> bac: Maverick has the newer version of python-debian, and we'll hopefully include a more up to date version in the PPA as well soon.
<bac> jelmer_:  ok, but eventually we'll all find it in one place.  how about an XXX comment for cleaning up the import when things stabilize?
<jelmer_> bac: That's a good idea.
<bac> thanks jelmer_ .  r=bac
<jelmer_> bac: Thanks!
<bac> hey bigjools it'd be good to tell your reviewer if you're looking for an RC.  it really shouldn't matter but i would look at things much harder if i knew it was an rc candidate.
<bac> otoh, i should've asked...
<bigjools> well, I don't understand how that affects the review
<bac> bigjools:  ideally it shouldn't.
<bac> they should all be super stringent
<bigjools> but I should have said - in fact I normally do
<bigjools> so sorry 'bout dat
<bac> but in reality i know i'd pay a lot more attention
<henninge> bac: goooooooooooal!
* bac changed the topic of #launchpad-reviews to: bigOn call: adeuring, bac || reviewing: -,bjornt || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<henninge> bac: gooooooooooooooal!
<henninge> NED - BRA 2:1
<henninge> jtv is probably dancing Samba ... ;-)
<jtv> henninge: good thing I'm not on Mumble
<henninge> yeah, it'd probably burst my earphones
<jtv> My God, that was a deserved red
<henninge> I say
<jtv> Stabbing your cleats right into the back of a prone man's leg...
<jtv> Damn, that red card should've gone to Caca
<henninge> jtv: that was yellow
<jtv> yes, but what Caca did deserved red.
<henninge> jtv: did you realize that we will be watching the final together in Prague?
<jtv> henninge: I'm being very careful in my use of my country's very rich vocabulary for your countrymen  ;-)
<jtv> Would be fun to watch de-nl together!
<henninge> What I expect! ;)
<henninge> They need higher goals for those kind of shots ...
<jtv> We got lucky there...
<jtv> ...and not there
<henninge> jtv: Congratulations!
<jtv> henninge: thanks!  Now you do the same and we'll meet in Prague!
<henninge> jtv: We'll meet anyway ... :-P
<jtv> :D
<jtv> henninge: Sneijder just explained why he kept shooting into the goalie's arms: he knew the guy isn't so good at catching close to his body.
<jtv> The surprising thing is I don't hear any vuvuzelas outside...  Excellent noise isolation my dad has.
<jtv> bac: Holland passes 2â1, deservedly.  Loyalty aside I was sort of rooting for Brazil until the first minutes when the fouls started.
<bac> jtv:  nice
<henninge> jtv: enjoy your weekend! I'll be going there now, too. ;-)
<jtv> henninge: thanks, you too... have a lemonade on us!
<henninge> I will ! ;-)
<bryceh> jtv, excellent, thank you!
<jtv> bryceh: whoa, took me a while to figure out what that was about.  :)
* adeuring changed the topic of #launchpad-reviews to: On call: bac || reviewing: bjornt || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<jtv> bac: apparently the Brazilian fans were good sports and many said the Dutch team was simply better in the second half.  I think the referee let a few Dutch fouls slip later on to compensate for some really bad ones he let go from the other side earlier.
<jtv> I wish we'd get instant replay soon... bad calls breed lots of resentment on both sides that they take out on each other.
* sinzui changed the topic of #launchpad-reviews to: On call: bac || reviewing: bjornt || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<sinzui> Hi bac. I have a branch for review. My apologies for the diff which I think is hard to read because of the many small changes needed to expose summary and copyright for packages.
<bac> sinzui: ok
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: sinzui || queue: [bjornt] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> sinzui: i get errors in test_highlighted_copyright_is_None
<sinzui> :(
<sinzui> I get them too
<sinzui> one moment
<sinzui> ah
<sinzui> bac: The setup is wrong. The copyright must be set to None. Our factory is better than our data, or some of our uploaders
<sinzui> bac: this is the fix for when there is no copyright file: http://pastebin.ubuntu.com/458417/
<bac> sinzui: if you go to https://launchpad.dev/ubuntu/warty/+source/a52dec you'll see a warning that there is no current release
<sinzui> yes, that is true
<bac> clicking on the +changelog gives a nice but empty page
<bac> +copyright oopses
<sinzui> bac: sample data is very very bad
<sinzui> remember that we have to use the factory to make SPPH for everything, that page is ehy
<bac> so if it were fully fleshed out the oops would not happen
<sinzui> pmount and mozilla-firefox have some data
<bac> sinzui: ok.  i just want assurance that we're being suffiently defensive
<sinzui> The page told the truth about no release, and we do not make links to impossible packages
<sinzui> however, I will make copyright not oops when someone is being bad
<sinzui> bac: this is the fix to see info that there is no copyright: http://pastebin.ubuntu.com/458422/
<bac> great
<mars> sinzui, ping, when you have a moment, could you please review https://code.edge.launchpad.net/~mars/launchpad/ga-deployment-only/+merge/29116 ?
<sinzui> mars: I do not think that will work
<mars> neither do I
<mars> '|' is not OR, is it
<sinzui> mars tales is testing None or something
<sinzui> False is something
<mars> eewww, even worse
 * sinzui thinks if there is a tales formatter for false>None
<mars> surely there is some way to just do a binary expression in TALES?
<sinzui> no
<sinzui> but you can do:
<sinzui> python: is_edge or is_lpnet
<mars> do the variables come through?
<mars> from TALES back to Python ?
<sinzui> They do in this case I believe. We scoped them to the entire template
<mars> simple, testing now
<sinzui> mars, if not you need to do python: config.launchpad.is_edge ??
<mars> sinzui, python: is_edge or is_lpnet works
<sinzui> rock
<mars> branch scanner is slow today
<mars> pushed, just waiting for the update
<mars> updated
<mars> sinzui, r=you?
<sinzui> mars: yes r=me
<mars> I wonder if 'ec2 land' understands the release-critical tag
<mars> thanks sinzui
<mwhudson> mars: it adds [release-critical=] if there's a review with that tag iirc
* bac changed the topic of #launchpad-reviews to: On call: bac || reviewing: - || queue: [bjornt] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
<bac> mwhudson: that is correct
<mars> cool
<mars> sinzui, could you please hit 'Approve' on the merge proposal?  https://code.edge.launchpad.net/~mars/launchpad/ga-deployment-only/+merge/29116
* bac 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
<mars> bac, care to give a second a review of https://code.edge.launchpad.net/~mars/launchpad/ga-deployment-only/+merge/29116 ?
<bac> sure
<bac> mars i'm confused
<bac> you reviewed your own branch?
<mars> bac, I don't know how to formally say 'Curtis reviewed it' - there is no Proxy Review button
<bac> mars: why didn't he mark the MP?
<mars> I can vote by proxy - why not review by proxy as well?
<bac> the commit message is going to be a hoot
<mars> :D
<bac> [r=mars][rc=mars][bug=mars]
<mars> bac, I don't know, he isn't answering
<bac> i would actually suggest you get an rc from curtis since he was the last RM.  i'd be very uncomfortable giving myself an rc
<mars> bac, I can do the r-c myself
<bac> you certainly can.  i think it is a bad idea.  that's just me.
<mars> you know, by being Release Manager and all
<mars> true, but that is why I asked for your second code review
<mars> from a RM perspective, this has to land to fix the lp_lucid builder
<mars> so there is no question around the RC
<mars> bac, ok, Curtis reviewed it - could you still take a look at the code, just to make sure it's sane?
<bac> mars: doing so now
<mars> thanks
<bac> mars, what about staging and dogfood?
<mars> bac, no, we do not want to track the usage of those servers.  They will just pollute the data.
<mars> Since they are test environments
<bac> mars: fair enough.  r=me
<mars> thanks bac
