| === jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | thumper: you're probably eod, but if not… care to review? https://code.edge.launchpad.net/~jtv/launchpad/bug-643345/+merge/36258 | 07:20 |
|---|---|---|
| jtv | Anyone else who could review for me? stub? | 07:30 |
| jtv | jml maybe? | 07:34 |
| jtv | gmb, you perhaps? | 07:50 |
| jtv | allenap then? | 07:53 |
| jtv | StevenK? | 07:54 |
| StevenK | jtv: I can look, but I can only give you code* | 07:55 |
| jtv | StevenK: theoretically that would slow down the final review, but it's not a very long branch so might as well. | 07:57 |
| jtv | did noodles775 just join us? | 07:57 |
| noodles775 | Join you for what? ;) Morning! | 07:57 |
| jtv | morning! | 07:57 |
| jtv | I'm shopping around for a reviewer for a CP branch… StevenK could do it, but will need a mentor to approve his review. | 07:58 |
| jtv | It's this branch: https://code.edge.launchpad.net/~jtv/launchpad/bug-643345/+merge/36258 | 07:58 |
| noodles775 | Sure. | 07:58 |
| jtv | \o/ | 07:59 |
| noodles775 | jtv: maybe you could help me too? I've got an ec2 test failure email, but the tests pass locally, and the version of db-devel that was most recently merged also went through buildbot fine (r9808: https://lpbuildbot.canonical.com/builders/lucid_db_lp/builds/237) | 08:02 |
| jtv | noodles775: happy to return the favour | 08:02 |
| noodles775 | I've stared at it a bit yesterday, but would love a second pair of eyes? | 08:02 |
| * jtv looks | 08:02 | |
| noodles775 | Thanks, I'll forward the failure email. | 08:02 |
| jtv | noodles775: I know the feeling—I could do with a new pair of eyes myself. | 08:03 |
| noodles775 | The email has the branch that you should be able to make and test locally. Thanks jtv | 08:04 |
| StevenK | noodles775: jtv's MP doesn't have any obvious gotchas that I can see. | 08:06 |
| noodles775 | Thanks StevenK | 08:06 |
| jtv | noodles775: wow, that's not a very informative failure is it? | 08:07 |
| noodles775 | jtv: no, and it's not just one... there's also 2 windmill errors (which also pass locally, at least did for me). | 08:08 |
| jtv | Yes, I'm looking at them | 08:08 |
| jtv | noodles775: the test failure (as opposed to the 2 errors) looks familiar | 08:08 |
| noodles775 | I thought so too, but couldn't see any evidence of it on buildbot. | 08:09 |
| jtv | we had something like that recently… I think StevenK investigated but the problem was elusive | 08:09 |
| jtv | IIRC the test had just been converted from using getLatestOops (or something along those lines) to self.oopses (or something along those lines) when that happened. | 08:10 |
| noodles775 | StevenK: http://pastebin.ubuntu.com/498260/ | 08:10 |
| noodles775 | ? | 08:10 |
| noodles775 | jtv: did it also fail only on ec2 test and not on db-devel? (that would explain why I can't see it on the recent buildbot runs). | 08:12 |
| noodles775 | jtv: And if that's the case, can you confirm for me that all 3 tests pass for you locally on that branch, and I'll submit it? | 08:12 |
| StevenK | Funny you should mention that: https://hudson.wedontsleep.org/job/db-devel/44/testReport/lp.services.job.tests.test_runner/TestJobRunner/test_runAll_mails_oopses/ | 08:12 |
| jtv | noodles775: the former is something StevenK can answer better, and the latter is something I'm about to try | 08:12 |
| noodles775 | StevenK: Ah! So it's not just my branch! So why is it failing ec2 test and hudson, but not db-devel? | 08:13 |
| StevenK | noodles775: To be completly honest, I'm not sure | 08:14 |
| StevenK | But I am happy that hudson has a test failure that is useful and reproducable | 08:15 |
| noodles775 | Different versions of oops-tools? | 08:15 |
| jtv | could that make a difference to the running of the tests? | 08:21 |
| jtv | I thought that was all post-facto processing, not related to actual generation of oopses. | 08:21 |
| jtv | noodles775: the "failure" one passes for me… trying the "error" ones. | 08:25 |
| noodles775 | Thanks. | 08:25 |
| jtv | StevenK: found any nits to pick yet? :) | 08:26 |
| noodles775 | jtv: only things I've thought so far are: | 08:26 |
| noodles775 | 1) It wasn't obvious to me without reading the code below that committer_string isn't an actual commit string provided (somehow) by the committer. Not sure if there's a better name, or maybe its just me. | 08:27 |
| noodles775 | 2) In getBzrCommitterID you're using self.committer.preferredemail, even though abentley's change of the related bug title seems to indicate this is wrong? | 08:28 |
| noodles775 | (but I don't know the background) | 08:28 |
| jtv | noodles775: the first of those errored windmill tests passed for me, but the second crashed firefox. (I'm on maverick beta) | 08:28 |
| noodles775 | So am I... that's interesting though, I'd not run them together, always separately. | 08:29 |
| jtv | noodles775: I'd be happy to come up with a better name for 1. As for 2, that falls under the "laying groundwork" part—there must be better fixes, but for now I just want the failures out of the way. | 08:29 |
| jtv | noodles775: I'm running the tests separately as well. | 08:29 |
| noodles775 | Yes, but (regarding 2) couldn't you just remove that elif for the moment to get the failures out of the way? | 08:30 |
| thumper | jtv: reviewed | 08:30 |
| noodles775 | jtv: Ah, another thought... I wonder if the windmill failures are related to the FF version. | 08:30 |
| jtv | noodles775: well AIUI it's wrong _because_ it's not guaranteed to be present. | 08:30 |
| jtv | Not inherently wrong otherwise that I know of—people on that end made a deliberate decision to use email addresses, and I don't want to controvert them wholesale. | 08:31 |
| jtv | thumper: thanks! I also have noodles775 looking at it just now | 08:31 |
| thumper | jtv: ok | 08:31 |
| noodles775 | Ah, I'll just add my review so far as a comment FTR - I didn't realise thumper was/had looked at it. | 08:31 |
| thumper | noodles775: that's fine | 08:32 |
| jtv | thumper: but very happy to have your view on it | 08:32 |
| thumper | I didn't read all the comments after jtv pinged | 08:32 |
| jtv | there were too many :) | 08:32 |
| jtv | noodles775: the branch puller test leaves a branch locked, but otherwise its test passes for me. | 08:32 |
| noodles775 | jtv: so one windmill test errored for you... which one was it? test_pofile_new_translation_autoselect ? | 08:34 |
| jtv | yes | 08:34 |
| * noodles775 runs it locally | 08:35 | |
| jtv | and once Firefox gets involved, I'm not even willing to guess at the moving parts and variables that come into play… | 08:35 |
| jtv | thumper: pushed changes for your review | 08:44 |
| jtv | (moving on to noodles' review) | 08:44 |
| jtv | noodles775: pushed changes for your review | 08:47 |
| thumper | ack | 08:48 |
| jtv | noodles775: just pushed another tiny, tiny thing I missed in renaming that parameter. Nothing you wouldn't expect. | 08:52 |
| noodles775 | Yep, looking now. | 08:53 |
| jtv | dankeschön | 08:53 |
| noodles775 | jtv: committer_id is much clearer, thanks. And test_pofile_new_translation_autoselect still passes fine for me locally. | 08:56 |
| jtv | noodles775: maverick or lucid? | 08:57 |
| noodles775 | maverick | 08:57 |
| jtv | same here :( | 08:59 |
| jtv | may be a plugin | 08:59 |
| jtv | phone call… | 08:59 |
| noodles775 | jtv, StevenK: the oops error just appeared on buildbot, so that one seems spurious. | 08:59 |
| jtv | :/ | 09:01 |
| StevenK | Oooh, so it is real | 09:01 |
| noodles775 | Real in the sense that it happens sometimes even on buildbot, yes. | 09:02 |
| jtv | noodles775: by the way, sorry about the double-review hassle; I was knocking on doors and moving on when I got no reply. Are things to your satisfaction now? | 09:17 |
| noodles775 | jtv: no problem! Yep - I only reviewed the code as per my comment, but I'm happy with that (after your variable change). | 09:18 |
| jtv | noodles775: thanks—then I'll land on devel | 09:18 |
| jml | *sigh* | 09:39 |
| jml | no one reviewed my branch yesterday | 09:39 |
| jtv | jml: as it happens, I'm OCR. Putting out some fires today, which is why I haven't gotten around to the backlog, but I could do it now. | 09:39 |
| jml | jtv, that'd be great, thanks. | 09:39 |
| jml | https://code.edge.launchpad.net/~jml/launchpad/buildd-slavescanner-bustage/+merge/36187 | 09:40 |
| === jtv changed the topic of #launchpad-reviews to: On call: jtv || Reviewing: jml || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | jml: you're turning into a Hindu god. | 09:40 |
| jml | jtv, oh? | 09:41 |
| jtv | Lots of creative destruction and, evidently, more than the normal quota of hands to type with. | 09:41 |
| jtv | jml: who is "we" in this narrative by the way? | 09:42 |
| jtv | Royal "we"? | 09:42 |
| jml | jtv, bigjools & I | 09:43 |
| jml | jtv, and Shiva | 09:43 |
| jtv | jml: …and this is why the comma is a bad way to indicate that you're addressing someone directly in IRC. :) | 09:43 |
| jml | jtv, I hate it the comma autocomplete | 09:43 |
| jml | jtv, but xchat doesn't give me a control to change it | 09:44 |
| jtv | ah! there are clients that do that? | 09:44 |
| jtv | a mild bowel discomfort be upon them | 09:44 |
| jtv | jml: diff line 41 is weird: "Similarly, but" | 09:45 |
| jml | jtv, oh yeah. I guess "But" will do | 09:46 |
| jtv | jml: I'm unable to find the connection between the narrative and the test there, but that's probably just me. If you assume me they match, that's good enough for me—as long as I get you to expend some thought on the matter. | 09:48 |
| jml | jtv, they do match. the broader narrative is about checking the deb lines. | 09:49 |
| jtv | jml: I believe you blindly. | 09:50 |
| jml | jtv, I don't think archive-dependencies.txt is a well-written document, and would rather (one day) move all of those to unit tests. | 09:50 |
| jtv | I'm glad you're making the effort. | 09:50 |
| jtv | jml: diff line 698 would read even better IMHO if you switched conditions around to read "... and not ..." | 09:56 |
| jml | jtv, good call | 09:57 |
| jtv | jml: also, I see some XXX comments that lack date, name, and bug reference. | 09:58 |
| jml | jtv, yeah, I'll quickly review those now. | 09:59 |
| jtv | thanks | 09:59 |
| jtv | jml: a bit scary that the OkSlave previously did almost nothing but then suddenly Popen'ed in resume()… | 10:03 |
| jml | jtv, yes, it is. | 10:05 |
| jml | jtv, lots of this stuff is scary | 10:05 |
| jml | jtv, maybe in a branch today or tomorrow we'll actually kill all of those mocks and do the mocking at the xmlrpclib layer. | 10:06 |
| jtv | that sounds like a cool idea… so much of it is no-brainer replication anyway. | 10:06 |
| jml | exactly | 10:06 |
| jtv | The call log is quite close to what FakeMethod does, even. | 10:06 |
| jml | yeah, but we are selective about what we append | 10:06 |
| jml | the whole thing is really close to niemeyer's mocker | 10:07 |
| * jtv sobs quietly into a handkerchief, then composes himself | 10:07 | |
| jml | which I'm not sure why we don't use. | 10:07 |
| jtv | Not in this branch though! | 10:07 |
| jtv | But yes, if that fits, by all means do it. | 10:07 |
| jtv | Ah, a meaningful docstring on a test case. The astrologists always said it would happen, but I didn't expect to see it in my lifetime. | 10:09 |
| jml | jtv, fwiw, Michael Foord, who was recently hired by Canonical, also has a popular mocking library :) | 10:09 |
| jtv | TMTM | 10:09 |
| jml | something like that :) | 10:09 |
| jtv | jml: why does TestBinaryBuildPackageBehavior.setUp invoke its super, create a factory, and log in? Doesn't TestCaseWithFactory provide all that? | 10:10 |
| jml | jtv, TCWF does indeed provide all of that, but we are subclassing Twisted's TestCase, since we need the Deferred support | 10:11 |
| jml | jtv, and multi inheritance for TestCases is a nightmare. | 10:11 |
| jtv | jml: I only see it being derived from unittest.TestCase. | 10:11 |
| jml | jtv, "from twisted.trial import unittest" | 10:11 |
| jtv | that is obscene | 10:12 |
| jml | jtv, we all make mistakes in our youth that we later come to regret | 10:12 |
| jml | jtv, one of mine was calling the main trial module 'unittest' | 10:12 |
| jtv | jml: was it legal in the country you grew up in? | 10:12 |
| jtv | ah wait, I'm thinking of drugs there. | 10:13 |
| jtv | Different mistake. | 10:13 |
| jml | jtv, yes, but socially frowned upon. | 10:13 |
| jtv | Good, good. | 10:13 |
| jtv | How about, in this special case, importing twisted and qualifying as twisted.trial.unittest.TestCase? | 10:13 |
| jml | jtv, well, we often do "from twisted.trial.unittest import TestCase as TrialTestCase"... | 10:14 |
| jtv | that'd work too | 10:14 |
| jml | jtv, but I didn't do that here because it adds TrialTestCase to the test suite | 10:15 |
| jml | (also my fault) | 10:15 |
| jtv | Can you "from twisted.trial import unittest as twisted_unittest"? | 10:15 |
| jml | jtv, yeah, I'll do something like that. | 10:16 |
| jtv | Thanks. I sorely need a break; brb | 10:16 |
| jml | np | 10:16 |
| jtv | jml: back from break… have you considered taking twisted out of the test altogether? | 10:27 |
| jml | jtv, no. | 10:27 |
| jtv | the obvious follow-up: would it be worth considering? | 10:27 |
| jtv | I did that in one of the slave tests. | 10:28 |
| jml | jtv, ok, sorry, what I meant was that I considered and rejected it... | 10:28 |
| jtv | I forget how, sadly. | 10:28 |
| jtv | Ah :) | 10:28 |
| jml | jtv, we're changing the BuilderSlave to have a Twisted API | 10:28 |
| jml | jtv, so that the buildmaster can be properly written | 10:28 |
| jml | jtv, that API change is going to bubble up and affect all of the *Behavior objects | 10:29 |
| jtv | ah | 10:29 |
| jtv | And, I suppose, make it far too risky to try and hack your way out of twisted just for the purpose of this test. | 10:29 |
| jml | jtv, that's pretty much what's driving this work | 10:29 |
| jml | jtv, indeed. | 10:30 |
| jml | jtv, we *might* be able to do something tricky later on | 10:30 |
| jml | jtv, but until things are stabilized, I'd rather not. | 10:30 |
| jtv | But Knuth's Law. Got it. | 10:30 |
| jml | Yes. That one :) | 10:30 |
| jml | (as an example, lp.codehosting.vfs has code that works with either a synchronous xmlrpc client or an asynchronous one). | 10:31 |
| jtv | In ll. 984—992 of the diff, would it perhaps be clearer to use a list comprehension, perhaps even without the nested function definition? | 10:33 |
| jtv | expected = [ | 10:33 |
| jtv | ['cacheFile', 'sendFileToSlave', ('ensurepresent', url, '', '')] | 10:33 |
| jtv | for url in [chroot.http_url] + extra_urls] | 10:33 |
| jtv | Oh, you're extending, not appending. | 10:34 |
| jml | jtv, I'm open for ideas on making that code clearer. | 10:34 |
| jml | jtv, I had to switch to Haskell mode to be mentally capable of solving the problem | 10:35 |
| jtv | Good start. | 10:35 |
| jtv | jml: personally, I've come off the idea of building these great clever assertion methods in tests. I do like the general approach, but find it better to return something I can compare in a single comparison directly in the test. | 10:38 |
| jtv | Makes failures much easier to deal with. | 10:38 |
| jml | jtv, certainly that would be nice | 10:39 |
| jml | jtv, the current API we're testing is a bit of a blunt instrument. I can't think of anything less bad than the assertion we have now. | 10:39 |
| jtv | jml: I think this might be clearer… http://paste.ubuntu.com/498340/ | 10:41 |
| jml | jtv, you mean, a) change the function to a list comp; b) change the assertion method into a helper that builds the object to compare against? | 10:43 |
| jtv | jml: exactly | 10:43 |
| jml | ok. I'm willing to give that a try. | 10:43 |
| jtv | (Of course not all of the function will fit into a list comprehension, but forming a list would be the obvious core of it) | 10:43 |
| jtv | jml: I'd also extract the dict into a variable, which will break down the structure a bit and also allow the contents to follow our syntax guidelines. | 10:44 |
| jml | how do they break from our syntax guidelines? | 10:45 |
| jtv | syntax guidelines say dictionary = { | 10:45 |
| jtv | 'item1': 1, | 10:45 |
| jtv | 'item2': 2, | 10:46 |
| jtv | } | 10:46 |
| jml | oh right. | 10:46 |
| jtv | ahhh, looking at the calls, I see why you have the assertion method. It must be fun debugging this stuff. | 10:50 |
| jml | jtv, the test failures are relatively easy to grok. | 10:51 |
| jtv | btw I guess twisted tests are expected to return a deferred, or watchumacallit? | 10:51 |
| jml | jtv, but, well, big fat APIs that do a lot of stuff are always hard to test. | 10:51 |
| jml | jtv, yes, they are | 10:51 |
| jml | jtv, full story: if a trial test returns a deferred, trial will wait for that deferred to fire before moving on to the next test. | 10:52 |
| jml | jtv, that facility is necessary when dealing w/ async code | 10:52 |
| jtv | jml: that sort of suggests itself… but makes the naming similarity to unittest all the more unfortunate | 10:52 |
| jml | jtv, in most ways it's quite similar to unittest | 10:53 |
| jtv | It's good to follow the same patterns, but awkward to have a different call model in such a close semblance of unittest | 10:54 |
| jtv | jml: in ll. 1073—1077 of the diff, it may be easier to construct the expected error string outside of the assertion. | 10:56 |
| jtv | I wish these tests could be shorter, but see no very reasonable way to do it. | 10:57 |
| jml | sure. | 10:57 |
| jml | me neither. | 10:57 |
| jml | I'm hoping that as we push deeper into this stuff something will present itself | 10:57 |
| jtv | You could extract the chroot-setting part into a helper. Bit ugly with the commit in the middle (why not transaction.commit, by the way?) but would extract some unhelpful text from the tests. | 10:59 |
| jtv | I suspect the setup of builder and slave could also be extracted. | 11:00 |
| jml | maybe. | 11:02 |
| jml | their construction is parametrized differently in different tests, so I'm a little bit wary of prematurely refactoring. | 11:02 |
| jml | not transaction.commit() because zopeless. | 11:02 |
| jml | maybe that doesn't matter these days | 11:03 |
| jtv | I don't think it does | 11:03 |
| jml | In that case, layer.txn ought to be hidden. | 11:03 |
| jtv | I'm not sure but I think layer.txn == transaction | 11:04 |
| jtv | jml: the archive and the build are parameterized differently, but the builder and slave..? | 11:05 |
| jml | jtv, parametrized by the archive, arch series, virtualized or not. | 11:06 |
| jtv | jml: of course there's nothing stopping a helper from accepting similar pameters | 11:06 |
| jml | yeah. | 11:06 |
| jml | but then when we add a new test we'll probably need to change the helper | 11:07 |
| jtv | it's you call though; the question is how relevant it is to the test | 11:07 |
| jml | I'd like to leave it for now. | 11:07 |
| jml | where are we at more generally w/ the review? bigjools is doing some stuff that I'd like to stick my nose into. | 11:07 |
| jtv | jml: I think we're through most of the code you added; after that I'll make one more brief pass to look for lost coverage but I don't expect to find anything. | 11:08 |
| jml | jtv, cool. thanks. | 11:09 |
| jtv | jml: I suppose the bit in lines 63—99 is really just unnecessarily duplicating Librarian work? | 11:11 |
| jtv | jml: I'm saturated, so not much use for me to look through this further. I'll have to trust that the coverage matches, or if not, that (1) people will be afraid to touch the code anyway or (2) any failures will cause widespread panic and so will be instantly fixed and tested in a modern way. | 11:15 |
| jml | jtv, understood | 11:16 |
| jtv | jml: got a revision for me? I'm EOD'ing as far as all else is concerned. | 11:16 |
| jml | jtv, the 63-99 is already tested in buildmaster/model/tests/test_builder.py | 11:16 |
| jml | jtv, http://pastebin.ubuntu.com/498363/ | 11:16 |
| jtv | thanks | 11:16 |
| jml | jtv, I had to keep the assert helper because the cookie is only created after startJob is called. | 11:17 |
| jtv | jml: understood, I think… still think you've made an improvement there. | 11:18 |
| jml | jtv, thanks. | 11:18 |
| jtv | Brings the interesting part of a failure one level closer to where the reader looks for it. | 11:19 |
| jtv | In the traceback, I mean. | 11:19 |
| jml | yeah. | 11:20 |
| jtv | jml: I've approved. | 11:26 |
| jtv | On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | 11:26 |
| jtv | I meant… | 11:27 |
| === jtv 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 | ||
| jml | jtv, thanks. | 11:34 |
| jtv | np | 11:34 |
| === mrevell is now known as mrevell-lunch | ||
| deryck | adeuring, hi, can I get a review from you? (he says, picking a bugs guy at random.) | 13:37 |
| adeuring | deryck: sure | 13:38 |
| deryck | adeuring, thanks! https://code.edge.launchpad.net/~deryck/launchpad/displayname-private-team-oops-634847/+merge/36199 | 13:38 |
| === sinzui changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === jcsackett changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | MP for me: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738-2/+merge/36242 | 13:48 |
| adeuring | deryck: r=me | 13:54 |
| deryck | adeuring, awesome, thanks! | 13:55 |
| mars | StevenK, still online? | 14:29 |
| StevenK | mars: I am | 14:40 |
| mars | StevenK, heya, did you get to any of the reviews that I left in the queue last night? | 14:41 |
| StevenK | mars: You know, I forgot I was OCR today. :-( | 14:41 |
| mars | StevenK, no problem, everyone does that from time to time | 14:42 |
| mars | StevenK, if you would like, there are two reviews in the channel queue, and one in +activereviews. But I realize it is late there, well past your EOD | 14:42 |
| StevenK | mars: Yup, I don't think I can think straight enough :-) | 14:43 |
| mars | StevenK, next week then. leonardr might be OCR, but we can do the same thing again, and save up some reviews for you. There are usually a few at the end of the day here | 14:44 |
| mars | so it doesn't impact the developers if leonardr and I save them for you | 14:44 |
| sinzui | EdwinGrubbs, ping | 15:15 |
| EdwinGrubbs | sinzui: hi | 15:16 |
| sinzui | EdwinGrubbs, can you review the two sql scripts that Chex and I tested on staging yesterday to clean up deactivated/merged user data? We need another engineer to agree they are sane: http://pastebin.ubuntu.com/498514/ and http://pastebin.ubuntu.com/498518/ | 15:17 |
| sinzui | EdwinGrubbs, Staging ran fine, and the UI looked great after we ran them | 15:18 |
| EdwinGrubbs | sinzui: wouldn't it be a good idea to check whether Person.merged is not null instead of just trusting the name? | 15:32 |
| sinzui | Edwin I think that would be a good idea | 15:33 |
| EdwinGrubbs | sinzui: I don't see any other issues. However, it would be nice if a backup of those tables could be taken just in case some user needs to be reverted however unlikely that is. | 15:45 |
| sinzui | EdwinGrubbs, since we tested on staging. without any issues. I think I should revise the script, maybe combine the deactivated/merged rule so that it is one script. Then test it on staging next week with fresh data | 15:46 |
| EdwinGrubbs | sinzui: will removing the team memberships have an adverse affect on the non-human user ~katie, who is suspended and a celebrity in the code. | 15:52 |
| sinzui | Oh :( I bet it will | 15:53 |
| sinzui | I need to think about that | 15:53 |
| sinzui | Edwin do you think is it safe to remove suspended users from the invited and proposed states, but ignore those that are members? I think it is better to leave some cruft behind and run the parts we trust to work | 15:58 |
| rockstar | abentley, could I get a review of https://code.edge.launchpad.net/~rockstar/launchpad/build-farm-job-constraint/+merge/36219 | 16:03 |
| rockstar | (I was asked to leave it for StevenK so that he could get review experience, but he didn't touch it yesterday) | 16:03 |
| abentley | rockstar, r=me | 16:04 |
| rockstar | abentley, thanks. | 16:06 |
| === gary_poster is now known as gary-lunch | ||
| deryck | adeuring, hi. So a couple minor thoughts about your branch.... | 16:41 |
| deryck | adeuring, do you really need "We’ve" in place of the plain "We've" in HTML? | 16:41 |
| adeuring | deryck: I simply copied that from the regular error page | 16:42 |
| adeuring | but I can change it to a simple "'" | 16:42 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: - || queue: [sinzui, jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| deryck | adeuring, yeah, let's do that. Unless there's some compelling reason to encode that character. | 16:43 |
| deryck | adeuring, and then I tend to prefer using ellipses to hide more of that error mesaage in the page test. In case the template changes it doesn't make the test fragile. | 16:44 |
| deryck | adeuring, but that's up to you. Otherwise, r=me. | 16:44 |
| adeuring | deryck: right using more "..." makes sense | 16:45 |
| adeuring | deryck: and thanks! | 16:45 |
| deryck | adeuring, np | 16:46 |
| deryck | thanks for the fix. | 16:46 |
| === benji is now known as benji-lunch | ||
| === EdwinGrubbs is now known as Edwin-lunch | ||
| === Ursinha is now known as Ursinha-lunch | ||
| === gary-lunch is now known as gary_poster | ||
| === benji-lunch is now known as benji | ||
| === deryck is now known as deryck[lunch] | ||
| === Ursinha-lunch is now known as Ursinha | ||
| === deryck[lunch] is now known as deryck | ||
| === Edwin-lunch is now known as EdwinGrubbs | ||
| leonardr | EdwinGrubbs, who can do a ui review? i haven't needed one since we instituted the process, but i need one now | 19:53 |
| EdwinGrubbs | leonardr: I can do it, but I'm not graduated, so you will have to get two reviews. noodles has graduated, and he is oncall tomorrow morning. | 19:56 |
| leonardr | ok, cool | 19:56 |
| leonardr | i'll get something ready | 19:56 |
| abentley | EdwinGrubbs, could you please review https://code.launchpad.net/~abentley/launchpad/fix-groupby/+merge/36360 ? | 20:13 |
| EdwinGrubbs | abentley: sure, I'll let you jump the queue since the branch is so small. | 20:14 |
| abentley | EdwinGrubbs, thanks. | 20:15 |
| jcsackett | EdwinGrubbs: realized i posted this before you came on call: https://code.edge.launchpad.net/~jcsackett/launchpad/unknown-blueprints-service-597738-2/+merge/36242; it's the MP i already put myself in the queue for, when you have time. | 20:20 |
| EdwinGrubbs | jcsackett: I should get to that in about an hour. | 20:21 |
| jcsackett | EdwinGrubbs: fantastic. thanks. :-) | 20:21 |
| leonardr | edwingrubbs: https://code.edge.launchpad.net/~leonardr/launchpad/rename-grant-permissions/+merge/36363 | 20:44 |
| leonardr | if you can't get to it, no big deal | 20:44 |
| EdwinGrubbs | we'll see | 20:44 |
| EdwinGrubbs | abentley: r=me | 21:02 |
| abentley | EdwinGrubbs, thanks. | 21:02 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: sinzui || queue: [jcsackett, leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| rockstar | abentley, could you take a look at: https://code.edge.launchpad.net/~rockstar/launchpad/recipe-too-new/+merge/36379 | 21:40 |
| abentley | rockstar, you need to do monkeypatches in a try/finally block; otherwise, they can leak. | 21:42 |
| abentley | rockstar, or you could use a with statement. | 21:42 |
| abentley | rockstar, the with statement gives you a little code reuse. | 21:43 |
| rockstar | abentley, so, you're suggesting creating a contextmanager that does the monkey patch? | 21:44 |
| abentley | rockstar, yeah, I think that's the cleanest way. | 21:44 |
| rockstar | abentley, okay. | 21:44 |
| abentley | r=me with that change. | 21:48 |
| EdwinGrubbs | sinzui: review sent | 21:48 |
| rockstar | abentley, where should that context manager live? lp.code.testing doesn't exist, but that makes the most sense. | 21:50 |
| abentley | rockstar, we use lp.code.tests.helpers for that kind of thing. | 21:50 |
| rockstar | Ah, even better. | 21:50 |
| sinzui | thanks edwin | 21:51 |
| rockstar | abentley, a cursory review of http://bazaar.launchpad.net/~rockstar/launchpad/recipe-too-new/revision/11608 please? | 22:07 |
| === Ursinha is now known as Ursinha-afk | ||
| abentley | rockstar, you should do the assignment to RecipeParser.NEWEST_VERSION before the try. The only thing in the try should be the yield. | 22:10 |
| rockstar | abentley, really? Is there some extra logic in the context manager then? | 22:10 |
| abentley | rockstar, in this case, it doesn't matter, because the teardown doesn't require the setup to have succeeded. | 22:10 |
| rockstar | abentley, ah, okay. | 22:10 |
| abentley | rockstar, no. This is true of all try/finally blocks. | 22:10 |
| rockstar | abentley, yeah, I hadn't really thought that through. | 22:11 |
| EdwinGrubbs | jcsackett: r=me | 22:14 |
| jcsackett | EdwinGrubbs: thanks. | 22:14 |
| === salgado is now known as salgado-afk | ||
| EdwinGrubbs | leonardr: review sent | 22:51 |
| === EdwinGrubbs changed the topic of #launchpad-reviews to: On call: EdwinGrubbs || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | edwin, thanks | 22:58 |
| === Ursinha-afk is now known as Ursinha | ||
| === EdwinGrubbs 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!