[02:48] sinzui: if you're back, I've moved the profile module to lp.services as per your request, but it looks to me like it does not meet the namespace policy about deps. I've mailed the bug [02:48] s/bug/mp/ === rinze changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [10:36] sinzui, hi === matsubara-afk is now known as matsubara [12:55] Hi salgado, if you've got a spare few mins, can you take a look at the issue I've identified at: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885 [12:55] Let me know if I'm not making sense on the MP. [12:56] noodles775, sure, I have it in my inbox and will get to it shortly [12:56] salgado: thanks. [13:06] noodles775, in which case is the sreg extension not included in the response? [13:07] salgado: normal login with existing account. ie. 1) start dev server, 2) click login 3) enter admin@canonical.com:test 4) click login. In this case calling _getEmailAddressAndFullName raises the assert. [13:08] It's not seen in the current code as _getEmailAddressAndFullName is only called if the account doesn't exist. [13:10] noodles775, that's weird because the code that generates the positive openid response is including the sreg extension unconditionally [13:10] * salgado tries to dupe [13:11] indeed, duped [13:12] Yeah, I'd assumed the sreg response should always be there, and was surprised too. [13:14] noodles775, but this is a bug in the testopenid implementation [13:14] Where should I look? [13:14] testopenid == the OpenID provider we use when developing/testing [13:14] I'm checking it now [13:14] noodles775, lib/lp/testopenid/browser/server.py [13:15] OK, so we can be confident that the staging/production SSO always include the sreg. [13:15] createPositiveResponse() adds the sreg extension, but it's empty [13:15] Thanks. [13:16] noodles775, I think I've found the problem [13:17] noodles775, OpenIDLogin.render() asks for 'email' and 'fullname' in the sreg response [13:17] bug testopenid ignores that and only includes the nickname === rinze is now known as jelmer-lunch [13:17] s/bug/but [13:18] Right... I see. [13:18] So should it instead include *only* the requested items (email/fullname) or in addition to the nickname? [13:19] I think it's fine to include the 3 of them, but the nickname shouldn't be used anywhere [13:20] OK, I'll update it... thanks salgado ! [13:22] salgado: also, I assume you don't have time to review the actual branch, but if you could have a brief look and make sure you think it's a sane change, that would be great. [13:24] noodles775, sure, I can do that [13:25] Ta. [13:27] noodles775, out of curiosity, why somebody needs a launchpad account to buy stuff from the software center? wouldn't a Ubuntu SSO account be more appropriate in this case? [13:28] salgado: yes, the must already have an SSO account to make the payment, it's after they've paid for the software, we need to give them access to a private PPA (ie. a private ppa subscription), and that's only possible if you've got an LP account. [13:29] I see [13:29] and it'd be lots of work to make it possible for someone to have a private ppa subscription without having a Launchpad profile, I assume? [13:30] Yes, but that is the long-term plan. [13:31] cool! [13:38] 415 + except LookupError: [13:38] 416 + assert email_address is not None and full_name is not None, ( [13:38] 417 + "An email address and full name are required to " [13:38] 418 + "create an account.") [13:38] noodles775, I'd move that assert out of the except block [13:39] That won't be required... I'd only added it because of the above issue. [13:40] Hrm, although, it's still possible for other callsites in the future to pass in None (even if processPositive.. doesn't). [13:40] So yep, will do. [13:40] indeed [13:48] noodles775, I think you can get rid of checked_for_email and the two email_set.getByEmail(email_address) calls by doing "email = email_set.getByEmail(email_address)" at the beginning instead of "email = None" [13:50] salgado: yep - that was part of r9514 which I've reverted after our discussion (I couldn't do it at the beginning at that point, because email was None when called via processPositive..) [13:50] right [13:50] I guess I should reload the m-p page, then [13:50] * noodles775 hasn't pushed... hangon. [13:52] OK, pushed r9515... [14:02] noodles775, IPersonSet.getOrCreateByOpenIDIdentifier() has a requestor argument but the actual implementation doesn't [14:02] salgado: thanks, I'll remove it from the IFace. I've just pushed 9516 which just adds the email/fullname to the sreg response for the testopenid server too. [14:04] cool, let me reload again [14:31] Thanks salgado... I'll paste the irc conversation on the MP too. (And I've pushed the changes you suggested so far). I think that should be enough to get a normal review. [14:39] noodles775, np === noodles775 changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [sinzui, noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:57] jelmer-lunch: if you've time when you get back: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-598464-get-or-create-from-identity/+merge/28885 [14:59] jelmer-lunch: my windmill tests pass now... I think there was some change in setup that we both needed to synchronize to. === jelmer-lunch is now known as jelmer_ [15:00] noodles775: ok [15:01] jtv: I'll have another look, thanks. === jelmer_ is now known as Guest58376 === Guest58376 is now known as rinze [15:08] Sure. === deryck is now known as deryck[lunch] [16:24] noodles775: anything to add to jelmer's review? [16:28] jtv: I pasted our irc conv. on the mp. [16:30] noodles775: that's all about us being rusty and making dirty jokes afaics, nothing about the branch itself... [16:30] Ah, apart from the bit about henning's xxx that triggered the jokes [16:31] rinze: the tests on my JS branch are passing now, repeatedly and consistently... do I get your Approval vote? [16:32] noodles775: if you approve of the js, maybe you can hit the js review button? [16:33] jtv: no, I was just making sure rinze hadn't missed anything obvious... I didn't do a proper review. As you mentioned in your MP, the branch is just cleaning up already working JS (outside of the chromium fix - just a different class).. [16:33] Oh, but I can hit the js review button if you need it (sorry). [16:33] I'd love that, thanks! [16:35] jtv: Yeahp! [16:35] jtv: Updating the MP now [16:35] rinze: great! Then I can still get it in before the deadline. [16:36] Also, if somebody here can review a oneliner: https://code.edge.launchpad.net/~jelmer/launchpad/fix-devel/+merge/29011 [16:36] It's the testfix for devel [16:37] rinze: done (and jtv too) [16:37] thanks [16:37] rinze: least I can do... [16:37] oh, too late :-) [16:37] jtv: noodles beat you to it, but thanks anyway :-) === bigjools changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: - || queue: [sinzui, noodles775, bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === rinze changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles775 || queue: [bigjools] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:43] * rinze looks at bigjools branch briefly [16:43] bigjools: there's a typo in paramter in your angry comment :-) [16:43] I was too angry to speel [16:44] in lib/lp/soyuz/scripts/tests/test_populatearchive.py nonvirtualized seems to be a boolean parameter, yet it defaults to None and checks for "is not None" [16:45] I.e. if testCopyArchiveVirtualization specified nonvirtualized=False it would still not use virtualization [16:45] rinze: yes deliberately, but I suppose it should default to False [16:45] I was still too angry to think :) [16:46] both fixed === henninge_ is now known as henninge === salgado is now known as salgado-lunch [16:54] bigjools: did you push your changes? [16:54] rinze: no, I was waiting for more comments about my angry coding :) [16:54] hehe [16:55] bigjools: Tests pass here and I don't see any other issues. [16:56] perhaps you could add an assert somewhere that self.makeCopyArchive() can also still create archives that require virtualized builds [16:56] yeah I saw that problem as you mentioned it too [16:56] merde [17:02] bigjools: in your latest tip, the nonvirtualized argument to makeCopyArchive defaults to None rather than False [17:02] rinze: some changes pushed up again [17:02] ARGH [17:02] missed it [17:03] ok pushed up again [17:05] bigjools: I don't see the diff for that last change yet, but I trust that's correct :-) r=me [17:05] thanks rinze :) [17:05] saving me from my own stupidity on that branch === rinze changed the topic of #launchpad-reviews to: On call: jelmer || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [17:18] jtv: review sent (finally) [17:18] ahhh! [17:19] henninge: hope it's an approval! [17:19] jtv: almost [17:19] grrr [17:19] jtv: I mean, there are no big issues but I'd like to hear your opinions on some of my comments. [17:20] ;-) === deryck[lunch] is now known as deryck === rinze 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 [18:05] is anyone around for a simple but urgent review? === salgado-lunch is now known as salgado === matsubara is now known as matsubara-lunch === matsubara-lunch is now known as matsubara === rockstar changed the topic of #launchpad-reviews to: On call: rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [19:49] * bigjools meets rockstar here [19:49] bigjools, I've come to the conclusion that soyuz is generally hard to understand unless you know the whole entire subsystem. [19:49] The latter has become a new goal of mine. [19:50] attaboy [19:50] I still learn stuff 3.5 years after I started on it.... [19:53] bigjools, why do you "create copy archives for rebuild testing"? That's not a PPA thing, right? [19:54] rockstar: no, it's more like a main archive presented like a PPA. It allows us to copy source from somewhere else and rebuild it [19:55] bigjools, presented like a PPA? What's the use case there? [19:55] UI code re-use :) [19:55] (Patch looks fine, just trying to squeeze a few user stories out) [19:55] we didn't want it looking like the normal distro pages [19:55] the main use case is to find failed builds [19:55] Why shouldn't it look the same? [19:56] Er, why shouldn't it look like the normal distro pages? [19:56] they're overkill for this type of archive, mainly [20:08] bigjools, so I guess the thing I'm not understanding is what type of archive this is. [20:08] Is it a distro archive or a PPA or something else I don't know about. [20:12] it's a third type [20:14] james_w, which is? [20:14] Is it like a throwaway archive just for rebuilding copies of existing packages? [20:16] rockstar: pretty much [20:16] james_w, ah, okay. That parses better now. [20:16] it's grown some extra features, but the primary use case is to copy sources from Ubuntu in to a new archive, and then rebuild them there [20:17] so the bug in question is where it copies a bunch of sources, and then fails while creating the build records for them [20:17] Yeah, that makes sense. [20:17] in particular because the new archive doesn't have i386 enabled, and i386 is required to build arch "all" packages, it's the nominatedarchindep [20:22] * bigjools watches james_w explain how my branch works :) [20:22] sorry - had to dash off, child-related poo emergency [20:22] nice [20:22] actually it wasn't :( [20:22] Yeah, I very much understand nominatedarchindep. Remember that time that recipe builds were using the arm builder... [20:24] unfortunately yes [20:25] damn you bigjools, I've now started typing poopy instead of poppy [20:27] rockstar: a small branch of my own if you would: https://code.launchpad.net/~james-w/launchpad/move-file-is-a-directory/+merge/29033 [20:28] james_w, I live serve... [20:28] Er, I live to serve [20:28] \o/ [20:28] I apparently don't live to type correctly. [20:29] james_w: haha [20:38] rockstar: thanks for the review [20:39] james_w, your branch is made of awesomeness. Thanks for doing that. [20:39] rockstar: aw jeez you Quoted me! [20:39] One day, we'll have all our dependencies nice and neat. [20:39] bigjools, yeah, it had to be done. [20:40] bigjools, also, you're like, the 6th person today to tell me about poo related mishaps that occurred today. [20:40] one day you will have kids and will enthusiastically join in [20:40] thanks rockstar === matsubara is now known as matsubara-afk === salgado is now known as salgado-afk