[13:19] <bac> hi abentley, can you look at https://code.edge.launchpad.net/~bac/launchpad/bug-597809/+merge/28690 ?
[13:20] <abentley> bac, sorry, forgot to update the topic.
[13:20] <bac> abentley: ah, right, you were yesterday.  sorry.
[13:26] <bac> leonardr or mars:  are either of you reviewing today?
[13:27] <leonardr> bac: good question. i guess i'm doing it today--i remember being sick last tuesday
[13:27] <leonardr> bac, give me the link to your branch
[13:27] <bac> thanks leonardr.  i have a trivial branch to start your day.
[13:28] <bac> https://code.edge.launchpad.net/~bac/launchpad/bug-597809/+merge/28690
[13:31] <leonardr> bac: why should the privacy sprite not appear on private team pages? is there some other code now that takes care of that?
[13:33] <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.
[13:33] <bac> leonardr: the <div> i removed that rendered the padlock was leftover from the pre-3.0 design
[13:33] <leonardr> ok, r=me
[13:33] <bac> thanks
[14:38] <rinze> leonardr: Hi, can I add another branch to the queue?
[14:38] <leonardr> rinze, sure
[14:38] <leonardr> give me the link
[14:39] <rinze> https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/28750
[14:39] <rinze> leonardr: Sorry, I just noticed I proposed against lp:launchpad by accident
[14:40] <rinze> leonardr, I'll get back to you
[14:41] <leonardr> sure
[14:42] <rinze> leonardr: https://code.edge.launchpad.net/~jelmer/launchpad/594237-arm-checkbox/+merge/28751
[15:00] <leonardr> rinze, i must ask some basic questions
[15:00] <leonardr> what makes an architecture "restricted"
[15:01] <leonardr> ?
[15:01] <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
[15:02] <leonardr> ok
[15:02] <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.
[15:05] <leonardr> rinze:
[15:05] <leonardr> 	+ terms.append(SimpleTerm(family, token=family.name,
[15:05] <leonardr> 45	+ title=family.title))
[15:06] <leonardr> i think our coding style requires putting all the arguments to SimpleTerm on a single new line
[15:06] <rinze> ah, ok
[15:09] <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
[15:37] <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.
[15:38] <henninge> jtv, leonardr: I am taking it.
[15:39] <jtv> thanks henninge
[15:51] <leonardr> rinze: what happens if you get the enabled_restricted_families_collection_link. are IProcessorFamily objects actually published in the web service?
[15:52] <rinze> leonardr: That's a good question, I hadn't actually considered that - just changed the arm_builds_enabled variable which was exposed.
[15:53] <rinze> leonardr, it looks like it doesn't actually work because of IProcessorFamily not being available so I should probably remove the exported() bit.
[15:54] <leonardr> rinze: ok, either way you are breaking the backwards compatibility of the web service.
[15:54] <leonardr> in the unlikely event that someone wrote a web service client that checks .arm_builds_enabled
[15:54] <leonardr> we have two options
[15:55] <leonardr> you can reimplement arm_builds_enabled as a property, export in 'beta', and de-export it in 'devel'
[15:55] <leonardr> (i can show you how to do this)
[15:56] <leonardr> or, we can decide that it's so unimportant that we can simply remove it retroactively with no worries
[15:56] <leonardr> flacoste, your opinion? -^
[15:56] <rinze> leonardr: Sorry, I'm being very clumsy today
[15:56] <leonardr> rinze: np, it's a complex change
[15:56] <rinze> It looks like arm_builds_enabled wasn't actually exported, but I made a mistake in making enabled_restricted_families exported.
[15:57] <leonardr> aha
[15:57] <leonardr> ok, so simply don't export it, and there will be no backwards compatibility issues
[15:57] <leonardr> you may want to export it later, but no need to do it now
[15:57] <leonardr> flacoste, never mind
[15:57] <flacoste> leonardr: ack
[15:57] <rinze> leonardr: right - and thanks for pointing that one out
[15:59] <leonardr> rinze: r=me with changes. i'll summarize in the review
[16:00] <rinze> leonardr: thanks!
[16:04] <leonardr> rinze, review written
[16:04] <leonardr> be sure when you remove exported() to also remove the extra links from your tests
[16:08] <rinze> leonardr: will do
[16:22] <rinze> leonardr: could I add another (simpler) branch to the queue?
[16:23] <leonardr> rinze, sure
[16:23] <rinze> leonardr: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/394798-auto-buildd-secret/+merge/28759
[17:17] <rockstar> leonardr, may I hop on your queue?
[17:17] <leonardr> rockstar, sure add it
[17:19] <leonardr> rockstar, give me the mp url as well
[17:25] <rockstar> leonardr, putting together the mp right now.
[17:25] <leonardr> great
[17:26] <leonardr> rinze, what happens if you specify a build secret for a public archive? is that tested in code that you didn't touch?
[17:27] <rinze> leonardr: yeah, that's already disallowed by code I didn't touch (though I'm not sure if it's tested)
[17:28] <leonardr> ok
[17:28] <leonardr> r=me
[17:29] <leonardr> sinzui, what's the branch you want reviewed? registry-tales-2?
[17:30] <rinze> leonardr, thanks
[17:30] <sinzui> yes please
[17:42] <leonardr> sinzui, r=me
[17:42] <sinzui> thanks
[17:56] <rockstar> leonardr, https://code.edge.launchpad.net/~rockstar/launchpad/delete-recipes/+merge/28773
[18:05] <leonardr> rockstar: i was just about to ask
[18:15] <leonardr> rockstar, "if self.buildqueue_record:" should be "if self.buildqueue_record is not None:"
[18:15] <rockstar> leonardr, ah, yeah, that's probably better.
[18:16] <leonardr> rockstar: do test_destroySelf sets up a recipe build that has no associated build queue record, to test your new conditional?
[18:17] <rockstar> leonardr, yeah, it does.
[18:17] <rockstar> leonardr, I actually wrote the test to reproduce the bug first, and then fixed the code to make the test pass properly.
[18:17] <leonardr> ok, r=me with that minor change
[18:20] <rockstar> leonardr, thanks.
[19:35] <EdwinGrubbs> leonardr: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-576388-proposing-invited-team/+merge/28786
[19:54] <leonardr> edwingrubbs: sure
[19:54] <bdmurray> leonardr: I could use a review of https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/28543
[19:55] <leonardr> bdmurray: ok, you're in the queue
[20:40] <EdwinGrubbs> leonardr: I need to get some food. Do you have any questions for me about the branch before I go?
[20:41] <leonardr> edwin: no, sorry, i'm just getting started
[20:41] <leonardr> i'll put questions in irc if i have them
[20:51] <leonardr> edwin: i would like to see your test show the string 'Accepted invitation whilst trying to propose the team' somewhere
[20:52] <leonardr> also, 'while' should be just as good there as 'whilst'
[20:53] <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
[20:53] <leonardr> other than that, r=me
[21:07] <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"?
[21:08] <bdmurray> leonardr: yes, that is what bug 595124 is about
[21:08] <_mup_> Bug #595124: A bug's can_expire attribute is confusing <dhrb> <easy> <Launchpad Bugs:Triaged> <Ubuntu:Invalid> <https://launchpad.net/bugs/595124>
[21:11] <leonardr> findExpirableTasks(days_old=60) finds bugs that have not had any activity in 60 days?
[21:11] <leonardr> bdmurray -^
[21:13] <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?
[21:14] <bdmurray> leonardr: isExpirable(days_old=0) is what can_expire used to be
[21:14] <bdmurray> so might expire eventually
[21:15] <leonardr> the thing i don't understand is where days_old=0 fits into that
[21:15] <bdmurray> It is for showing on a bug's web page that it will expire in XYZ days
[21:16] <bdmurray> for example bug 301020
[21:16] <_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>
[21:17] <leonardr> ok, it says it will expire in 59 days
[21:17] <leonardr> so isExpirable(0) must have returned true
[21:17] <leonardr> but i don't understand the meaning of the zero in there
[21:17] <leonardr> presumably isExpirable(61) would have returned false?
[21:18] <bdmurray> well 60 which is the configuration value yes
[21:18] <leonardr> what is the difference between isExpirable(0) and isExpirable(60)?
[21:19] <bdmurray>                     AND Bug.date_last_updated < CURRENT_TIMESTAMP
[21:19] <bdmurray>                         AT TIME ZONE 'UTC' - interval '%s days'
[21:19] <bdmurray> it is passed to that ^ in findExpirableBugTasks
[21:20] <leonardr> so isExpirable(0) gets a bug no matter when it was updated -- will this bug eventually expire
[21:20] <bdmurray> yes
[21:20] <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
[21:21] <bdmurray> correct
[21:21] <leonardr> ok
[21:22] <leonardr> in the test, you set a bug to invalid because an invalid bug won't expire?
[21:22] <bdmurray> right otherwise I would have changed all the other tests
[21:23] <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?
[21:24] <bdmurray> switching from "expire sometime" to "expirable" the 31 is the days old
[21:24] <leonardr> ok
[21:28] <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?
[21:29] <bdmurray> because the comment would set date_last_updated to today which is less than 60 days ago
[21:30] <leonardr> ok
[21:30] <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?
[21:31] <bdmurray> they aren't equivalent because can_expire uses 60 days now
[21:36] <leonardr> ok, in that case you're changing the behavior of the web service in a way that's not backwards compatible
[21:36] <leonardr> since some bugs that used to have can_expire=True will now have False
[21:37] <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?
[21:37] <bdmurray> okay, is there a policy about that?
[21:39] <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
[21:40] <leonardr> and actually, this case might be one i didn't plan for
[21:40] <leonardr> actually, nm, i have it
[21:40] <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
[21:42] <leonardr> basically you will define two can_expire properties
[21:42] <leonardr> can_expire and can_expire_beta
[21:42] <lifeless> hi
[21:42] <lifeless> whats the protocol to get something reviewed - put your hand up and wait ?
[21:42] <leonardr> can_expire will be the one you have now, but you will only publish it in version 'devel' of the web service
[21:43] <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
[21:43] <lifeless> ok
[21:43] <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
[21:44] <lifeless> I'll start with a 'is there any one here up for reviewing two small refactoring branches'
[21:44] <lifeless> :)
[21:45] <leonardr> bdmurray: for an example, take a look at lazr.restful/src/lazr/restful/example/multiversion/resources.py
[21:46] <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
[21:46] <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
[21:48] <leonardr> i will put this information into the review
[21:48] <bdmurray> leonardr: great, thanks
[21:50] <lifeless> gary_poster: are you EOD yet?
[21:50] <gary_poster> on call
[21:50] <lifeless> do you mean 'on a phone call' ? [conflation with 'on call' in the topic]
[21:53] <leonardr> lifeless, he's probably on the phone
[21:53] <leonardr> flacoste, i'd like your opinion on bdmurray's branch https://code.edge.launchpad.net/~brian-murray/launchpad/595124/+merge/28543
[21:54] <flacoste> https://lpstats.canonical.com/graphs/LPProjectBugTriage/
[21:56] <lifeless> trending down
[21:57] <lifeless> sinzui: perhaps you are around and not EOD ?
[21:58] <sinzui> I am about
[21:58] <lifeless> I wrote a couple of patches in the weekend
[21:58] <lifeless> prompted by our discussion about the milestone page performance
[21:58] <lifeless> I was following my nose into the code
[21:58] <lifeless> they seem like improvements to me, and I'd like to get them reviewed.
[21:59] <lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/lsprof/+merge/28583
[21:59] <lifeless> and
[21:59] <lifeless> https://code.edge.launchpad.net/~lifeless/launchpad/lognamer/+merge/28574
[21:59] <lifeless> if you have the time, some feedback would be great
[22:00] <sinzui> I will review them now
[22:00] <lifeless> thank you!
[22:13] <gary_poster> lifeless: thanks for changes.  I need to run, but will look more closely later.
[22:13] <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.
[22:13] <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...
[22:14] <lifeless> gary_poster: thank you
[22:15] <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.
[22:16] <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. ;-)
[22:16] <lifeless> gary_poster: :)
[22:17] <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
[22:18] <gary_poster> no, understood.  I would have gone along with the XXX if you really wanted too (but with a bug)
[22:18] <gary_poster> ok
[22:19] <gary_poster> so, you need zcml:condition
[22:19] <lifeless> gary_poster: bugs that folk don't have time to work on are a bit close to busywork ;)
[22:19] <gary_poster> agreed :-)
[22:19] <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...
[22:20] <gary_poster> trying to remember how to do that
[22:20] <lifeless> gary_poster: don't worry about it
[22:20] <gary_poster> ./zopeappsession.zcml:106
[22:20] <gary_poster> OK
[22:20] <gary_poster> anyway
[22:20] <lifeless> gary_poster: its currently on in prod
[22:20] <gary_poster> heh
[22:20] <lifeless> removing it is orthogonal to making it event driven
[22:20] <gary_poster> ok
[22:20] <gary_poster> true
[22:20] <gary_poster> ok, running away
[22:20] <gary_poster> bye
[22:20] <lifeless> I am likely to keep pulling on this thread to get actual kcachegrind profiles
[22:20] <lifeless> ciao
[22:20] <gary_poster> yay!
[23:09] <lifeless> sinzui: thanks for the reviews, I has replied.