[01:53] anyone around? [02:05] * thumper hides [03:36] thumper: can I tempt you for a review? :-)? [03:37] NCommander: not right now, I'm just about to go make dinner and come back later this evening [03:37] thumper: fair enough [03:40] NCommander: i might be able to have a look at it [03:41] mwhudson: cool! [03:41] mwhudson: https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog_librarian - it probably needs another ec2test run but I fixed all the failures that were in the email [03:51] NCommander: did you know that there's a theoretical limit of 800 lines of diff for a review? [03:52] oh i see, a lot of the diff is sampledata changes [03:52] mwhudson: no I didn't, although the wiki says to ignore the sampledata change size :-/ [04:13] NCommander: it's getting a bit late for me, but does your branch store the changelog in the database or in the librarian? [04:13] mwhudson: librarian. See dscfile.py changes [04:14] mwhudson: findAndMoveCopyright and the self.librarian.create call [04:14] NCommander: then the sampledata changes look strange [04:14] as they seem to include big chunks of changelog [04:15] NCommander: do you need to regenerate the sampledata again? [04:16] mwhudson: I see a whole lot of NULLs for the pointer to LFA. There is some changelog data stored in the database by the existing code however [04:16] mwhudson: thats going to get depericated and eventually removed with a little luck [04:17] oh ok [04:17] ahh, happiness is meld on a bigger monitor [04:17] mwhudson: that's the changelog_entry your probably seeing, and there are quite a few issues with its design sadly, raw source changelog will hopefully let us replace it -) [04:17] meld? [04:17] NCommander: a diff tool [04:19] mwhudson: I'm sorry to keep you awake :-/ [04:19] aagh soyuz code it burns the eyes [04:19] NCommander: it' [04:20] s only late to be concentrating, not to be awake :) [04:20] mwhudson: heh, I plan to be writing more soyuz code in the future, so make sure the brian bleach is near by [04:20] createUploadedSourcePackageRelease takes far too many arguments [04:21] mwhudson: no kidding :-/ [04:23] NCommander: ah right, i've finally found the interesting parts of your change [04:46] mwhudson: I'll be AFK for about 15 minutes [04:47] NCommander: it'll probably be that long before you have a review, but thanks for letting me know [05:03] mwhudson: back [05:03] NCommander: still going! [05:07] NCommander: review submitted [05:12] mwhudson: thanks. Fixing. [05:13] mwhudson: maybe yo ucan explain when a module should be in __all__ [05:13] NCommander: well, maybe that's overly picky of me [05:13] NCommander: one way around is that it _needs_ to be in all if it's imported in another module [05:14] NCommander: i guess the question is, is findFile part of the 'public interface' of the module [05:14] it's a close call in this case [05:15] mwhudson: I think its used in the test suite [05:16] mwhudson: ugh, a few of these exist just before I didn't properly clean up with all the refactoring this branch has gone through [05:16] * NCommander headthunks [05:16] NCommander: that's what reviews are for :-) [05:17] mwhudson: with 10MiB changelog, that was a security fix that was discovered when I was writing the code [05:18] mwhudson: bigjools and I threw some numbers, and we decided 10MiB was a good value. The old value was 2GiB (hardset DB limit) [05:18] NCommander: ok [05:18] NCommander: i remember some discussion [05:18] but i remembered it being about copyright, not copyright and changelog [05:19] if i remembered wrong, that's fine :) [05:19] mwhudson: well, that's from an earlier version of the branch where changelog and copyright were in the database [05:19] changelog will now be in librarian instead of the DB, so I could actually drop that check [05:20] mwhudson: or, well, move it to debian/copyright's finder function [05:21] NCommander: in this case i'm just asking the question [05:21] making sure you've thought it through [05:21] i don't have the answer :) [05:24] mwhudson: heh :-) [05:26] mwhudson: you can't set changelog_path with dsc_file's path since its a temporary directory :-/ [05:26] * NCommander was greatly annoyed by having to make that interface change [05:27] NCommander: i mean you could say dsc_file.changelog_path = foo then call findAndMoveChangelog(dsc_file) [05:28] mwhudson: ah, I could do that, although then I need to whack the testing code a bit [05:29] mwhudson: (I sorta like to keep my functions self contained, but maybe just because I'm weird ...) [05:30] NCommander: maybe leave it [05:34] NCommander: i'm off to sort out dinner, i'll be around ish for another 30 mins maybe but perhaps you'll need to get someone else to review the branch again [05:41] mwhudson: I'm almost done fixing most of the issues [05:53] mwhudson: pushed [05:53] (and resubmitted) [05:53] NCommander: you don't need to resubmit [05:55] mwhudson: oh ... [05:55] * NCommander already did :-/ [05:55] well you'll know for next time :) [05:57] NCommander: sorry have to go now, i'll review it first thing tomorrow if noone else gets to it [05:58] mwhudson: fair enough [05:59] NCommander: If you need to make changes to a branch, just push it, the MP is clever enough to realise the branch changed [05:59] StevenK: I hit the resubmit button so mwhudson Needs Fixing goes back to Review [06:00] NCommander: You can edit the MP state too ... [06:03] StevenK: ah [06:06] NCommander: I can edit my own MPs and set them betwen WIP, Needs Review and Merged. I'm unsure why the last one is allowed, but the first two make perfect sense. [06:07] StevenK: are you in ~launchpad? [06:07] StevenK: I can see Merged, but I always thought that's because I'm in ~launchpad [06:12] I don't think so. [06:13] Confirmed, I'm not a member of ~launchpad [06:45] StevenK: What harm is there in letting people set their own MPs as Merged? [06:45] It's useful if it wasn't automatically detected. [06:46] Well, what happens if an MP is set to Merged before it's even reviewed? [06:46] It drops off the review list. [06:47] IE, it could just drop through the cracks if the submitter just sets it to Merged? [06:47] Yes. [06:47] But if they do that, then they're pretty silly. [06:47] It's just like if they set it back to Work in Progress [06:48] I've done that, which is useful [06:48] "I thought I was ready, oh damn, I missed x, y and z." [07:36] StevenK: file a bug? [09:22] stub, BjornT: Could you take a look at https://code.edge.launchpad.net/~gmb/launchpad/bugwatch-next_check-bug-544238 please? Really need to get this one landed as soon as possible. [09:26] nag nag nag... :) [09:31] :) [09:31] stub: Thanks for the review :) [09:34] stub: BTW, mind looking at the revised raw source changelog (which now uses librarian when you get a chance?) [09:34] morning bigjools [09:39] jml: Can I have your vote on https://code.edge.launchpad.net/~gmb/launchpad/bugwatch-next_check-bug-544238 please? [09:39] gmb: sure, i'll look at it, if you explain how it will be used in more detail :) [09:40] BjornT: Cool. So, we want to be a bit more sensible with scheduling bug watch checks. At the moment we check each watch once every 24 hours, but if it's erroring a lot (which we now record in the BugWatchActivity table) we don't want to check it so often. [09:41] BjornT: So, instead of building that logic into BugTracker.getBugWatchesNeedingUpdate(), which would be complex at best, we think we should have a garbo-hourly process that looks at all the bug watches checked in the last hour, checks to see how often they've errored and then schedules their next_check accordingly. [09:41] So BugTracker.getBugWatchesNeedingUpdate() will look at the next_check field instead of lastchecked to determine whether a watch needs checking. [09:42] The advantage is that we don't have to further complicate checkwatches to get the back-off for erroring watches. === stub1 is now known as stub [10:00] gmb: ok, that makes sense. i'm wondering if we could find a better name? if you set next_check to some time, it doesn't mean that it will be checked exactly at that time, will it? [10:01] BjornT: Hmm, true. How about check_after? [10:02] check_due ? [10:02] * stub gets out his bike shed schematics [10:03] It should be blue. [10:05] gmb: maybe next_check is good, as long as there is a comment explaining the semantics. can't think of a better name, and i think next_check is better than the other suggestions. [10:05] BjornT: Right; I'll add the comment (should've added it anyway). [10:16] BjornT: you around? [10:21] NCommander: kind of. i'm in desperate need of lunch, though :) [10:22] BjornT: ah, no rush :-) [10:32] BjornT: if after lunch you'd be willing to look at my raw_source_changelog_librarian branch, I'd be most grateful [10:32] stub: thanks for the review [10:36] NCommander: sure. i have a bit of a backlog returning from vacation, but i'll try to get at it today [10:36] BjornT: thanks [10:36] BjornT: (its a new merge proposal versus the old one) === stub1 is now known as stub [10:44] intellectronica: you don't think it's worth closing the choice edit before bringing up the confirmation? [10:45] (for https://code.edge.launchpad.net/~intellectronica/launchpad/bug-531963/+merge/21628) [10:45] noodles775: i do. thanks for your patch, that's exactly what i was missing :) [10:46] i didn't consider using css to get rid of it [10:46] intellectronica: ah, I just didn't see it in the MP diff. Great. [10:46] gmb, I'm confused [10:47] gmb, do you want me to do a code review or a db review? [10:47] jml: Sorry, DB; typo when I requested the review. [10:48] gmb, ta [10:58] anyone around to do a code review? === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [NCommander,jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [11:48] hi NCommander [11:48] morning bac [11:48] bac: https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog_librarian/+merge/21909 [11:48] NCommander: morning. i'll take a look. [11:48] bac: thanks:-) === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: ncommander|| queue: [jpds] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:01] NCommander: i see that branch has already been reviewed by michael. generally, for consistency, we have the original reviewer look at changes. [12:04] bac: his comment he made earlier tonight was to have someone else look at it, else he'd look at it tonight [12:04] bac: 01:57:31 < mwhudson> NCommander: sorry have to go now, i'll review it first thing tomorrow if noone else gets to it [12:04] NCommander: ah, ok [12:05] bac: sorry for the confusion [12:05] NCommander: and is the branch really 2600 lines? [12:05] bac: no, its about ~100-200, but I regenerated sample data [12:05] ah, ok. thanks for that [12:05] * bac looks [12:05] * NCommander hands bac the brain-bleach === mrevell is now known as mrevell-lunch [12:58] hi NCommander -- i've updated your MP. just a few fixes and then it'll be ready to land. thanks a ton for the branch! === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: jpds|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [12:59] bac: I still need a BjornT +1 before it can land :-/ [12:59] NCommander: right [13:00] bac: ugh, I missed afew changes. I shouldn't resubmit on lack of sleep >.<; [13:00] NCommander: np [13:01] jpds: i don't see your MP on https://code.edge.launchpad.net/launchpad/+activereviews [13:05] bac: I think bigjools is dealing with it. [13:05] yes === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [13:09] jpds: ok, great. please remove your name from the topic if you line up a reviewer. [13:13] bac: I got everything fixed except one lint error that wasn't in code I modified. Its too long by one character, but I don't see a good way to break this up to multiple lines [13:13] DistroSeries.id == POTemplate.distroseriesID).config(distinct=True) [13:14] bac: suggestions welcome [13:14] NCommander: what file? [13:15] bac: lib/lp/registry/model/distroseries.py [13:15] bac: sorry :-/ [13:18] NCommander: you could drop the .config() and put it on the next line as a separate statement [13:18] kind of cheesy. [13:20] bac: er, I can? [13:20] * NCommander isn't sure he's reading this right them, or why there is a == on that line === mrevell-lunch is now known as mrevell [13:22] bac: oh, I can. bugh, brain without coffee isn't good [13:34] henninge: hi, do you have time to look at https://code.edge.launchpad.net/~adiroiban/launchpad/bug-201749/+merge/21250 ? [13:35] adiroiban: maybe later, I am at a sprint atm. [13:35] henninge: ok. no problem! [13:39] bac: so, I fixed everything, except utilities/paste doesn't work for me (it wants a python module which I'mlooking for now) [13:40] NCommander: you can do it by hand. was just a suggestion [13:40] bac: ah, ok [13:40] NCommander: but i'd be interested to hear why it doesn't work. you do have launchpad-dev-dependencies installed? [13:41] bac: I do, python-clientcookie wasn't installed [14:01] bac: changes made, incremental diff posted. I think I addressed all of mwh and your concerns [14:03] [topic] on call: bac || reviewing: -|| queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:03] er === NCommander changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [NCommander] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:06] rockstar: hi, fancy a UI pre-imp check? [14:17] NCommander: approved with one tiny change. thanks === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [14:17] bac: eeek, oops. Sorry. I ran that test suite, nothing blew up :-/ [14:23] BjornT: how goes your backlog? [14:35] NCommander: slowly! [14:35] BjornT: ow :-/ [14:37] bac, hi, do you have time to take a look at https://code.edge.launchpad.net/~danilo/launchpad/bug-545070/+merge/21938? (very simple terminology change) [14:37] danilos: yep [14:38] bac, thanks [14:38] BjornT: if you don't have time to look at my branch today, no rush. Its a pretty simple change (one row addition, forgein key on LFA), and its not a huge priority [14:39] danilos: r=bac [14:39] bac, thanks [14:45] NCommander: oh, why didn't you say it was a simple change? :) approved [14:47] BjornT: awesome. Now how do I get it landed? :-) [14:48] bigjools, sure! [14:48] rockstar: great! https://code.edge.launchpad.net/~julian-edwards/launchpad/ppa-deletion-ui/+merge/21925 [14:48] NCommander: if you ask bac, he might be kind enough to help you :) [14:49] rockstar: there's no tests yet, just the basic implementation [14:49] but feel free to have a play [14:49] BjornT: should the status be set to Needs merge versus Needs Review? [14:50] bigjools, if it's UI, I don't care about tests. :) [14:50] rockstar: attaboy [14:51] NCommander: are all of your changes done and pushed? [14:51] bac: yes. [14:52] NCommander: ok, i'll send it through ec2 and land it for you [14:52] bac: w00t [15:17] bigjools, so, I don't feel like I know enough about Soyuz to really grok what I'm trying to review here (and there's no movie/screen shot). How can I get to the view? [15:19] rockstar: go to http://launchpad.dev/~cprov/+archive/ppa [15:19] sorry I assume too much :) [15:19] bigjools, :) It's okay. [15:20] bigjools, so, once a PPA is requested for deletion, can it be uploaded to? [15:21] Also, "Delete PPA" shouldn't show if the deletion request has already been made. [15:21] OTP, will type when I can :) [15:22] I also wonder if a red notification for "Deletion in progress" is probably better, since it's more likely to grab your attention. [15:22] Although the red often means "It's broken. It's broken! It's BROKEN!!!!" [15:45] bac: can you review https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-538469-invalid-upstream-link/+merge/21947 === salgado is now known as salgado-lunch [15:47] EdwinGrubbs: yes === matsubara is now known as matsubara-lunch === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: edwin|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [16:05] rockstar: ok sorry let me read backscroll === deryck is now known as deryck[lunch] [16:34] rockstar: gnargh too many distractions [16:34] rockstar: to answer your questions, no you can't upload to it, the browser code disables it for that reason [16:37] EdwinGrubbs: nice. r=bac [16:38] thanks [16:38] bigjools, okay. Did you see my other suggestions? [16:38] rockstar: making it red? [16:38] yeah [16:38] how do I do that? === bac changed the topic of #launchpad-reviews to: on call: bac-lunch || reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [16:44] bac: so, stupid question, is my branch on EC2, buildbot, or? (I'm kinda confused on how trees actually land ... or how PQM works) [16:57] rockstar: what's the best way of making that notification box red? === matsubara-lunch is now known as matsubara === salgado-lunch is now known as salgado [17:06] bigjools, I think that there's a similar method to the info notification method that does red boxes. [17:07] perfect === deryck[lunch] is now known as deryck [17:16] bigjools, also, the "Delete PPA" link shouldn't show if the deletion has already been requested. [17:16] rockstar: I thought about that, it was easier to leave it and make the page template different! [17:17] generally I really hate links appearing and disappearing with no explanation :/ [17:17] but I can change it [17:17] one day I hope we'll present disabled links [17:18] bigjools, grey it out then? [17:18] rockstar: does Link do that? [17:19] bigjools, I don't think so, but it should... :) [17:19] heh [17:19] bigjools, basically, you shouldn't be able to click it if it's no longer deletable (because it's already deleted) [17:20] I didn't think about it too hard because it won't be around longer than 5 minutes [17:20] http://validator.w3.org/feed/check.cgi?url=http%3A%2F%2Fplanet.nclug.org%2Frss20.xml [17:20] in any case we're changing tack slightly, and only removing the repository, the PPA will be disabled and hidden [17:21] * rockstar slaps his middle click [17:22] bigjools, yeah, I figured that, but to the user, it's "deleted" [17:22] rockstar: yeah [17:22] rockstar: so I'll make the whole thing disappear and redirect to the user's profile page, what do you think? [17:23] bigjools, yeah, that's probably a good idea. [17:23] And then the info notification is something like "The PPA deletion has been requested." [17:24] right [17:24] in red? [17:24] :) [17:25] bigjools, no, at that point, blue is fine. [17:33] rockstar: cool, thanks for the pre-imp [17:33] bigjools, no prob. [17:34] rockstar: I'll ping you for a review when it's done, apparently you need the experience :) [17:34] bigjools, yes, I'm only a lowly padawan. [17:35] One day I'll be a great Jedi warrior. [17:35] Either that, or become Darth Vader. [17:35] Depends on whether or not I fall in this river of lava. [17:35] These aren't the notifications you're looking for [17:35] * bigjools waves hand [17:36] my font made me read that as river of Java === gary_poster is now known as gary-lunch === salgado_ is now known as salgado [18:10] NCommander: can i get you to set the commit message on https://code.edge.launchpad.net/~mcasadevall/launchpad/raw_source_changelog_librarian/+merge/21909 === bac changed the topic of #launchpad-reviews to: on call: bac- || reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [18:33] bac: done [18:33] bac: did ec2 pass? [18:33] NCommander: haven't sent it off yet. there is a bug in our ec2 script. will do so now that you've set the commit msg [18:36] bac: very small branch https://code.edge.launchpad.net/~leonardr/launchpad/add-mutators/+merge/21965 === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: leonardr || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews [18:40] leonardr: done [18:40] bac, great [18:40] NCommander: i've sent your branch off to ec2 with 'ec2 land'. if the tests pass it will go directly to pqm. either way you'll get email [18:41] well, theoretically [18:44] abentley: thanks for your email yesterday about the pitiful state of +activereviews [18:46] bac, :-) === gary-lunch is now known as gary_poster [19:01] bac: are you running NCommander's branch through ec2 ? [19:01] * mwhudson reads backlog [19:01] bac: nm [19:01] mwhudson: yeah, i just started it [19:02] mwhudson: first attempt failed === EdwinGrubbs_ is now known as EdwinGrubbs === EdwinGrubbs_ is now known as EdwinGrubbs === bac changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews === salgado is now known as salgado-brb [21:52] NCommander: i sent your branch off to ec2 twice and if failed both times. it didn't send email, though, so i'm not sure what the problem is [21:52] NCommander: i'm running your tests locally [21:52] bac: ugh, now what? :-/ [21:52] see if i can get output locally [21:53] * NCommander feels like he really broke LP :-/ [21:56] can someone stamp https://code.edge.launchpad.net/~mwhudson/launchpad/disable-test_import_bzrsvn/+merge/21984 pls === matsubara is now known as matsubara-afk [22:10] bac: I'm probably going to be poof soonish, and then I'm travellig, so this branch might end up sitting until next week if theres a legit test failure [22:11] NCommander: i'll let you know what happens [22:11] bac: thanks. === salgado is now known as salgado-afk