[03:32] anyone got a sec? had ec2 find a new failure in my librarian branch; 1 line code change, 6 line test change (3 in two tests) [03:33] http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9600 [03:33] thumper: ^ [04:17] thumper: hi ? [04:18] lifeless: hi [04:18] can you eyeball http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9600 please [04:18] what does self._when_streaming() do? [04:18] also [04:18] why do you need to import the entire module ? import lp.services.features [04:19] self._when_streaming is a hook point for subclasses, I added it earlier but forgot to call it :( [04:19] it lets the subclass do $whatever, in this case add a header. [04:20] I could do from lp.services import features if you like [04:20] per_thread, the variable that needs to be poked, is a package scope variabe [04:20] theres a bug, I'll add an XXX to it [04:21] you could make a null feature fixture [04:21] we should just fix the bug [04:21] https://bugs.edge.launchpad.net/launchpad-foundations/+bug/631884 [04:21] <_mup_> Bug #631884: feature flags get out of sync easily [04:22] ok [04:22] do you have an MP? [04:22] yes [04:22] or just landing? [04:22] its already review on the MP [04:22] ok [04:22] fine then [04:22] this is fixing the MP because it found a bug [04:23] if you know what I mean [04:23] I've hit a snag with my lp:project for private code [04:23] a snag in that it isn't working [04:24] thumper: :( [04:24] thumper: which bit isn't working? [04:25] thumper: can we help? [04:25] mwhudson: only the bit that bzr calls to resolve lp:foo [04:25] possibly [04:25] if I go `bzr push bzr+ssh://bazaar.staging.launchpad.net/+branch/wikkid` it worked when I had privacy set and no trunk branch [04:25] but both push and pull fail with: [04:26] bzr: ERROR: Invalid url supplied to transport: "lp://staging/wikkid": Wikkid Wiki has no default branch. [04:26] which surprised me [04:26] I'm looking for the whole in the test coverage [04:45] I found the problem [04:45] the tests were there [04:45] but not checking for the right thing [11:07] any one on call? [11:08] https://code.edge.launchpad.net/~thumper/launchpad/xmlrpc-tests/+merge/34719 [11:08] rollout blocker ^^^ [11:20] done by gmb === noodles775 changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer [12:26] Hi henninge! If you'd like to, I've got a branch which will require a UI review at: A following branch will add the remaining non-js functionality (the search bar). [12:26] grr [12:26] https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 [12:27] but no pressure, or rush... it can wait until someone else is available if you're busy. [12:27] noodles775: no, that's ok. I am already looking at it. [12:28] Thanks henninge [12:28] * noodles775 takes a break for lunch. === mrevell is now known as mrevell-lunch [14:44] bac: Hi [14:44] hi jelmer [14:46] bac: I've (finally!) managed to fix up the tests for the branch I proposed on Friday, and I was wondering if you could have another look. Julian's also done a review this morning - we'd really like to get this in for the current release. [14:47] jelmer: ok. [14:48] jelmer: you have gotten all of the tests to work now? [14:49] bac: Yes. You were indeed right that there were some failing; what I was running on dogfood was slightly more hackish (but working) than what was in the branch I had proposed. [14:50] jelmer: i'd like to run the tests. it'll take about 30 minutes. that's ok for you? [14:50] bac: yeah, np [14:50] jelmer: ok. === mrevell-lunch is now known as mrevell [15:06] gmb: jelmer's branch is approved and awaits your RC approval: https://code.edge.launchpad.net/~jelmer/launchpad/506256-remove-popen/+merge/34549 [15:07] * bac was going to say "Imprimatur" but couldn't spell it. [15:08] gmb: actually, wait a bit on that. my testing may have been flawed [15:09] * gmb waits [15:18] gmb: done. [15:18] bac, Looking now. [15:21] bac, jelmer : rc=me; please land it on db-devel if you can; we want to close devel for landings today. [15:21] (Once it's un-broken) [15:21] gmb: ok [15:21] bac, gmb: Thanks for the reviews. [15:31] mars or leonardr: Could either of your review a trivial fix that we're hoping to land in the release? https://code.edge.launchpad.net/~michael.nelson/launchpad/db-611568-no-email-for-commercial-subscriptions/+merge/34757 [15:32] oh, right, it's tuesday [15:32] mars, are you around? it's your turn [15:35] noodles775, r=me [15:36] Thanks leonardr ! [15:36] gmb: If you've time, the above MP is an RC. [15:38] noodles775, Sure, I'll take a look now. [15:38] noodles775: Hi! [15:38] Hiya henninge [15:39] noodles775: I am sorry, I got busy doing other things ... [15:39] noodles775, rc=me. [15:39] Thanks gmb [15:39] noodles775, That's bound for db-devel isn't it? [15:39] gmb: I've set it for db-devel yes (in case devel is closed). [15:39] noodles775: http://launchpadlibrarian.net/55136007/distro-series-differences.png [15:39] noodles775, Brilliant. (it isn't closed but will be soon, hopfully) [15:39] noodles775: ^ that is what we already have? [15:40] henninge: no, that's what that branch adds. It's the first step towards the planned UI. [15:41] noodles775: so the mockup is about what the planned UI is but it will be done in a future branch or branches? [15:41] noodles775: and the screenshot is what this review is currently about? [15:42] henninge: yes, I need to do it in stages to keep it manageable. I'm not sure if you saw my comment on the MP, regarding whether I should wait until the UI is complete before requesting a review... I think we may need to re-think this with feature flags. [15:42] oh, missed that ;) [15:43] noodles775: no, UI reviews should be done as early as possible as they influence the coding [15:43] I don't want to build up a huge branch (or pipeline of braches) locally, so landing the UI incrementally seemed sane. [15:43] Yep, great. [15:44] noodles775: what happens if the "latest comment" is longer? Wrapping or truncating? [15:45] Currently it would wrap. [15:46] noodles775: I think I'd prefer truncating ... [15:46] or wrap once, then truncate if that is not too hard. [15:46] since there will be an option to expand an entry, the user will have easy access to the full comment. [15:48] henninge: yep, I agree (and yes, truncating won't stop wrapping, but it will be better than displaying long comments.) [15:49] noodles775: and maybe "no signer" should simply be "unknown" [15:50] noodles775: you know, I wish we had an easy way for "personal demos". [15:50] just push the branch and let it run. [15:50] Now I have to branch, make schema, make run to see your branch in action. [15:51] henninge: I did try to make it easy with the script ;) (ie. merge;make schema;bin/iharness < sample_data.py) [15:52] henninge: I think we can (or at least, could) put ec2 instances up for demo, but not sure its worth it. [15:53] noodles775: there was a description for that, iirc. Maybe there is already an "ec2" command for that. [15:53] * henninge has to explore that but not now. [15:53] noodles775: I am just wondering what the package name and versions link to. [15:54] noodles775: and what does the "-" link to in the first column? [15:54] it looks blue [15:54] Both columns link to the source package publishing histories in the parent/derived series respectively. [15:55] henninge: it is part of the same link. The first column uses the name-version as the link text, the second column doesn't repeat the name (as in the mockup, as having a separate column for the name looked strange). [15:56] (which is why the column headers are "Warty package"/"Hoary version") [15:56] yes, that is quite a neat solution [15:57] I was just considering if having an extra column would really look that bad [15:57] Regarding the "no signer", that was just based on the precedent of the PPA packages page, but unknown might be clearer. [15:57] but this solution is fine, too. [15:57] noodles775: also, the version numbers seem to go *down* from parent to child or is that just a glitch in your sample data? [15:57] noodles775: what is a blacklist, btw? [15:58] Argh! [15:58] oh, you mean from the mockup (I was confused, knowing it wasn't mentioned on the screenshot). [15:58] ? [15:58] noodles775: yes, mockup [15:59] Argh! [15:59] noodles775: I just had a call and have to run out right now. Terribly sorry. [15:59] noodles775: Is it OK if I sum up our conversation to a review later? [16:00] henninge: of course! [16:00] Thanks! [16:00] henninge: for your summary, the choice of "blacklist" is here https://lists.launchpad.net/launchpad-dev/msg04536.html [16:00] thanks === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer [16:12] noodles775, are you still in the queue? [16:13] leonardr: I am, for a different branch: https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 [16:13] noodles775: ok, mars is off today, so i'll take it in a bit [16:14] leonardr: thanks! [16:42] leonardr: I need to be off, do you mind doing the review via email? [16:42] noodles775, sure === benji is now known as benji-lunch [17:52] rockstar: ping [17:52] henninge, pong [17:52] rockstar: Hi! ;-) [17:52] henninge, how's things? [17:52] very good, thanks. I just did a UI review. [17:52] https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-browser2/+merge/34739 [17:53] henninge, great, I'll take a look. [17:55] henninge, instead of "no signer" or "unknown" perhaps we shouldn't show it at all. [17:56] rockstar: I thought the "when" information was still of interest? [17:56] rockstar: maybe the column should be called "Last uploaded" and the content "18 minutes ago by Foo Bar" [17:57] henninge, I was just about to suggest that exact thing. [17:57] or just "18 minutes ago" [17:57] do we have similar columns elsewhere on LP? [17:57] henninge, basically, if the user can't do anything about it, we shouldn't show "unknown" because the user may think they're doing something wrong. [17:57] * rockstar JUST learned that in his UX class. [17:57] good point [17:57] ;) [17:58] henninge, we have similar date columns, and I think we could/should adopt a similar pattern to what we're proposing here. [17:58] sounds good [17:58] henninge, great. [18:00] rockstar: Also, I begin to wonder if the Name of the commenter should not be linkified, too, like the uploader. [18:00] but that would put more stuff in the comment colmun. [18:01] henninge, yeah, and we should use a full display name. [18:01] henninge, if we still limit it to two lines, that would still be okay. [18:01] rockstar: did you noticed that the name of the currently logged-in user (top right) is not the full name anymore? [18:01] I wonder why that was changed. [18:02] rockstar: for the comment column: I actually think it should be like the upload column: [18:02] henninge, because full names aren't unique. [18:02] ah [18:02] "18 minutes ago by Mark Shuttleworth: I am working on this" [18:03] henninge, yup. I think that's a good idea. === benji-lunch is now known as benji === deryck is now known as deryck[lunch] [18:07] rockstar: are you going to add those suggestions to the MP or shall I do it? [18:08] henninge, you could just put our whole discussion from IRC copy-n-pasted into a comment there. [18:11] rockstar: done === deryck[lunch] is now known as deryck [19:43] leonardr: up for a short but critical review? https://code.edge.launchpad.net/~gary/launchpad/bug632218/+merge/34785 . The description is a bit bare bones, but I'm hoping the linked bug report (bug 632218) gives you an idea of what is going on and what I set out to do. [19:43] <_mup_> Bug #632218: staging builddmaster is having building woes with permissions - template engine [19:43] gary: ok, if it's shorti 'll take it now [19:44] it is [19:55] leonardr: (did you see that? ^^^) [19:55] gary: yes, reading the bug now [19:55] cool thank you [19:56] gary: so you remove *.pt.py during make clean? [19:57] yes, leonardr [19:57] and you give the 'compile' step responsibility for (re-)creating them? [19:57] yes [19:57] ok, r=me [19:57] thank you [20:22] leonardr: have time for a modest branch to review? [20:22] bac, sure, put it in the queue [20:22] leonardr: not RC === bac changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: - || queue: [noodles775, bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer [20:22] leonardr: https://code.edge.launchpad.net/~bac/launchpad/bug-237722/+merge/34790 [21:13] leonardr: given the queue and the time, I doubt you have time for any more reviewing today, right? [21:13] benji: actually the queue is about to be emptied. i can do one more if it's not too long === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: bac || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer [21:14] leonardr: it should be pretty short: https://code.edge.launchpad.net/~benji/launchpad/bug-413174/+merge/34794 [21:14] it's just using the new lazr.restful.error.expose functionality to kill a couple of OOPSes in LP [21:16] cool [21:22] thanks for the nice review leonardr [21:36] benji, why put the message as the default in MultipleProductReleases? i'd rather see it at the single raise site [21:36] in fact, why have a separate exception that you only use once? [21:38] I'm not married to it. My reasoning was that there is only one reason to raise that exception (vs. the other exception I added) so I made it the default text. In other words, put it in the place that matches its generality. [21:38] would it be make sense to raise expose(Exception("A milestone can only have...")) ? [21:39] or ValueError or something [21:39] re. separate exception: we want to be able to distinguish it from other exceptions that might be raised from that function [21:39] right, I considered ValueError, but the cost of adding a more specific exception seemed lower than the value of being explicit [21:43] ok, it's your call [21:43] can you refactor get_last_oops_id into a helper module? [21:46] benji, on line 246, if for some reason the exception isn't thrown, the test will pass when it should fail [21:46] oh, no, nevermind [21:46] you tested that immediately before [21:58] benji: looks good, just refactor get_last_oops_id [22:05] benji: you might like self.assertNoNewOops [22:05] ooh, I might indeed [22:05] currently only used in lib/lp/services/job/tests/test_runner.py [22:06] plus, TestCases have an oopses attribute that might be useful === abentley_ is now known as abentley [22:14] leonardr: in what way do you want me to "[r]efactor get_last_oops_id"? [22:15] let me double check, but i think you define the same method twice [22:15] (I think I'll just be removing it.) [22:15] ah, yes, just remove it [22:15] leonardr: ah, indeed! [22:15] thanks === 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 || gmb is RM; for RC reviews please add him as a reviewer === 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 || gmb is RM; for RC reviews please add him as a reviewer