| wallyworld | https://code.edge.launchpad.net/~wallyworld/launchpad/reassign-reviewer-cancel-option/+merge/36671 | 00:26 |
|---|---|---|
| wallyworld | StevenK: ^^^^^^^^^^^^ | 00:26 |
| wallyworld | thanks ;-) | 00:26 |
| === Ursinha-afk is now known as Ursinha | ||
| === matsubara is now known as matsubara-afk | ||
| poolie | can someone review https://code.edge.launchpad.net/~jameinel/launchpad/lp-service/+merge/35877 | 02:46 |
| poolie | it's a big performance improvement | 02:46 |
| wallyworld | StevenK: ping? | 03:28 |
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| henninge | noodles775: just one little thing: I don't think putting an all-caps CURRENT and ALWAYS in test names names is the right thing to do. I'd just keep the names low key ... ;) | 08:13 |
| henninge | s/names names/method names/ | 08:14 |
| noodles775 | henninge: as in test_blacklist_options_initial_values_CURRENT ? Right - I initiall was reflecting the enum. I've sent that branch to land, but will updated it in the next. | 08:16 |
| noodles775 | s/initiall/initially | 08:16 |
| henninge | noodles775: that's fine, it's not a big deal. | 08:16 |
| === StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: poolie || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| allenap | gmb: Can I foist a trivial review onto you? | 11:09 |
| bigjools | and me please! *2 | 11:11 |
| gmb | allenap, bigjools: Queue 'em up. | 11:11 |
| === allenap changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: poolie || queue: [jtv, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| allenap | gmb: Danke. | 11:11 |
| === bigjools changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: poolie || queue: [jtv, allenap, bigjools*2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | buckets | 11:11 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: jtv || queue: [allenap, bigjools^2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | gmb: did you change your mind about my branch? | 11:26 |
| gmb | jtv: No, but I've had poolie's branch in my queue since Friday | 11:26 |
| gmb | Seemed a good idea to get it out of the way first. | 11:26 |
| jtv | gmb: ironically, he owes me one. | 11:26 |
| jtv | I traded reviews with him, but never got mine. | 11:26 |
| gmb | Shocking behaviour. | 11:27 |
| jtv | (And at that time he couldn't have known how rough I'd be on his, heh heh) | 11:27 |
| gmb | :) | 11:27 |
| gmb | jtv: r=me | 11:32 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: allenap || queue: [bigjools^2] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jtv | gmb: thanks | 11:35 |
| gmb | allenap: r=me | 11:38 |
| allenap | gmb: Cheers :) | 11:38 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: bigjools || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| gmb | bigjools: r=me * 1 | 11:52 |
| bigjools | gmb: thanks - the other one errored on the email MP proposal, I just did another by hand | 11:52 |
| gmb | bigjools: Yep, looking now. So is this one a correction of the stuff that we rammed in on rollout day? | 11:53 |
| gmb | Or am I confusing different 'enabled' fields | 11:53 |
| bigjools | gmb: not exactly but nearly - the rollout day change was just the db change | 11:53 |
| gmb | Ah, yes. Of course. | 11:53 |
| bigjools | I need to tweak the code now | 11:53 |
| gmb | Cool | 11:53 |
| bigjools | if you need help understanding the test let me know | 11:53 |
| gmb | k | 11:54 |
| bigjools | you need to know how arch-all publishing works to appreciate it :) | 11:54 |
| gmb | bigjools: The test makes sense to an extent but for the sake of completeness... how does arch-all publishing work ? :) | 11:55 |
| bigjools | gmb: although the binary is only built in one architecture, it has to be published everywhere | 11:56 |
| bigjools | the code I tweaked calculates the other archtectures for publishing | 11:57 |
| gmb | Right. | 11:58 |
| gmb | bigjools: Pretty much as I thought, then. | 11:58 |
| bigjools | ok cool | 11:58 |
| gmb | bigjools: r=me; looks good | 11:58 |
| bigjools | thanks gmb | 11:58 |
| gmb | np | 11:58 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | ah wgrant is stalking me again | 12:00 |
| wgrant | bigjools: Yeah, sorry. | 12:00 |
| wgrant | I didn't see it was being discussed in here. | 12:01 |
| bigjools | wgrant: I'll fix copying separately | 12:01 |
| wgrant | Hmmm. | 12:01 |
| wgrant | I suppose. | 12:01 |
| bigjools | it could do with refactoring | 12:02 |
| wgrant | I'm also slightly worried about letting new binaries through the publisher unchecked. But I guess that's not a huge problem. | 12:02 |
| bigjools | anyway the urgent case is the upload one | 12:02 |
| bigjools | unchecked? | 12:02 |
| wgrant | I think I'd prefer it if the publisher whinged if it tried to publish binaries in a disabled arch. | 12:03 |
| wgrant | This is messy :( | 12:03 |
| bigjools | what about the case where you want to delete? | 12:04 |
| wgrant | Hm? | 12:04 |
| bigjools | actually, hmmm, getPendingPublications doesn't look for that | 12:05 |
| bigjools | it could whinge but it's not necessary - preventing creating publications is the important bit | 12:05 |
| wgrant | I guess we can fix any screwup there later. | 12:06 |
| wgrant | It's the indices that are the important bit. | 12:06 |
| wgrant | Since they can never change after release. | 12:06 |
| bigjools | yes | 12:06 |
| wgrant | But copies need to be fixed soon, since there'll be SRUs possibly even before release. | 12:07 |
| wgrant | Hrrrrrm. | 12:11 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: vittles || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === mrevell is now known as mrevell-lunch | ||
| === matsubara-afk is now known as matsubara | ||
| === mrevell-lunch is now known as mrevell | ||
| === gmb changed the topic of #launchpad-reviews to: On call: gmb || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === vednis is now known as mars | ||
| === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| bigjools | gmb: can you please review the extra revision I added to the second branch you did for me earlier? | 15:30 |
| bigjools | it adds another check - yay for duplicated code logic. (bug filed BTW) | 15:31 |
| gmb | bigjools: Sure. | 15:31 |
| gmb | bigjools: What's the MP URL again? | 15:31 |
| bigjools | gmb: https://code.edge.launchpad.net/~julian-edwards/launchpad/no-disabled-arch-publications-bug-648715/+merge/36838 | 15:32 |
| bigjools | cheers | 15:32 |
| bigjools | whoever in the Code team added the partial diff attachment processing, you're totally awesome | 15:33 |
| gmb | +1. That's the first time I've seen that. | 15:34 |
| bigjools | it's been there for a while | 15:34 |
| gmb | bigjools: Yeah, but no-one ever does any work round here. | 15:34 |
| bigjools | Lancaster? | 15:34 |
| bigjools | :) | 15:34 |
| gmb | Touche. | 15:35 |
| gmb | bigjools: r=me on the changes. | 15:35 |
| bigjools | gmb: thanks muchly | 15:36 |
| gmb | np | 15:36 |
| abentley | gmb, leonardr: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/incremental-diffs/+merge/36875 ? | 15:52 |
| === abentley changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: - || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | abentley, sure | 15:55 |
| abentley | leonardr: thanks. | 15:55 |
| === leonardr changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-lunch | ||
| jcsackett | gmb or leonardr: time for a review? | 16:22 |
| leonardr | jcsackett, put it in the queue | 16:22 |
| === jcsackett changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: abentley || queue: [jcsackett] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| jcsackett | leonardr: you got it. MP is https://code.edge.launchpad.net/~jcsackett/launchpad/bad-state-transition-641266/+merge/36726 | 16:23 |
| leonardr | jcsackett: and give me the mp url | 16:23 |
| jcsackett | one step ahead of you. :-P | 16:24 |
| leonardr | abentley, all the fields in IIncrementalDiff are exported, but you don't call export_as_webservice_entry(). do you want these objects to be published through the web service? | 16:36 |
| abentley | leonardr: I guess so. | 16:37 |
| abentley | gmb, leonardr: Could you please review https://code.edge.launchpad.net/~abentley/launchpad/incremental-diff-job/+merge/36881 ? | 16:38 |
| leonardr | abentley: try running the webservice tests with your branch as-is and see if you get any failures. i'm curious. | 16:38 |
| gmb | abentley: I'll take a look at that branch for you. | 16:39 |
| abentley | leonardr: How do I do that? | 16:39 |
| abentley | gmb: thanks. | 16:39 |
| gmb | ... after I've looked at jcsackett's. Argh; spent too much time looking at tests and am now all turned about. | 16:39 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: jcsackett, abentley || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === abentley changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: abentley || queue: [jcsackett, abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| === abentley changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: jcsackett, abentley || queue: [abentley] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | abentley: bin/test -vvt webservice | 16:45 |
| leonardr | abentley: even if the existing webservice tests pass, i suspect your branch will break as soon as someone does something to retrieve a diff from the web service, and the diff they get happens to be an incremental diff | 16:50 |
| abentley | leonardr: No, I don't think so. Incremental diffs aren't diffs, and don't have any URL at all. | 16:50 |
| leonardr | abentley: in that case, i don't think you need to call exported() on their fields | 16:51 |
| abentley | leonardr: So either I should go all the way and export incremental diffs over the API, or I should drop the export calls? | 16:53 |
| leonardr | abentley: exactly. export() is you telling lazr.restful which parts of the incremental diffs should be visible | 16:53 |
| abentley | leonardr: Fair enough. I'll probably export them, just because sooner or later, we'll get bug reports if I don't. | 16:54 |
| leonardr | abentley: bug reports saying "export these pls"? | 16:55 |
| abentley | leonardr: yeah. | 16:55 |
| leonardr | abentley: ok, i think you should do that in a separate branch | 16:55 |
| leonardr | since you'd also have to give an incremental diff a url | 16:55 |
| abentley | leonardr: okay. | 16:56 |
| leonardr | abentley, is an incremental diff really a 'thing' with an independent existence? it seems more like the product of an algorithm. that doesn't preclude giving it a url, but it might make the work more complicated | 16:57 |
| abentley | leonardr: You could look at it as merely cached data. | 16:58 |
| abentley | leonardr: Same for any diff, I guess. | 16:58 |
| leonardr | abentley: at this point we're beyond a review of this branch, but if you get into this, maybe we can talk about how existing diffs are published in launchpad-dev? | 16:59 |
| abentley | leonardr: sure. | 17:01 |
| === gmb changed the topic of #launchpad-reviews to: On call: gmb, leonardr || Reviewing: abentley, abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | abentley: can you explain this line? IMasterStore(IncrementalDiff).add(incremental_diff) | 17:02 |
| leonardr | are you caching the diff in the database for later? | 17:02 |
| leonardr | and what's the difference between IMasterStore.of() and Store.of()? it looks like one takes a class and the other an instance? | 17:04 |
| abentley | leonardr: I am storing the value in the database. We cannot generate incremental diffs on demand. We will generate them in a cron job | 17:04 |
| abentley | leonardr: IMasterStore(class) returns the writable store that is meant to be used for that class. Store.of(instance) returns the store the the instance was retrieved from. | 17:05 |
| leonardr | abentley: ok, that does give the incremental diff an independent existence | 17:05 |
| abentley | leonardr: export removed, diff is updated. | 17:11 |
| abentley | leonardr: s/export/expose | 17:12 |
| === jcsackett is now known as jcsackett|afk | ||
| leonardr | abentley, line 292, i think the end quotes need to be on their own line | 17:19 |
| abentley | leonardr: done. | 17:21 |
| leonardr | going through tests now | 17:21 |
| gmb | abentley: r=me on the incremental diff job branch with a few small changes requested. | 17:25 |
| === gmb changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: abentley || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | abentley, i think in test_generateIncrementaldiff there are several multi-line loc you could combine and stay under the character limit | 17:25 |
| === sinzui changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: abentley || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| abentley | gmb: thanks. | 17:25 |
| sinzui | leonardr, I have a branch the refactors our rdf page. | 17:26 |
| leonardr | and i could really use some comments explaining what all these branches have to do with each other. it's tough to figure out just by looking | 17:26 |
| leonardr | sinzui: ok, send me the mp | 17:26 |
| sinzui | https://code.edge.launchpad.net/~sinzui/launchpad/rdf-links-1/+merge/36865 | 17:26 |
| leonardr | abentley: or some more expressive commits--right now all the commits are nearly identical except for one or two single-character differences | 17:27 |
| === matsubara-lunch is now known as matsubara | ||
| leonardr | i think it would help a lot to have a comment explaining the original state of the precursor branch, the state of the prerequisite branch, and then the changes made by the new branch | 17:28 |
| abentley | leonardr: I was only able fix one multi-line loc. | 17:28 |
| leonardr | abentley: ok, i must have misjudged. what do you think of the comment idea? | 17:28 |
| abentley | leonardr: Sure, I can do that. | 17:29 |
| leonardr | a way to make it simpler without comments would be to have a method that both creates a branch and does a commit | 17:29 |
| abentley | leonardr: http://pastebin.ubuntu.com/502197/ | 17:36 |
| abentley | leonardr: half of those commits are merges, though. | 17:37 |
| leonardr | abentley: that makes it a lot clearer | 17:39 |
| leonardr | i don't know if the method that creates branch and does commit would help much, but it might help distinguish between the branches as they were "before", and the branches that get changed "during" the test | 17:40 |
| leonardr | abentley: a couple nosy questions about difftacular, which i see you wrote yourself during july (?) | 17:42 |
| leonardr | how hard would it be to make an egg of it? i get the impression we prefer eggs to sourcedeps | 17:42 |
| leonardr | granted that you have tests in difftacular, but did you get someone to review that code? | 17:43 |
| abentley | leonardr: impossible. bzr plugins cannot be provided as eggs. | 17:43 |
| leonardr | ok | 17:44 |
| abentley | leonardr: I did not get someone to review that code. | 17:45 |
| abentley | leonardr: I'm sorry, but I need to go now to meet someone for lunch. | 17:45 |
| === abentley is now known as abentley-lunch | ||
| leonardr | abentley: no problem. i'll approve this mp with your changes, contigent on getting difftacular reviewed. i can probably review it today if it's not too complicated, since i need to leave some mps for a trainee who's ocr tomorrow | 17:48 |
| === leonardr changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: sinzui || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews | ||
| leonardr | sinzui, just out of curiosity, have people been complaining about the rdf and the owl? | 18:21 |
| sinzui | leonardr, none directly...they cannot find it. We have circuitous complaints that RDF is not documented | 18:22 |
| sinzui | oh, Google knows about it | 18:23 |
| sinzui | bots don't complain though, they just 404 | 18:23 |
| leonardr | sinzui, obvious typo: "it's" in line 204 | 18:40 |
| * sinzui can fix that | 18:40 | |
| leonardr | sinzui: it looks like there was a problem where both the rdf spec and the link to it were called "rdf". was that the actual source of the problem (like a conflict) or was it just something that looked confusing so you fixed it? | 18:43 |
| EdwinGrubbs | abentley-lunch: I didn't get the job runner cron script ready in time for your OCR day. Do you have time to review it now, or do you know someone else who is familiar enough to handle it? | 18:44 |
| sinzui | leonardr, that was part of the problem. browser resource loses the mimetype. lazr folder fixes that | 18:44 |
| === benji is now known as benji-afk | ||
| sinzui | leonardr, I think it was first broken because the director or zcml moved and the publisher could not find the dir. | 18:45 |
| leonardr | sinzui: i see | 18:45 |
| leonardr | ok, r=me | 18:45 |
| === jcsackett|afk is now known as jcsackett | ||
| abentley-lunch | EdwinGrubbs: I can look at it now. | 19:51 |
| === 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 | ||
| leonardr | abentley-lunch: when you have time, i've looked through the difftacular codebase and i have some questions at various granularities | 19:52 |
| === abentley-lunch is now known as abentley | ||
| abentley | leonardr: sure. | 19:54 |
| leonardr | abentley: my biggest problem is that i don't understand what ignore_heads does | 19:54 |
| leonardr | it looks like it takes one branch, and a bunch of other branches, and returns a bunch of revision ids in the first branch that can be ignored? | 19:55 |
| abentley | leonardr: it's the changes introduced by those revisions that can must be ignored. | 19:57 |
| leonardr | abentley: how about iter_lca_revision? i don't know what an lca is, but i'm guessing this is what strips out the revisions to be ignored? | 19:58 |
| abentley | leonardr: an LCA is a Least Common Ancestor. It is an ancestor shared by both revisions that has no descendants that are also ancestors of the revisions. | 20:00 |
| leonardr | so the revisions included in the diff are the lcas of the original revision and all the revisions being ignored? | 20:01 |
| abentley | leonardr: we don't really diff revisions. We generate two trees and diff those. | 20:03 |
| abentley | The "new" tree is the tree for the "new_revision". | 20:03 |
| abentley | The "old" tree is generated by merging the changes from the ignore_branches that were merged into the new_revision into the old_revision as well. | 20:04 |
| EdwinGrubbs | abentley: thanks, here it is: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-jobqueue-cron-script/+merge/36910 | 20:04 |
| leonardr | ok, that's similar to what i was trying to say | 20:04 |
| leonardr | let me take another look at the tests now that i understand ignore_heads | 20:04 |
| abentley | EdwinGrubbs: I am looking into it, but I am also multitasking pretty hard. | 20:05 |
| EdwinGrubbs | abentley: no problem, I need to get some lunch anyway. | 20:05 |
| leonardr | abentley: why does ignore_heads return an empty list in test_ignore_heads_already_merged? is it because the merge happened in rev2, not between rev2 and rev3? | 20:13 |
| abentley | leonardr: it is because those branches haven't been merged into bar. | 20:18 |
| leonardr | oh, they were merged into foo and you checked bar | 20:19 |
| leonardr | no, i'm confused again. 'repo' is foo.branch.repository | 20:19 |
| leonardr | you're generating a diff between foo rev2 and foo rev3 | 20:19 |
| leonardr | and you're ignoring bar | 20:19 |
| leonardr | but bar was merged into foo during rev2, so it doesn't show up | 20:20 |
| leonardr | that makes sense to me, but it's not what you're testing? | 20:20 |
| abentley | leonardr: sorry, misread it. | 20:21 |
| abentley | leonardr: yes, your original idea was right. | 20:21 |
| leonardr | ok | 20:21 |
| leonardr | abentley: hopefully the final question before the suggestions | 20:22 |
| leonardr | what exactly is going on in the unignored merge scenario? | 20:23 |
| leonardr | i suspect it's not unignored on its own, but that you use it in a way where bar.branch is not ignored where it would otherwise be ignored | 20:23 |
| leonardr | or, you create a new branch, merge it into foo, and its changes are picked up while bar's changes are ignored | 20:25 |
| abentley | leonardr: I don't understand what "unignored on its own" would mean. Everything depends on how you invoke the diff generation. | 20:27 |
| leonardr | abentley: i don't think "unignored on its own" has any meaning, but "create_unignored_merge_scenario" implies that it does | 20:28 |
| leonardr | i'm trying to figure out what is special about the scenarios where you call that functino. such that the "unignored" branch is not ignored | 20:29 |
| abentley | leonardr: I think it means that the merge would not be ignored if you did a normal diff. | 20:30 |
| leonardr | abentley: in the two tests where you use it, the merge is *not* ignored. you ignore bar instead, and the "d" line shows up in the diff | 20:31 |
| leonardr | which makes sense, given the name. i'm just not sure whether that's what you're testing | 20:32 |
| abentley | leonardr: I'm testing that when the branch isn't in the list of branches to ignore merges from, its merges aren't ignored. | 20:33 |
| abentley | leonardr: unlike mainline_diff, which will ignore such merges. | 20:34 |
| leonardr | ok, i get it now, but it's confusing that the tests are called "ignore_x" and you're checking that only *certain* x are ignored | 20:34 |
| leonardr | abentley: for lack of a better place to put these comments, i'm going to append them to my review of your launchpad branch | 20:38 |
| abentley | leonardr: okay. | 20:39 |
| leonardr | abentley: i've approved your launchpad branch. let me know when you do a difftastic follow-up and i'll review that | 20:51 |
| === 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 | ||
| abentley | leonardr: the preview tree must be used as the input to the next merge. Otherwise, you're not doing proper merges. | 20:57 |
| leonardr | abentley: ok, i missed that you were reassigning tree | 20:59 |
| abentley | EdwinGrubbs: "for child in children: child.wait()": doesn't this mean that you can't re-run a job source until all job sources have terminated? | 21:08 |
| abentley | EdwinGrubbs: I was thinking that we would exit after all the JobSources had been forked, not after they all completed. | 21:12 |
| EdwinGrubbs | abentley: I guess it does. The problem I was having is that if the parent process doesn't wait for the children to exit, when you run it manually, the child processes continue to spew output to the terminal. One solution to this problem would be to not have the parent process use a lockfile, since the child processes already have their own lockfiles. | 21:12 |
| abentley | EdwinGrubbs: All the output should be redirected to logfiles anyhow. | 21:12 |
| abentley | EdwinGrubbs: Waiting doesn't prevent the children from spewing output to the terminal, it just means they won't do it while you're using the terminal for something else. Which isn't really a risk for cron scripts. | 21:15 |
| EdwinGrubbs | abentley: To prevent it from becoming more complicated to gather the output in the tests, I think I should add a command-line option so the tests can force it to wait, but I'll try running the tests without the waiting to see if that works at all. | 21:16 |
| EdwinGrubbs | I meant "spewing" to indicate that the output was coming at an annoying time. I actually wanted the children's output, but I just didn't want to be surprised by it. | 21:19 |
| sinzui | bac: those stories do not ready like a story. should we be concerned that foo bar is being called an owner in a story? | 21:20 |
| * sinzui thinks stories need an owner/contributor browser and that is Sample Person | 21:20 | |
| bac | sinzui: yes, it required more set up | 21:20 |
| sinzui | who owns applets? | 21:21 |
| bac | sinzui: i welcome ideas on how to make it looks less mechanical | 21:21 |
| bac | sinzui: either foobar or sample | 21:21 |
| abentley | EdwinGrubbs: Why do you default runner_class to JobRunner when an unknown runner class is specified? | 21:21 |
| sinzui | The code team have been turning their mechanical tests into unittests that still use testbrowser | 21:21 |
| bac | sinzui: foobar owns applet | 21:22 |
| * sinzui bangs head on a wall | 21:22 | |
| sinzui | The whole test is using a god to manage an upstream project | 21:23 |
| bac | sinzui: i didn't intend for this to spiral. i made requested UI changes and broke the world. and then backfilled. | 21:23 |
| * bac bad | 21:23 | |
| sinzui | bac: right. I am thinking how do we stop hurting ourselves with this. | 21:23 |
| abentley | EdwinGrubbs: What about phrasing it like this? http://pastebin.ubuntu.com/502299/ | 21:24 |
| sinzui | bac: did you check for test duplication? maybe we can delete instead of add to make it pass | 21:24 |
| EdwinGrubbs | abentley: oh, that was left over from the old solution. It handled defaulting to JobRunner if it didn't exist, but getattr() wouldn't default to JobRunner if it was set to null. Although, it's unlikely that someone will specify the field without filling it in with something valid, so I could probably go back to just using getattr() and let it blow up later if the config is invalid. | 21:24 |
| bac | sinzui: no, i didn't look carefully for duplicates. being unfamiliar with the code tests i'm thinking that would take a while | 21:25 |
| * sinzui takes a quick look | 21:25 | |
| EdwinGrubbs | abentley: that's a nice rewrite. I'll use that. | 21:25 |
| abentley | EdwinGrubbs: I think it's better if it blows up the value is invalid. | 21:25 |
| sinzui | bac: code/browser/tests does not appear to intersect | 21:26 |
| sinzui | oh, the list is nice | 21:27 |
| bac | what list? | 21:27 |
| bac | sinzui: ^ | 21:29 |
| sinzui | bac: I like the listification of the hosting options | 21:29 |
| abentley | EdwinGrubbs: Do you need NoErrorOptionParser? Can't you just blow up in ProcessJobSource.__init__? | 21:29 |
| bac | sinzui: yes, +1 to mrevell for the suggestion | 21:30 |
| sinzui | bac: I have been thinking about privacy too long. I keep seeing context/displayname and thinking that will raise a 403 | 21:30 |
| EdwinGrubbs | abentley: the problem is if I want to pass "-v" into ProcessJobSource, the OptionParser in the __main__ section needs to ignore it. | 21:32 |
| sinzui | bac: you cleaned up a generator, but 'v' is does not conform to lp standards | 21:32 |
| * sinzui argued for 1-char vars in comprehensions | 21:33 | |
| bac | sinzui: i'll join you in that argument | 21:33 |
| bac | but for now i'll fix it | 21:33 |
| abentley | EdwinGrubbs: But if you do the argument parsing in ProcessJobSource.__init__, you'll have the real parser available to you. | 21:33 |
| bac | sinzui: and then have to split the line, resulting in it being more unreadable | 21:34 |
| sinzui | bac: branch-listing.pt may render faster if we used "sprite branch" instead to using an <img> element 75 times | 21:35 |
| * sinzui was confused | 21:35 | |
| bac | sinzui: IRC hide-n-seek? | 21:35 |
| EdwinGrubbs | abentley: yes, but I can't get the real parser until after I parse the JOB_SOURCE name out of the command line arguments, since that is necessary for looking in the configs to find out which runner class to pass to ProcessJobSource.__init__(). | 21:35 |
| sinzui | maverick has enabled tap on my magic mouse :( | 21:35 |
| abentley | EdwinGrubbs: You mean which job source class to pass to ProcessJobSource.__init__() ? | 21:36 |
| sinzui | Bac: this paste looks good. I do not see what can be done about the tests. Given the age and value of the branch, I think it is ready to land. | 21:36 |
| EdwinGrubbs | abentley: sorry, I mean that ProcessJobSource.__init__() would be looking up the runner class, which it would be passing to JobCronScript.__init__() | 21:37 |
| bac | sinzui: i've made sprite change and the generator change. thanks. | 21:37 |
| abentley | EdwinGrubbs: So I think the simplest thing is to override runner_class after you've done JobCronScript.__init__() | 21:40 |
| sinzui | bac: you have my r for code. | 21:43 |
| bac | sinzui: and UI? | 21:43 |
| sinzui | That too, am I stepping on salgado though? | 21:44 |
| bac | sinzui: i'll send this off to ec2 for testing and wait for salgado to reply. | 21:45 |
| sinzui | bac: yes please | 21:45 |
| mwhudson | salgado is not working today, i think, if that's relevant | 21:45 |
| EdwinGrubbs | abentley: ok, it looks like I just have to override config_name and dbuser also, but init_zca doesn't get called in the __init__(), so I can get rid of the extra parser. | 21:46 |
| bac | mwhudson: will he be around tomorrow? | 21:46 |
| mwhudson | bac: hopefully | 21:47 |
| abentley | EdwinGrubbs: have you considered providing a list of crontab_groups in [process-job-source-groups] and then providing configuration for each group? | 21:48 |
| abentley | EdwinGrubbs: I think then you wouldn't need all_job_sources or grouped_sources. | 21:49 |
| EdwinGrubbs | abentley: my main concern then is that a job class could become a member of multiple groups. Even if the cronscript would spit out an error when that happened, it would make the config file less convenient to use, and the config file will be edited a lot more than the cronscript's code. | 21:52 |
| abentley | EdwinGrubbs: fair enough. | 21:53 |
| abentley | EdwinGrubbs: could you please add documentation to your tests? Each test is supposed to have at least one line. | 21:55 |
| === lifeless_ is now known as lifeless | ||
| abentley | EdwinGrubbs: is there a reason why test_setstatus_silent and test_setstatus_admin differ by more than the silent=True parameter? | 22:00 |
| EdwinGrubbs | abentley: ugh, another problem with not passing in the job_source to __init__() so that we can re-use its option parser is that the job_source is used to determine the config_name, and the config_name is passed to LaunchpadCronScript.__init__() as the name it uses for the cron control file, which determines whether the script runs at all. | 22:00 |
| abentley | EdwinGrubbs: so my meta-suggestion is "if the way things fit together is getting in our way, let's fix it instead of routing around it or doing double handling". | 22:02 |
| EdwinGrubbs | abentley: I'll add some comments to the tests. setStatus(silent) can only be done by a Launchpad admin, so I made extra changes for that restriction after I had already finished test_setstatus_admin(), which just refers to the team admin. | 22:03 |
| abentley | EdwinGrubbs: since you're doing admin = person_set.getByEmail('admin@canonical.com') you can use login_person instead of login, if you like. | 22:04 |
| lifeless | also, if I can suggest | 22:05 |
| lifeless | from lp.testing.sampledata import ADMIN_EMAIL | 22:05 |
| abentley | EdwinGrubbs: Have you tried using canonical.launchpad.scripts.tests.run_script? | 22:07 |
| EdwinGrubbs | abentley: no, I just copied the first test for a cronjob that I found. | 22:09 |
| abentley | EdwinGrubbs: Perhaps I should have gone back and fixed all the tests to use that when I wrote it, but I was not very aggressive back then. | 22:10 |
| EdwinGrubbs | abentley: regarding your meta-suggestion, I think it will all work better if I move all the setup logic from __init__() to run() in all the parent classes. The only downside is that it will create the lockfile briefly before it exits if the cron crontrol file tells it not to run. | 22:11 |
| EdwinGrubbs | abentley: I'll switch to run_script. | 22:12 |
| abentley | EdwinGrubbs: that sounds reasonable. | 22:14 |
| abentley | EdwinGrubbs: (moving the setup logic to run) sounds reasonable. | 22:14 |
| abentley | EdwinGrubbs: It's past EOD for me. Can we pick this up tomorrow? | 22:16 |
| EdwinGrubbs | abentley: sure, talk to you later | 22:17 |
| abentley | EdwinGrubbs: cool. | 22:17 |
| === salgado is now known as salgado-afk | ||
| === matsubara is now known as matsubara-afk | ||
| === Ursinha is now known as Ursinha-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!