/srv/irclogs.ubuntu.com/2010/09/16/#launchpad-reviews.txt

lifelessjml: your oops has come up : https://lp-oops.canonical.com/oops.py/?oopsid=OOPS-1719ED56900:34
lifelessjml: that doesn't look like you to me.00:34
=== EdwinGrubbs changed the topic of #launchpad-reviews to: On call: - || Reviewing: jelmer || queue: [allenap, lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
EdwinGrubbslifeless: If you have time, I have a branch that needs a db review. https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-615654-registry-jobqueue-schema/+merge/3561902:25
StevenKEdwinGrubbs: Hi! Can you please look at the MP you reviewed for me last night?02:27
StevenKEdwinGrubbs: I agree with your changes, I've applied and tested them.02:28
lifelessEdwinGrubbs: hi, stub needs to review it02:28
lifelessEdwinGrubbs: the policy is 'request from both of us so we both know' and 'stub does it unless hes on leave'02:29
EdwinGrubbsStevenK: I'll look at it now.02:29
EdwinGrubbsStevenK: that looks good. r=me02:32
StevenKEdwinGrubbs: Thanks!02:32
EdwinGrubbslifeless: ok, thanks02:33
lifelessEdwinGrubbs: you did raise an interesting question though and I've commented there02:34
lifelessEdwinGrubbs: I was talking with deryck this morning about having a single deferred email dispatcher02:34
lifelessthat would jsonify-or-so *all* the emails an appserver wants to send, shove that into one job, and then it gets committed as part of the transaction02:35
lifelessand a Job would process those for the whole system after02:35
StevenKSomeone around who can review a small branch?02:41
StevenK1 small code change, mostly just lint02:41
lifelessyes02:44
StevenKlifeless: https://code.edge.launchpad.net/~stevenk/launchpad/db-clean-up-lint-idsjob/+merge/3562102:45
StevenKlifeless: Sorry to do this, but I'm about to head out, and I'd like that branch on ec2 while I'm out, can you push it up your stack, please?03:25
lifelessStevenK: I did it already03:32
StevenKlifeless: Sorry, I'm a muppet.03:36
lifelessyes03:36
lifeless:P03:36
=== StevenK changed the topic of #launchpad-reviews to: On call: - || Reviewing: jelmer || queue: [allenap, lifeless, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jelmer || queue: [allenap, lifeless, StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jelmerresolved 7 conflicted files.. posting the diff now10:12
noodles775Thanks jelmer10:13
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775 || Reviewing: jelmer || queue: [allenap, lifeless, StevenK, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775henninge: I've got another MP needing UI thought if you've time (but no rush): https://code.edge.launchpad.net/~michael.nelson/launchpad/635005-difference-details-2/+merge/3564010:58
henningenoodles775: wow, with screencast?10:59
henningenoodles775: you rock!11:00
noodles775henninge: well, I try to make it easy to understand (hard to describe in text) :)11:00
henningenoodles775: really great! Didn't you describe somewhere how you do these? Or did someone else?11:01
noodles775henninge: I did put something on the wiki at some point... it's just gtk-recordmydesktop, thank them :)11:02
henningenoodles775: I would not expect to have a standard for comments with logos because we have never had any of those.11:03
henningenoodles775: on first view they struck me as taking up a lot of space.11:04
henningenoodles775: my take: we should not have two types of comments "with" and "without" logos.11:04
noodles775henninge: yeah, I noted that in the MP, saying I should probably just do the same as mp/bug comments. The space is because the person logo (ie. it would be there photo), only comes in the 64x64 size.11:04
noodles775s/there/their11:05
* henninge did not read ... just watched in awe .. ;)11:05
* henninge reads now11:05
henningenoodles775: yes, I think the comments should look like other comments we have elsewhere.11:07
henningeor we should commit to changing all comments to include headshots ... ;)11:07
noodles775yep, agreed.11:07
henningenoodles775: that multiline comment - does it have double new lines or why is the line spacing so big?11:08
noodles775henninge: the formatter being applied treats double new-lines in text as paragraph boundaries... I think I need to ensure paragraphs have less margin in those comments.11:09
henningenoodles775: It should be treated by the css for comments, like on any other comment.11:11
henningenoodles775: Meaning: I don't think you need to worry about that much.11:11
noodles775henninge: yes, that's what I meant (ie. I need to check...)11:11
noodles775henninge: are you running maverick?11:12
henningeyes11:12
noodles775Can you please just run one test for db-devel and see if you get the error I mention on the MP?11:13
noodles775I've just tried the following with r9787 of db-devel (which went through buildbot recently), but always get the error linked on the MP.11:14
noodles775bin/test -vv -m test_archive -t test_getPublications_returns_all_published_publications11:14
noodles775(just a random test).11:14
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, - || queue: [allenap, lifeless, StevenK, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775(well, one that uses the DB).11:14
allenapGood morning noodles775 :)11:14
noodles775Hi allenap! Welcome back!11:14
henningenoodles775: you need to downgrade psycopg11:14
allenapnoodles775: Cheers :) Right, on to lifeless's branch.11:15
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, lifeless || queue: [allenap, StevenK, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775henninge: ah, I do remember glancing over something with that info... thanks.11:15
henningenoodles775: download from here https://edge.launchpad.net/ubuntu/lucid/i386/python-psycopg2/2.0.13-2ubuntu211:16
henninge(edge css is broken again)11:16
henningenoodles775: then do "dpkg -i --force-downgrade" with it.11:16
noodles775Thanks.11:16
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, StevenK || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
allenapGah, not doing very well this morning.11:21
noodles775Oh? I just thought you were super fast :)11:21
allenapnoodles775: Which of jelmer's branches are you reviewing? I'll do the other.11:21
noodles775allenap: sorry, this one: https://code.edge.launchpad.net/~jelmer/launchpad/506256-remove-popen-2/+merge/3541211:22
noodles775I've just claimed to avoid more confusion :)11:22
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, jelmer || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
henningenoodles775: I don't understand what the "Pacakge differences" section is telling me.11:23
noodles775henninge: yes - it may need more explanation. So when there are different versions of a package in the two distro series...11:24
noodles775there will be a base version (ie. the last common version before they diverged)11:24
noodles775Those differences will be available on request (or generated automatically - still need to do that), to show the difference between the base version and each distro series version.11:25
henningenoodles775: Ah, I see. Those will be clickable and expand, too?11:26
noodles775henninge: no, that wasn't planned, but could be nice.11:26
henningenoodles775: isn't the "last common version" information missing?11:26
henningenoodles775: but they will be linkified to an extra page?11:27
noodles775henninge: yes, it says "unimplemented" in the demo (as I need to add a db column for it).11:27
* henninge is blind11:27
noodles775And yes, the diffs will link to librarian files.11:27
henningenoodles775: and it will always be those two link: "base to parent" and "base to derived" ?11:28
noodles775henninge: sometimes just one (ie. when a package hasn't been updated in the parent series, but only the derived, it's no point generating the base-parent diff)11:30
henningenoodles775: so, because they are downloads AFAIUI, they should be preceeded by the download logo.11:36
noodles775They don't work as downloads on other pages... let me check.11:37
noodles775(not that that means they shouldn't)11:37
henningenoodles775: links to librarian?11:38
noodles775links to text files on the librarian.11:38
henningethat depends on your browser settings, I think11:38
noodles775henninge: right, and it's a .gz anyway...11:39
noodles775So currently PackageDiffs aren't marked up as download links, but that would be great to change. +111:39
henningenoodles775: maybe call the section "Package differences from base version:"11:40
henningenoodles775: oh, where do I normally see them?11:40
* henninge has not worked with those before11:40
noodles785*sigh*11:44
noodles785Last thing I said was:Expand 'hello' and you'll see a link for an available diff.11:44
noodles785(for the link https://edge.launchpad.net/~michael.nelson/+archive/pocketsphinx/+packages?field.name_filter=&field.status_filter=&field.series_filter= )11:45
=== noodles785 is now known as noodles775
henningenoodles775: yes, those should have the download icon, too.11:48
henningenoodles775: I also wonder if a little indention sound be fitting because it is a list (mostly two entries).11:48
noodles775jelmer: You've added some delete perms to the uploader (now that you're removing the buildqueue entries earlier), but does that mean we can remove DELETE perms elsewhere?11:48
noodles775henninge: yes on both counts.11:49
jelmernoodles775: That's a good point, let me check11:49
henningeI don't know how "sound" got in there ... ;)11:49
henningeI probably meant "would" ...11:50
noodles775Yeah, I assumed you meant 'could' :)11:50
noodles775jelmer: also, I guess you tried store.flush() before you ended up with the txn.commit()?11:52
noodles775(or you know why that wouldn't work :) ).11:52
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: jelmer, lunch || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
jelmernoodles775: Yeah, I did - although not for all instances individually.11:59
henningenoodles775: reviewed, needs-fixing as discussed.12:05
noodles775Great, thanks henninge, I'll finish the windmill test too before requesting a re-review.12:06
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: lunch, lunch || queue: [allenap, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775allenap: did you review StevenK's branch? I can't see one on the active reviews page either, but I did think he mentioned he had one. StevenK ?12:24
noodles775jelmer: review sent... great work, just a few questions.12:25
jelmernoodles775: Thanks12:27
StevenKnoodles775: Sorry, had the silly thing as WIP12:29
StevenKnoodles775: https://code.edge.launchpad.net/~stevenk/launchpad/db-add-derivedistroseries-api/+merge/3550012:30
=== mrevell is now known as mrevell-lunch
jelmerallenap: Thanks for the review13:04
=== matsubara-afk is now known as matsubara
allenapjelmer: You're welcome :)13:16
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: lunch, noodles775 || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: lunch, StevenK || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: lunch, wallyworld || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== noodles775 changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: allenap, wallyworld || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== allenap changed the topic of #launchpad-reviews to: On call: noodles775, allenap || Reviewing: allenap, StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
noodles775allenap: what order of subscriptions are returned by getSubscriptionsFromDuplicates()?13:42
* allenap looks13:43
noodles775sorry, I meant how many subscriptions - and whether it's currently a method with performance issues or not.13:44
allenapnoodles775: I don't know if it has performance problems, but the code certainly appears sub-optimal. It should return one subscription per subscriber. Each subscriber could have more than one subscription so I've ensured that the earliest is always returned. I /think/ it was undefined before.13:46
noodles775allenap: why is it valid for a person to have multiple (current/valid) subscriptions to the same bug? Or is it just because it can also be via teamparticipation?13:54
noodles775Actually, looking at that query, it's only subscriptions for the person directly, so why can they have more than one?13:55
allenapnoodles775: It's because the subscriptions are to duplicates. There could be several duplicates for a bug and one person could be subscriber to more than one.13:55
noodles775Right - just spotted that... sorry for the noise.13:55
allenapnoodles775: It's not noise! :) It's quicker to ask than to pore over code. Although I regularly pore over code when I could ask, ahem.13:58
allenapnoodles775: http://paste.ubuntu.com/494739/ <-- a small optimization to that branch. Do you mind if I smuggle that in?13:59
noodles775'course not :)13:59
allenapnoodles775: Thanks :)14:01
=== Ursinha-afk is now known as Ursinha
jelmernoodles775: I've done the checks you mentioned your the review, and managed to avoid the remoteSecurityProxy call. However, I'd like to make an additional change - flushing the store of build in packagebuild to avoid potential race conditions between buildmaster and the upload processor.14:07
jelmerDoes that seem reasonable?14:07
noodles775jelmer: sure.14:08
noodles775allenap: OK, done. I added a thought for possibly doing that query all in the db... but haven't checked to see if it would work.14:15
allenapnoodles775: I'm interested.14:15
noodles775jelmer: have you/can you push an incremental?14:18
jelmernoodles775: ok14:18
jelmernoodles775: updated diff posted14:43
noodles775Thanks jelmer14:43
=== mrevell-lunch is now known as mrevell
=== benji____ is now known as benji
jelmernoodles775: I haven't looked at splitting up those two tests yet.14:48
noodles775jelmer: that's fine - it was just a thought, I didn't expect you to necessarily do it unless you were really keen :)14:49
allenapnoodles775: Thanks for the query suggestion. Does this look sane to you? http://paste.ubuntu.com/494766/14:56
noodles775allenap: yeah, great stuff! (assuming it works?)14:57
allenapnoodles775: The tests pass ;)14:57
noodles775Wonderful :)14:58
allenapnoodles775: To be honest, I'm not sure that there's sufficient coverage on this method, so I'm going to add another test.14:58
noodles775allenap: ensuring that the bug subscription is indeed the first one, or something else?14:59
allenapnoodles775: Yes. I didn't look closely before, but now I find that the only direct test I can find for getSubscriptionsFromDuplicates is right at the end of a doctest, and it doesn't make me feel awfully comfortable.15:01
noodles775Sounds excellent (and you could even remove the doctest example if it's not relevant documentation-wise?)15:06
noodles775jelmer: did you mean to paste the whole diff in that comment, or just the incremental? (the MP will update the diff when you push your branch right?). I'm trying to get an incremental but it looks like you haven't pushed your branch? (last rev. is 11106 20hrs ago).15:09
jelmernoodles775: http://pastebin.ubuntu.com/494772/15:13
noodles775jelmer: looks fine. Were those tests failing previously? (the one you've updated there that assumed the upload log would be present, but now asserts that it is not)?15:17
jelmernoodles775: I indented build.storeUploadLog(), which changed the behaviour slightly.15:17
jelmernoodles775: So we're not storing the upload log when the upload succeeded, consistent with previous behaviour (see the discussin in #launchpad-dev earlier for background)15:17
noodles775Yeah, I saw parts of it flash by :)15:21
=== noodles775 changed the topic of #launchpad-reviews to: On call: allenap || Reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha is now known as Ursinha-lunch
=== salgado is now known as salgado-lunch
leonardrallenap, i have a slightly oversized branch coming towards you16:42
allenapleonardr: I might not have time to do it today :-/16:57
leonardrallenap: ok, np16:57
leonardrtake a look once i get the mp up16:57
allenapCool.16:57
=== benji is now known as benji-lunch
=== matsubara is now known as matsubara-lunch
=== Ursinha-lunch is now known as Ursinha
bdmurrayIs there somebody I should ping about my UI review for https://code.edge.launchpad.net/~brian-murray/launchpad/limited-subscriptions-page/+merge/35177 ?17:38
=== deryck is now known as deryck[lunch]
=== salgado-lunch is now known as salgado
=== benji-lunch is now known as benji
bdmurraysalgado: is there a queue for ui reviews?18:15
salgadobdmurray, not that I know of18:17
bdmurraysalgado: Then I just ping a UI reviewer?18:19
=== matsubara-lunch is now known as matsubara
salgadobdmurray, yep, that's how people have been telling me about ui reviews18:26
bdmurraysalgado: ping ;-) https://code.edge.launchpad.net/~brian-murray/launchpad/limited-subscriptions-page/+merge/3517718:26
salgadobdmurray, heh, ok, I should be able to review it later today, but can't promise because I need to wrap up other stuff and I'm kinda slow because I've got one arm in a sling18:32
leonardrwell, allenap can't take it, but maybe someone else can look at https://code.edge.launchpad.net/~leonardr/launchpad/accept-oauth-signatures/+merge/35697 ?18:36
leonardri'd really like salgado to look at it but i don't want to put more work on his plate18:36
leonardrbdmurray, is your branch something i can look at?18:36
bdmurrayleonardr: If you can do UI reviews sure.18:40
leonardrbdmurray: sorry, not rated for ui reviews18:41
bdmurraysalgado: sorry about your arm - its no hurry I just wanted to get in some queue18:43
=== deryck[lunch] is now known as deryck
jelmerHi StevenK19:43
jelmerStevenK: Can I add a simple soyuz branch to your queue?19:48
lifelessjelmer: I think you mean allenap19:53
jelmerlifeless: You're right, thanks19:54
jelmerI think allenap has probably signed off by now though :-(19:54
jelmerallenap: Still there?19:56
lifelessjelmer: link the branch earlier ;)19:56
jelmerlifeless: :-) It's at https://code.edge.launchpad.net/~jelmer/launchpad/635591-sync-source-unicode/+merge/3571520:00
lifelessjelmer: your description references the same bug, not an older one?20:02
jelmerlifeless, it's a fix for an issue introduced by the fix for bug 635591, and found while I was QA'ing that.20:03
lifelessbut it appears to be the same branch20:03
lifelesswith everything the same20:03
lifelessit says its the fix for itself20:03
jelmerThe last revision is new20:03
lifelessthere's only one revision there20:03
lifelessunmerged that is20:04
jelmerYes, the original fix has already landed on devel.20:04
lifelessso where is the original MP?20:04
lifelesslook at this with fresh eyes20:05
lifelesswhere is the qa-bad tagged bug20:05
lifelessso that we know not to rollout the thing that failed qa20:05
jelmerhttps://code.edge.launchpad.net/~jelmer/launchpad/635591-sync-source-unicode/+merge/3563720:05
lifelessand why isn't that MP linked to the branch anymore ?20:06
lifelessah someone rebased.20:06
lifeless?20:06
jelmerI'm not sure why it's no longer linked.20:07
leonardrsinzui or mars, do you have any interest in doing an interactive review?20:07
jelmerlifeless: I've se the qa-bad tag.20:08
sinzuileonardr, I will have time in an hour20:08
lifelessapproved20:08
leonardrsinzui: ok, you're on20:08
jelmerlifeless: Thanks20:09
sinzuileonardr, sorry about my delays. I am ready to review21:40
leonardrsinzui, great21:40
leonardrthe branch is https://code.edge.launchpad.net/~leonardr/launchpad/accept-oauth-signatures/+merge/3569721:40
leonardrif there's anything you don't understand about the system i'm changing or what i'm aiming for, tell me andi 'll go through it21:40
sinzuileonardr, I am ready, the backstory is interesting21:43
leonardrsinzui: ok, go through the helpers i added to _webservice.py, and then see what i did with them in authorize-token.txt21:44
sinzuileonardr, do we care if I hack my user-agent string?21:49
leonardrsinzui: the user-agent thing is mostly to avoid any plausible deniability that an unauthorized client might have "accidentally" acquired someone's GRANT_PERMISSIONS token. it can't provide any real security21:51
sinzuifrom canonical.launchpad.interfaces import IPersonSet becomes21:55
sinzuifrom lp.registry.interfaces.person import IPersonSet21:55
sinzuileonardr, ^ c.l.interfaces is deprecated21:55
leonardrthanks21:56
=== salgado is now known as salgado-afk
sinzuiThough I am certain that IPersonSet will be the very last thing we remove21:56
sinzuileonardr, I am at 435 and I an very impressed.21:56
sinzuileonardr, are you using no-priv because this is a well known user? Is there reason to not use a person create with the factory?21:58
leonardrsinzui: i don't think it matters, i just haven't used the factory much21:58
sinzuileonardr, I have no other questions. This is a nice implementation and I see you tested the state conditions22:08
leonardrsinzui, thanks. can you point me to a good example of a pagetest that uses the factory?22:09
sinzuiI think so22:10
sinzuileonardr, the factory is already in the test name space. so the minimum you need it22:11
sinzuiis user = factory.makePerson(name='test-user')22:12
sinzuileonardr, you can also use login_person(user)22:12
sinzuilook in lp/registry/stories/webservice/xx-project-registry to see a lot of factory uses in a story22:13
leonardri think i have it...22:15
jelmerany rc reviewers around?23:07
=== matsubara is now known as matsubara-afk

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!