[00:00] <wgrant> jelmer: 15 minutes? kdebase too almost 9 hours...
[00:00] <wgrant> s/too/took/
[00:01] <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:02] <wgrant> The cache should be shared across the whole repo, though, right?
[00:03] <jelmer> wgrant, yes
=== salgado is now known as salgado-afk
=== StevenK_ is now known as StevenK
[02:35] <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:36] <StevenK> thumper: https://code.edge.launchpad.net/~stevenk/launchpad/fix-ssh-keys-make-lp-user/+merge/32543
[02:42] <thumper> StevenK: which soyuz test uses it?
[02:44] <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:45] <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:46] <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:47] <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:48] <StevenK> But I'm happy to propose another branch that just removes that whole doctest
[02:49] <thumper> is the purpose of the doc test to make sure that the script doesn't break?
[02:49] <StevenK> Yes
[02:50] <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:51] <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:52] <thumper> 2) take the easy way out, file a bug and add an XXX comment referencing the bug, and tag it tech-debt
[02:53] <StevenK> I'll do 2, and have a discussion about it when Julian isn't sick
[02:54] <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:55] <thumper> file the bug, add the comment, and I'll approve it
[02:55] <StevenK> Yup, doing so now
[02:58] <StevenK> thumper: Bug filed, comment added, branch pushed.
=== 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
[04:36] <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:38] <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:39] <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
=== 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
[10:04] <adeuring> On call: adeuring || reviewing: bryce || queue: []  || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== 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
[13:08] <jelmer> Hi Abel, can I add a branch to your queue?
=== matsubara-afk is now known as matsubara
=== mrevell is now known as mrevell-lunch
[13:33] <adeuring> jelmer: sure
=== 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
[13:36] <jelmer> adeuring: Thanks, the MP is at https://code.edge.launchpad.net/~jelmer/launchpad/128259-async-1/+merge/32439
[13:54] <adeuring> jelmer: the last assertTrue() in testSuccess() needs something like "in log_lines"
[13:58] <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:59] <adeuring> yet
=== mrevell-lunch is now known as mrevell
[14:20] <adeuring> jelmer: r=me, with a few more nitpicks
[14:20] <jelmer> adeuring: Thanks!
=== 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
[14:45] <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:55] <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:56] <bac> adeuring:  didn't want you to think i'd abandoned you
[14:57] <adeuring> bac: so, do you want to review sinzui's branch or shall i do it?
[14:59] <sinzui> bac said he would since he cares about the feature
[14:59] <sinzui> adeuring, ^
[14:59] <adeuring> sinzui: thanks;
[15:00] <sinzui> adeuring, bac will review all my branches because we are sprinting together.
[15:00] <adeuring> sinzui: thanks for the clarification
[15:46] <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
=== 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
[15:59] <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 ?
=== deryck is now known as deryck[lunch]
[16:33] <adeuring> leonardr: r=me
[16:36] <leonardr> adeuring, thanks
=== 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
[17:20] <rockstar> salgado, those changes look fine to me.
[17:20] <salgado> rockstar, cool, thanks!
=== benji is now known as benji-lunch
[17:55] <salgado> is there anybody willing to do a small review for me? https://code.edge.launchpad.net/~salgado/launchpad/vostok-traverse-package/+merge/32599
[18:30] <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:36] <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:37] <salgado> cool
[18:40] <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?
=== Ursinha-lunch is now known as Ursinha
=== benji-lunch is now known as benji
[18:47] <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:50] <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:51] <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:52] <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:53] <salgado> I'll do that change
[18:53] <salgado> thanks leonardr
[18:54] <salgado> oh, that's in the mp.  I thought it was in the code
[18:54] <salgado> duh
[20:58] <lifeless> EdwinGrubbs: hi
[21:11] <EdwinGrubbs> lifeless: hi
[21:15] <lifeless> I've replied on the MP
[21:31] <lifeless> but I'd be very happy to chat realtime as well if thats useful
[22:54] <mars> salgado, have a minute to review https://code.edge.launchpad.net/~mars/launchpad/fix-ec2-shutdown-617598/+merge/32633 ?
[22:58] <salgado> mars, care to add a comment explaining why we use 'at' instead of shutdown +X?
[22:59] <mars> salgado, good idea, will do
[22:59] <salgado> r=me with that
[22:59] <mars> great, thanks!
=== salgado is now known as salgado-afk
[23:04] <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:13] <mars> jelmer, why is the 'from dulwich.repo import Repo as GitRepo' on line 36 right beside the test code?
[23:14] <mars> jelmer, and should you be using .getUniqueString() as the message parameter?
[23:15] <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:16] <mars> hehe, yep, saw that :)
[23:16] <mars> jelmer, up to you.  I'll stamp it r=mars.  And about the import statement?
[23:17] <mars> oh, "It did that before" for /both/ the string and the import statement
[23:18] <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:19] <mars> and if .getUniqueString() is already there, just takes a few ^W to fix it
[23:26] <jelmer> I have no idea what ^W does, but I think I understand what you mean :-)
[23:26] <mars> readline delete word left
[23:27] <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:28] <jelmer> Heh, that's my first association with ^W as well.
[23:29] <jelmer> Anyway, I'm off.
[23:29] <jelmer> mars: Thanks again for the review, and have a nice weekend!
=== Ursinha is now known as Ursinha-afk