| wgrant | jelmer: 15 minutes? kdebase too almost 9 hours... | 00:00 |
|---|---|---|
| wgrant | s/too/took/ | 00:00 |
| jelmer | wgrant: it depends on the machine. pear importing kdelibs4 seems to take 15 minutes to figure out what revisions to fetch, 3 minutes to fetch the actual 1k revisions and then some time importing tags and moving branches around | 00:01 |
| mwhudson | each machine has to build the cache separately | 00:01 |
| wgrant | Ah, I see. | 00:01 |
| mwhudson | which is lame, but... | 00:01 |
| wgrant | The cache should be shared across the whole repo, though, right? | 00:02 |
| jelmer | wgrant, yes | 00:03 |
| === salgado is now known as salgado-afk | ||
| === StevenK_ is now known as StevenK | ||
| StevenK | thumper, lifeless: Do either of you have a few moments to look over a quick MP I put up? | 02:35 |
| thumper | StevenK: sure | 02:35 |
| StevenK | thumper: https://code.edge.launchpad.net/~stevenk/launchpad/fix-ssh-keys-make-lp-user/+merge/32543 | 02:36 |
| thumper | StevenK: which soyuz test uses it? | 02:42 |
| StevenK | thumper: Sorry, was distracted by mumble. lib/lp/soyuz/doc/sampledata-setup.txt | 02:44 |
| StevenK | Which calls utilities/soyuz-sampledata-setup.py | 02:44 |
| thumper | this is starting to make me hurl | 02:45 |
| thumper | what does that do? | 02:45 |
| StevenK | Haha, I knew I should have warned you | 02:45 |
| StevenK | utilities/soyuz-sampledata-setup.py sets up the database so developers can run Soyuz locally | 02:46 |
| thumper | so it isn't part of the actual test suite? | 02:46 |
| thumper | let me put this another way: | 02:46 |
| thumper | we should NOT be calling utility scripts from the test suite | 02:47 |
| StevenK | Well, the way I found this bug is via the test suite | 02:47 |
| StevenK | And I think the bug should be fixed | 02:47 |
| StevenK | But I'm happy to propose another branch that just removes that whole doctest | 02:48 |
| thumper | is the purpose of the doc test to make sure that the script doesn't break? | 02:49 |
| StevenK | Yes | 02:49 |
| thumper | what should be done then, is to move the guts of the scripts into something like lp.soyuz.tests or lp.soyuz.scripts.tests | 02:50 |
| thumper | and test that | 02:50 |
| thumper | and have the utility scripts call into those tested functions | 02:50 |
| thumper | you have two choices | 02:50 |
| thumper | 1) keep pulling on the thread and move the buts that need to be tested into lp.soyuz.<wherever>.tests | 02:51 |
| thumper | (or scripts) | 02:51 |
| thumper | and have the tests in tests | 02:51 |
| thumper | or | 02:51 |
| thumper | 2) take the easy way out, file a bug and add an XXX comment referencing the bug, and tag it tech-debt | 02:52 |
| StevenK | I'll do 2, and have a discussion about it when Julian isn't sick | 02:53 |
| thumper | ok | 02:54 |
| StevenK | thumper: But in either case, this is a real bug in make-lp-user, and should be fixed. | 02:54 |
| thumper | ack | 02:54 |
| thumper | file the bug, add the comment, and I'll approve it | 02:55 |
| StevenK | Yup, doing so now | 02:55 |
| StevenK | thumper: Bug filed, comment added, branch pushed. | 02:58 |
| === lifeless_ is now known as lifeless | ||
| === rockstar changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| wgrant | thumper: Why shouldn't it be called? | 04:36 |
| wgrant | The point is to test the script. | 04:36 |
| wgrant | And running the script is probably a reasonable way to do that. | 04:36 |
| thumper | if it is a tested script it should be in lp.soyuz.scripts | 04:38 |
| thumper | it is all about expectation | 04:38 |
| thumper | utilities are extra bits | 04:38 |
| thumper | the test suite should test lib/lp | 04:38 |
| thumper | not utilities | 04:38 |
| thumper | although it does test cronscripts and other scripts | 04:38 |
| thumper | but I think utilities is too much | 04:39 |
| wgrant | Why? | 04:39 |
| wgrant | Why not test it? What does it cost? | 04:39 |
| thumper | I'm not saying the code should be untested | 04:39 |
| thumper | it is about expectation | 04:39 |
| === maxb_ is now known as maxb | ||
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [bryce] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| adeuring | On call: adeuring || reviewing: bryce || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | 10:04 |
| === adeuring1 changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === adeuring1 is now known as adeuring | ||
| jelmer | Hi Abel, can I add a branch to your queue? | 13:08 |
| === matsubara-afk is now known as matsubara | ||
| === mrevell is now known as mrevell-lunch | ||
| adeuring | jelmer: sure | 13:33 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: jelmer || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jelmer | adeuring: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/128259-async-1/+merge/32439 | 13:36 |
| adeuring | jelmer: the last assertTrue() in testSuccess() needs something like "in log_lines" | 13:54 |
| jelmer | adeuring: Nice catch. I wonder if the matchers that James and lifeless have been working on has is useful here. | 13:58 |
| adeuring | jelmer: maybe; haven't used them | 13:58 |
| adeuring | yet | 13:59 |
| === mrevell-lunch is now known as mrevell | ||
| adeuring | jelmer: r=me, with a few more nitpicks | 14:20 |
| jelmer | adeuring: Thanks! | 14:20 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-afk | ||
| EdwinGrubbs | lifeless: can you look at the db changes in this branch? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490659-series-timeout/+merge/32555 | 14:45 |
| bac | hi adeuring | 14:55 |
| adeuring | hi bac! | 14:55 |
| bac | adeuring: i'm sprinting this week and won't be doing reviews full-time today | 14:55 |
| adeuring | bac: ok, no problem. | 14:55 |
| bac | but i'll be doing reviews for branches coming out of our sprint | 14:55 |
| adeuring | bac: even better :) | 14:55 |
| bac | adeuring: didn't want you to think i'd abandoned you | 14:56 |
| adeuring | bac: so, do you want to review sinzui's branch or shall i do it? | 14:57 |
| sinzui | bac said he would since he cares about the feature | 14:59 |
| sinzui | adeuring, ^ | 14:59 |
| adeuring | sinzui: thanks; | 14:59 |
| sinzui | adeuring, bac will review all my branches because we are sprinting together. | 15:00 |
| adeuring | sinzui: thanks for the clarification | 15:00 |
| leonardr | adeuring, i would appreciate a review of https://code.edge.launchpad.net/~leonardr/lazr.restful/check_total_size_active_on_call/+merge/32590 | 15:46 |
| adeuring | leonardr: sure | 15:46 |
| === adeuring changed the topic of #launchpad-reviews to: On call: adeuring || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| salgado | rockstar, yesterday I had a chat with flacoste and he suggested a change to https://code.edge.launchpad.net/~salgado/launchpad/layer-specific-navigation/+merge/32358, which you approved yesterday. care to have a look at the incremental changes: http://bazaar.launchpad.net/~salgado/launchpad/layer-specific-navigation/revision/9640 and http://bazaar.launchpad.net/~salgado/launchpad/layer-specific-navigation/revision/9642 ? | 15:59 |
| === deryck is now known as deryck[lunch] | ||
| adeuring | leonardr: r=me | 16:33 |
| leonardr | adeuring, thanks | 16:36 |
| === deryck[lunch] is now known as deryck | ||
| === adeuring 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 | ||
| === Ursinha is now known as Ursinha-lunch | ||
| rockstar | salgado, those changes look fine to me. | 17:20 |
| salgado | rockstar, cool, thanks! | 17:20 |
| === benji is now known as benji-lunch | ||
| salgado | is there anybody willing to do a small review for me? https://code.edge.launchpad.net/~salgado/launchpad/vostok-traverse-package/+merge/32599 | 17:55 |
| leonardr | salgado, let's trade | 18:30 |
| leonardr | i need a follow-up | 18:30 |
| leonardr | https://code.edge.launchpad.net/~leonardr/launchpad/new-lazr.restful-and-friends/+merge/32474 | 18:30 |
| salgado | leonardr, deal! | 18:30 |
| salgado | leonardr, approved. out of curiosity, have you changed total_size into a link because it may be expensive to compute and not always necessary? | 18:36 |
| leonardr | salgado: exactly | 18:36 |
| salgado | cool | 18:37 |
| leonardr | salgado, why did it become necessary to remove the security proxy on the bug? | 18:40 |
| leonardr | and what does the .. syntax in your zcml mean? | 18:40 |
| === Ursinha-lunch is now known as Ursinha | ||
| === benji-lunch is now known as benji | ||
| leonardr | does it make sense to add a test that shows you can't traverse to a source package on some site other than vostok? | 18:47 |
| leonardr | answer me those questions three and i will approve your branch | 18:47 |
| salgado | leonardr, I think the makeBugTask() method was not used anywhere, or it's used by tests which log in as admins before calling it | 18:50 |
| leonardr | ah | 18:50 |
| salgado | the .. syntax means relative paths; '..' is the parent directory | 18:50 |
| leonardr | ok | 18:50 |
| salgado | and you can traverse to source packages on the launchpad.dev vhost | 18:51 |
| leonardr | but your mp says "it's only possible to traverse to distro series and source packages on the vostok.dev vhost" | 18:51 |
| leonardr | did you mean that it's not possible to traverse to anything else on vostok.dev? | 18:51 |
| salgado | what we want is to make sure that in vostok.dev you can only traverse to a subset of those you can traverse to on launchpad.dev | 18:52 |
| leonardr | ok, the language was ambiguous | 18:52 |
| leonardr | r=me | 18:52 |
| salgado | yeah, I guess I should change that to "it's possible to traverse only to ..." | 18:52 |
| salgado | on vostok.dev it's possible to traverse only to ... | 18:52 |
| salgado | I'll do that change | 18:53 |
| salgado | thanks leonardr | 18:53 |
| salgado | oh, that's in the mp. I thought it was in the code | 18:54 |
| salgado | duh | 18:54 |
| lifeless | EdwinGrubbs: hi | 20:58 |
| EdwinGrubbs | lifeless: hi | 21:11 |
| lifeless | I've replied on the MP | 21:15 |
| lifeless | but I'd be very happy to chat realtime as well if thats useful | 21:31 |
| mars | salgado, have a minute to review https://code.edge.launchpad.net/~mars/launchpad/fix-ec2-shutdown-617598/+merge/32633 ? | 22:54 |
| salgado | mars, care to add a comment explaining why we use 'at' instead of shutdown +X? | 22:58 |
| mars | salgado, good idea, will do | 22:59 |
| salgado | r=me with that | 22:59 |
| mars | great, thanks! | 22:59 |
| === salgado is now known as salgado-afk | ||
| jelmer | Could somebody perhaps re-review https://code.edge.launchpad.net/~jelmer/launchpad/update-bzr-git/+merge/32527 ? Tim already approved the changes itself but I had to fix two tests as well. | 23:04 |
| mars | jelmer, why is the 'from dulwich.repo import Repo as GitRepo' on line 36 right beside the test code? | 23:13 |
| mars | jelmer, and should you be using .getUniqueString() as the message parameter? | 23:14 |
| jelmer | mars: This isn't a valid excuse, but the reason I did it that way is because that's what the old code was doing to import run_git as well. | 23:15 |
| mars | hehe, yep, saw that :) | 23:16 |
| mars | jelmer, up to you. I'll stamp it r=mars. And about the import statement? | 23:16 |
| mars | oh, "It did that before" for /both/ the string and the import statement | 23:17 |
| jelmer | mars: Yep. But I agree the imports should be at the top of the file and using getUniqueString() is preferable to using "dsadsa". | 23:18 |
| mars | ok, I would at least change the import statement - it will stand out when others read the code :) | 23:18 |
| jelmer | mars: Thanks for the review. | 23:18 |
| mars | and if .getUniqueString() is already there, just takes a few ^W to fix it | 23:19 |
| jelmer | I have no idea what ^W does, but I think I understand what you mean :-) | 23:26 |
| mars | readline delete word left | 23:26 |
| mars | dangerous to learn, because it is also Gnome/CUA 'Close Window and lose everything you were doing because you were typing too fast and hit Enter instead of Cancel on the Save dialog' | 23:27 |
| jelmer | Heh, that's my first association with ^W as well. | 23:28 |
| jelmer | Anyway, I'm off. | 23:29 |
| jelmer | mars: Thanks again for the review, and have a nice weekend! | 23:29 |
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!