[02:16] https://code.edge.launchpad.net/~thumper/launchpad/lp-code-errors/+merge/15724 [04:06] and now https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-attempt2/+merge/15728 [07:37] henninge: hi. is the submission now open? === 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 [07:39] adiroiban: yes, channel topic on #lp-dev says "PQM is open for srs bsns" ;-) [07:39] Good morning adiroiban :) [07:40] srs bsns ? Serious Business ? [07:40] correct [07:41] does it mean any kind of MP ? [07:41] our losas, aren't they just a funny bunch ? ;-) [07:42] adiroiban: yes, the other states are "closed", which was last week, and "testfix" which may happen anytime when the test suite breaks on the buildbot. [07:43] henninge: I see. That make sense now :) [07:46] henninge: will you "land" my branch , or is there something I need to do? [07:49] adiroiban: hang on, let me check on the situation [07:54] adiroiban: please merge the current devel into your branch and push it again. [07:56] adiroiban: like this: http://paste.ubuntu.com/336365/ [07:57] henninge: much appreciated :) [07:57] watch for merge conflicts [08:05] henninge: and I should create a new MP ? [08:05] adiroiban: no [08:06] adiroiban: if you didn't have any serious merge conflicts, we can just land it. [08:06] adiroiban: what was the branch again? [08:06] https://code.edge.launchpad.net/~adiroiban/launchpad/bug-97293/+merge/15391 [08:06] there were only 2 lines of code :) [08:07] from bzr merge -> All changes applied successfully. [08:09] cool [08:09] branch was pushed [08:09] adiroiban: a, I remember [08:10] adiroiban: yup got. Ok, nothing else for you to do now, I will land it. [08:11] henninge: thanks. An for the other one I should do the same merge and ping the person who told me that will land the branch? [08:11] adiroiban: just ping me [08:11] after the pushing [08:11] ok [08:15] henninge: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-193750 merged... [08:15] should I give you the full link to the branch or to the MP, or just the MP number? [08:17] adiroiban: all good, I got it. [08:17] ;) [08:18] henninge: :) ... another one https://code.edge.launchpad.net/~adiroiban/launchpad/bug-487970/+merge/15250 [08:26] henninge: last one: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-135008/+merge/15347 [08:27] adeuring: wow, you have been busy! Great job! [08:30] adiroiban: ah, one more thing you can do: Add a commit message to each mp! [08:31] that way I don't have to make them up ... ;) [08:31] henninge: hm... how? [08:31] :) [08:31] adiroiban: simply click on "Set commit message" :) [08:31] It's a new feature! [08:32] adiroiban: It's a green line starting with a (+) icon. [08:32] Ah... it was just my nose covering that link :) [08:32] lol [08:33] got it [08:33] and what should I write there? [08:34] it's just a bzr commit message to use when your branch is merged and commit into devel. [08:35] so you can probably just copy something you used for your local commits. [08:35] yes. thanks [08:35] "copying plural values will also enable the required fields" [08:35] adiroiban: But make it proper sentences, please. ;) [08:35] "Copying plural values will also enable the required fields." [08:46] henninge: done for all 4 MPs [08:46] adiroiban: cool, thanks. You are, done. I'll keep you posted on the proress during the day. [08:48] henninge: thanks. I will also need a pre-MP review for https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249 [08:49] should I ask it on lp-reviews or via the bug tracker? [08:51] adiroiban: no, using the bug tracker for code reviews is not what we do. [08:51] adiroiban: asking here or directly one of the lp developers is what you should do. [08:52] henninge: here or on lp-dev ? [08:52] adiroiban: Incidently I am an on-call reviewer (OCR) today. [08:52] ;) [08:52] adiroiban: "here" is #lp-reviews [08:53] but I am also Help contact and currently busy landing some great contributions submitted by a cool community member ... ;-) [08:54] OCRs are also doing pre-MPs reviews ? [08:54] sorry for all these questions... but I'm lost in this process [09:24] adiroiban: first one's in! http://bazaar.launchpad.net/~launchpad-pqm/launchpad/devel/revision/9977 [09:24] adiroiban: the others will take longer, though. I did not run this through the test suite since it does not yet run javascript tests. [09:25] I just ran the tests locally and submitted directly. [09:25] henninge: ok. np. thanks [09:30] adiroiban: was this your first contribution to Launchpad? [09:32] henninge: yep. 2 lines ... like 0.02$ :) [09:33] adiroiban: Well, Congratulations! [09:33] ;-) [10:16] adiroiban, henninge: hi :) [10:16] danilos: hi :D [10:16] hi danilos! [10:17] danilos: I would need a quick review for this branch so see if I should push it to a MP https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249 [10:18] adiroiban, I've seen you looked into a gazillion of bugs :) I'll be going through them and commenting; in general, I suggest you talk about the bugs beforehand (i.e. before starting on them), just to make sure you are not wasting your time if we disagree what the solution should be [10:18] adiroiban, sure, looking at bug 431249 [10:18] Bug #431249: Put navigation links in a seperate template/macro. [10:20] danilos: i agree... but the goal of all those bugs was to get into the code. Even if that work will not be used, I still learnt how to hack LP [10:20] adiroiban, sure :) [10:20] adiroiban, I am not saying it's not great, just giving an advance notice :) it's possible they are all doing exactly as desired :) [10:21] danilos: so you can consider this is the pre implementation discussion. I was aware of this risk [10:21] there is no problem if that code will not be used... as you can see, I start working on some bugs that were not even confirmed [10:22] adiroiban, right, bug 431249 fix looks reasonable; I'd still keep the page ID something like "translation-macros", even though it's registered only on TranslationsLayer, because this might hide a generic +macros if it exists [10:22] Bug #431249: Put navigation links in a seperate template/macro. [10:22] (or if it's created in the future) [10:22] adiroiban, otherwise, it looks like a good approach, so, provided there are sufficient tests, go for it :) [10:22] danilos: ok. I can change that [10:23] danilos: should I add more test ? [10:23] adiroiban, not necessarily, only add a test if there's not any one testing the generic presence of these links on both pages - do not go through all the conditional tests etc. [10:23] those changes should not touch any „visible” part [10:24] aha. ok [10:24] adiroiban, right, so, it's two options: find existing test or write one :) former is usually simpler :) [10:24] adiroiban, btw, did you get help from Henning about landing your already approved branches? [10:25] danilos: yep. everhing is ok. [10:26] danilos: the other feedback I'm asking is about this one: https://code.edge.launchpad.net/~adiroiban/launchpad/bug-406477 [10:29] adiroiban, looking [10:32] adiroiban, in general, it's fine; however, I'd rather structure those permissions differently, and make use of launchpad.TranslationsAdmin permission for this role instead of using launchpad.Admin (though, note that you'd have to change .zcml in that case as well, perhaps even some templates something/required:launchpad.Admin conditions) [10:33] adiroiban, but, if you want to keep it simple, just go with what you've got (what I am suggesting is simply because the current state of privileges/permissions in LP is a bit of a mess, and I'd like to clean it up :) [10:34] danilos: I can look into that. This was my first look at LP privilege system, and I just wanted to make sure I got it right. [10:37] danilos: thanks. This should could keep me bussy for some time :) [10:39] adiroiban, heh, nah, thank you :) === matsubara-afk is now known as matsubara === mrevell is now known as mrevell-lunch [11:55] henninge, trivial one for you: http://paste.ubuntu.com/336479/ [11:56] salgado: what is that file for? [11:56] * henninge is reading the file ... ;) [11:56] yeah, the heading there says it all [11:57] salgado: so, I wonder why this was able to land, then? [11:57] salgado: r=me, of course ;) [11:57] henninge, because PQM doesn't run the testsuite? [11:58] salgado: ;) I see it now ... [12:00] salgado: did you see my r=me ? [12:00] henninge, yep, thanks! [12:02] adeuring: Hi! [12:02] hi henninge [12:02] adeuring: Did you land a branch for Adi this morning? [12:03] henninge: yes [12:03] lp:bug-192750 [12:03] adeuring: ok, I have the same branch in ec2 for him. I think he wasn't aware that you were going to do that, too. [12:04] henninge: then you can kill the ec2 instance, I think [12:04] adeuring: oh yeah, np with that [12:04] I was just wondering what happened [12:06] adeuring: it's bug 19-3-750, though. [12:06] Bug #19: "Display settings" forgotten when "Save" is pressed [12:06] mup: bug 193750 [12:06] Bug #193750: remove references to +addticket [12:06] henninge: yes [12:27] henninge, I have yet another trivial one for you: http://paste.ubuntu.com/336490/ [12:27] this one reverts a couple changes I committed to mainline by accident [12:29] salgado: I guess it's too early in the morning for you! ;) [12:29] s/is/was/ [12:30] yeah, today didn't start very well [12:32] salgado: shouldn't this patch be the other way round? [12:32] I just compared it to the patch you want to revert. [12:32] henninge, yes, the other way around is what I'm committing, sorry [12:33] I applied it with -R, but gave you the original diff, where the change was introduced [12:33] http://paste.ubuntu.com/336495/ is the correct one [12:33] salgado: cool, r=me === henninge is now known as henninge-lunch [12:34] thanks henninge-lunch === henninge-lunch is now known as henninge === mrevell-lunch is now known as mrevell [13:14] Hi mrevell ! [13:14] hi henninge [13:14] mrevell: Do you know what help@lp.net points to? [13:15] henninge, yes, it points to feedback@launchpad.net [13:16] mrevell: sure? I got an email this morning addressed to me an to help but I only saw one copy. [13:17] I am subscribed to feedback. [13:17] henninge, let me checl [13:17] s/checl/check [13:19] mrevell: nm, I checked, too. The other one'sin the bin ... [13:19] henninge, I don't see anything in the queue [13:19] sorry [13:20] argh! [13:20] no, that's not it === sinzui changed the topic of #launchpad-reviews to: on-call: (henninge, also on CHR) || reviewing: - || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === abentley changed the topic of #launchpad-reviews to: on-call: (henninge, also on CHR), abentley || reviewing: - || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:23] abentley: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-433809-picker-search-slash/+merge/15674 [15:23] EdwinGrubbs: Okay === abentley changed the topic of #launchpad-reviews to: on-call: (henninge, also on CHR), abentley || reviewing: -, EdwinGrubbs || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:24] henninge or abentley: could either of you review an incremental to an MP which was approved last cycle (but not landed?). See my last comment at: [15:24] https://code.edge.launchpad.net/~michael.nelson/launchpad/443353-api-builds-from-private/+merge/14896 === henninge changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: EdwinGrubbs || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:24] noodles775, abentley: Sorry, got my hands full on CHR. [15:24] np, it's not urgent. [15:25] noodles775: I'll queue it. [15:25] Thanks abentley === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: EdwinGrubbs || queue [sinzui, sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:26] EdwinGrubbs: In the future, you can set db-devel as the prerequisite branch and still propose merging into devel. [15:28] oh, abentley, on that note, I've got a pipeline of MPs that were approved, but are all proposed for db-devel (as they were dependent on schema changes). I'm keen to land them now in devel. What would you recommend as the best way? [15:29] (I'm about to just aggregate all the changes into a new devel branch, propose it as an MP and link to the previous approvals before ec2 land'ing it). [15:29] noodles775: Personally, I would land the last pipe on devel using pqm-submit. [15:30] noodles775: I'm not sure whether ec2 land will allow you to override the target. I would assume not. [15:30] abentley: ok, I'll try that. Thanks! [15:30] (once I've QA'd it on dogfood etc.) [15:31] noodles775: No problem. (this implies you'll need to run the tests if you haven't already) [15:31] abentley: yes, I'll re-merge db-devel, run the tests, QA on dogfood, and if all goes well land that branch on devel. [15:32] noodles775: I would recommend merging devel rather than db-devel. === matsubara is now known as matsubara-lunch === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: noodles775 || queue [sinzui, sinzui, EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [15:56] noodles775: I'm concerned because this diff includes a lot of changes that don't seem to be in scope, e.g. YUI -> LPS. [15:58] abentley: I'm not sure why you say not in scope? The core change of the branch was to getBuildsForSources() - which affected the UI as outlined in the incremental? [15:59] noodles775: Your incremental diff doesn't show the YUI -> LPS change. [15:59] abentley: LPS? [16:00] noodles775: Look that the preview diff. It has a bunch of YUI -> LPS changes in it. [16:02] abentley: ah, you mean the generated diff - I'm assuming that's because I've just merged devel and pushed as you suggested (but the MP is for db-devel). [16:03] The actual diff for this change is in the comment. === noodles775 is now known as noodles775-otp [16:05] noodles775-otp: I see. I didn't realize you'd be asking for review on any of those. [16:06] abentley: just the incremental. But yes, after I did that, I thought a better option would have been to add a new pipe and do it there. [16:07] noodles775-otp: So to be clear: you actually want to land this on devel, not db-devel. [16:08] abentley: correct. I had to develop it using db-devel last cycle, but want to land it in devel now. [16:08] Is there a way to re-target the MP at devel? [16:09] noodles775-otp: No, that would be a different proposal, which makes sense because there are different criteria for landing on devel. [16:10] abentley: OK (although I can see there would be cases where it matters, but in this case it's really just the diff being reviewed). I just thought I had memories of using hitchhiker to do it once before. [16:11] noodles775-otp: No, hitchhiker only controls branches. It doesn't provide a way to manipulate merge proposals. [16:12] OK. Let me know if I should revert r9893 on that branch then. [16:12] noodles775-otp: Don't worry about it, I'm regenerating the preview diff locally. [16:13] ok, thanks. [16:25] noodles775-otp: Are both of the finds necessary, or would the second find (for copied binaries) work even if SourcePackagePublishingHistory.archiveID == Build.archiveID ? [16:35] abentley: hrm, I thought so when I wrote that a few weeks ago, but looking at the two finds now, I'm struggling to see why. === noodles775-otp is now known as noodles775 [16:36] BTW: If you're willing to re-review the entire MP, I could create a new MP against devel. [16:36] noodles775: Okay, I don't think it's a blocking issue, but if we can simplify by removing the first find, that would be great. So r=me, do what you will. [16:37] abentley: OK, I'll try it and see. Thanks! === matsubara-lunch is now known as matsubara === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: sinzui || queue [sinzui, EdwinGrubbs] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [16:41] EdwinGrubbs: In case you didn't see earlier, I've bumped your review to the queue because you weren't around to answer my questions. [16:42] abentley: I was only gone for a few minutes and then you were offline so I couldn't respond. [16:42] EdwinGrubbs: Sorry about that, my IRC client doesn't make that very obvious. === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: EdwinGrubbs || queue [sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [16:43] EdwinGrubbs: Okay, so is the conditional needed? [16:44] abentley: do you mean this conditional: if '/' in query: [16:45] EdwinGrubbs: No, I mean in patch line 100, "if result is not None". [16:46] EdwinGrubbs: It looks like the code should actually read "result = IStore(...).find(...).one(), so that you can raise lookuperror if there were no results. [16:47] abentley: well, I assume the original purpose of that is to provide the LookupError exception as opposed to something more generic. A try/except clause could also be used, but that would be less efficient. [16:47] EdwinGrubbs: I thought find could never return None. Am I wrong? [16:49] abentley: oh, I see what you are saying. I didn't think about the differences in the return values between sqlobject and storm. I'll test that I'm testing the right return value for an unsuccesful find(). [16:51] EdwinGrubbs: Great. I'll leave the review alone for now. Please ping me when you've examined that. [16:51] ok === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: sinzui || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:09] abentley, I have a one-liner for a testfix branch; can I skip the queue? http://paste.ubuntu.com/336670/ [17:10] salgado: I don't know what this LPS thing is. [17:10] abentley, I replaced all YUI() calls with LPS in our templates [17:11] LPS is just a YUI instance, stored globally to be used in templates [17:11] salgado: very well, r=me. [17:12] thanks abentley === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === abentley changed the topic of #launchpad-reviews to: on-call: abentley (on lunch) || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [17:33] abentley: your understanding of translatables not being batched is correct. [17:34] salgado: I got an email from Buildbot, but I can not access this link https://lpbuildbot.canonical.com/builders/lp/builds/405 [17:35] can you please copy the part describing the build problem? [17:35] adiroiban, no need to worry about it; you got that because my changes caused a test to fail [17:35] I've already fixed the test and submitted it to pqm [17:36] salgado: ok. thanks. I was not sure what to do with it :) [17:44] salgado, hey, got a time to take a peek at https://code.edge.launchpad.net/~danilo/launchpad/bug-493629/+merge/15758 (it's a one line change which hopefully fixes the JS buildbot for good, patch by mars actually) - I know you are not OCR, but just so I don't have to wait for abentley to come back from lunch and I can go for dinner :) [17:46] * salgado checks the diff [17:47] danilos, r=me [17:48] salgado, thanks === henninge_ is now known as henninge === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [19:40] abentley: hi, Can you please review https://code.edge.launchpad.net/~adiroiban/launchpad/bug-431249/+merge/15767 ? [19:40] adiroiban: Okay, I'll have a look in a minute === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: - || queue [adiroban] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [19:41] abentley: thanks. no hurry :) === abentley changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: adiroban || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [20:00] adiroiban: Has this branch been through a complete test suite run? [20:01] abentley: nope. Only the translation tests. [20:01] on my computer [20:01] adiroiban: It's approved, so let me know when it's been through a complete test suite run, and I'll land it. [20:02] abentley: should I run the full test on my computer? [20:02] adiroiban: Yes, that's fine. [20:03] on the previous one Henning launched the test on some supercomputer [20:03] running all tests on my computer would take many hours... [20:04] adiroiban: I run all the tests on my computer. [20:09] abentley: thanks. [20:10] adiroiban: There's no supercomputer, he would have just used Amazon's Elastic Compute Cloud. [20:10] can I run the complte test on a remote computer with no X libs? I have a shell account on a remote computer I can configure it for testing my branches [20:10] but I'm not sure about testing Javascript [20:11] adiroiban: Yes, I don't think the test suite uses x libs. The javascript testing isn't integrated into test runs anyhow. [20:12] abentley: ok. thanks. Then I'll start configuring that machine. [20:19] abentley: I fixed the handling of the return value from find() and I pasted the incremental diff into https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-433809-picker-search-slash/+merge/15674 [20:21] EdwinGrubbs: Cool. On the phone. === sinzui changed the topic of #launchpad-reviews to: on-call: abentley || reviewing: adiroban || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews [20:33] rockstar: shall I send a few reviews your way? [20:33] thumper, sure. Are they urgent? [20:34] thumper: isn't everything? [20:34] * thumper facepalms [20:34] rockstar: I'll fire the ui one your way [20:35] rockstar: I just have others that you may be interested in === matsubara is now known as matsubara-afk [20:39] thumper, okay, fire them my way. [20:40] rockstar: this is the ui one https://code.edge.launchpad.net/~thumper/launchpad/hiding-review-fields/+merge/15722 [20:40] rockstar: that is just a "get the branch and look at it" type review [20:41] rockstar: and others around the claim/delete review arena: [20:41] https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-browser-integration/+merge/15719 [20:41] https://code.edge.launchpad.net/~thumper/launchpad/claim-review-into-model-attempt2/+merge/15728 [20:41] https://code.edge.launchpad.net/~thumper/launchpad/delete-pending-reviews/+merge/15739 === abentley changed the topic of #launchpad-reviews to: on-call: || reviewing: - || queue [sinzui] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews === salgado is now known as salgado-afk === EdwinGrubbs is now known as Edwin-afk [23:20] sinzui: still around?