lifelesssinzui: 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 bug02:48
=== 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
rinzesinzui, hi10:36
=== matsubara-afk is now known as matsubara
noodles775Hi 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/2888512:55
noodles775Let me know if I'm not making sense on the MP.12:55
salgadonoodles775, sure, I have it in my inbox and will get to it shortly12:56
noodles775salgado: thanks.12:56
salgadonoodles775, in which case is the sreg extension not included in the response?13:06
noodles775salgado: 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:07
noodles775It's not seen in the current code as _getEmailAddressAndFullName is only called if the account doesn't exist.13:08
salgadonoodles775, that's weird because the code that generates the positive openid response is including the sreg extension unconditionally13:10
* salgado tries to dupe13:10
salgadoindeed, duped13:11
noodles775Yeah, I'd assumed the sreg response should always be there, and was surprised too.13:12
salgadonoodles775, but this is a bug in the testopenid implementation13:14
noodles775Where should I look?13:14
salgadotestopenid == the OpenID provider we use when developing/testing13:14
salgadoI'm checking it now13:14
salgadonoodles775, lib/lp/testopenid/browser/server.py13:14
noodles775OK, so we can be confident that the staging/production SSO always include the sreg.13:15
salgadocreatePositiveResponse() adds the sreg extension, but it's empty13:15
salgadonoodles775, I think I've found the problem13:16
salgadonoodles775, OpenIDLogin.render() asks for 'email' and 'fullname' in the sreg response13:17
salgadobug testopenid ignores that and only includes the nickname13:17
=== rinze is now known as jelmer-lunch
noodles775Right... I see.13:18
noodles775So should it instead include *only* the requested items (email/fullname) or in addition to the nickname?13:18
salgadoI think it's fine to include the 3 of them, but the nickname shouldn't be used anywhere13:19
noodles775OK, I'll update it... thanks salgado !13:20
noodles775salgado: 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:22
salgadonoodles775, sure, I can do that13:24
salgadonoodles775, 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:27
noodles775salgado: 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:28
salgadoI see13:29
salgadoand 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:29
noodles775Yes, but that is the long-term plan.13:30
salgado415+        except LookupError:13:38
salgado416+            assert email_address is not None and full_name is not None, (13:38
salgado417+                "An email address and full name are required to "13:38
salgado418+                "create an account.")13:38
salgadonoodles775, I'd move that assert out of the except block13:38
noodles775That won't be required... I'd only added it because of the above issue.13:39
noodles775Hrm, although, it's still possible for other callsites in the future to pass in None (even if processPositive.. doesn't).13:40
noodles775So yep, will do.13:40
salgadonoodles775, 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:48
noodles775salgado: 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
salgadoI guess I should reload the m-p page, then13:50
* noodles775 hasn't pushed... hangon.13:50
noodles775OK, pushed r9515...13:52
salgadonoodles775, IPersonSet.getOrCreateByOpenIDIdentifier() has a requestor argument but the actual implementation doesn't14:02
noodles775salgado: 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:02
salgadocool, let me reload again14:04
noodles775Thanks 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:31
salgadonoodles775, np14:39
=== 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
noodles775jelmer-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/2888514:57
jtvjelmer-lunch: my windmill tests pass now...  I think there was some change in setup that we both needed to synchronize to.14:59
=== jelmer-lunch is now known as jelmer_
jelmer_noodles775: ok15:00
jelmer_jtv: I'll have another look, thanks.15:01
=== jelmer_ is now known as Guest58376
=== Guest58376 is now known as rinze
=== deryck is now known as deryck[lunch]
jtvnoodles775: anything to add to jelmer's review?16:24
noodles775jtv: I pasted our irc conv. on the mp.16:28
jtvnoodles775: that's all about us being rusty and making dirty jokes afaics, nothing about the branch itself...16:30
jtvAh, apart from the bit about henning's xxx that triggered the jokes16:30
jtvrinze: the tests on my JS branch are passing now, repeatedly and consistently... do I get your Approval vote?16:31
jtvnoodles775: if you approve of the js, maybe you can hit the js review button?16:32
noodles775jtv: 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
noodles775Oh, but I can hit the js review button if you need it (sorry).16:33
jtvI'd love that, thanks!16:33
rinzejtv: Yeahp!16:35
rinzejtv: Updating the MP now16:35
jtvrinze: great!  Then I can still get it in before the deadline.16:35
rinzeAlso, if somebody here can review a oneliner: https://code.edge.launchpad.net/~jelmer/launchpad/fix-devel/+merge/2901116:36
rinzeIt's the testfix for devel16:36
noodles775rinze: done (and jtv too)16:37
jtvrinze: least I can do...16:37
jtvoh, too late :-)16:37
rinzejtv: noodles beat you to it, but thanks anyway :-)16:37
=== 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
* rinze looks at bigjools branch briefly16:43
rinzebigjools: there's a typo in paramter in your angry comment :-)16:43
bigjoolsI was too angry to speel16:43
rinzein 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:44
rinzeI.e. if testCopyArchiveVirtualization specified nonvirtualized=False it would still not use virtualization16:45
bigjoolsrinze: yes deliberately, but I suppose it should default to False16:45
bigjoolsI was still too angry to think :)16:45
bigjoolsboth fixed16:46
=== henninge_ is now known as henninge
=== salgado is now known as salgado-lunch
rinzebigjools: did you push your changes?16:54
bigjoolsrinze: no, I was waiting for more comments about my angry coding :)16:54
rinzebigjools: Tests pass here and I don't see any other issues.16:55
rinzeperhaps you could add an assert somewhere that self.makeCopyArchive() can also still create archives that require virtualized builds16:56
bigjoolsyeah I saw that problem as you mentioned it too16:56
rinzebigjools: in your latest tip, the nonvirtualized argument to makeCopyArchive defaults to None rather than False17:02
bigjoolsrinze: some changes pushed up again17:02
bigjoolsmissed it17:02
bigjoolsok pushed up again17:03
rinzebigjools: I don't see the diff for that last change yet, but I trust that's correct :-) r=me17:05
bigjoolsthanks rinze :)17:05
bigjoolssaving me from my own stupidity on that branch17:05
=== 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
henningejtv: review sent (finally)17:18
jtvhenninge: hope it's an approval!17:19
henningejtv: almost17:19
henningejtv: I mean, there are no big issues but I'd like to hear your opinions on some of my comments.17:19
=== 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
bigjoolsis anyone around for a simple but urgent review?18:05
=== 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
* bigjools meets rockstar here19:49
rockstarbigjools, I've come to the conclusion that soyuz is generally hard to understand unless you know the whole entire subsystem.19:49
rockstarThe latter has become a new goal of mine.19:49
bigjoolsI still learn stuff 3.5 years after I started on it....19:50
rockstarbigjools, why do you "create copy archives for rebuild testing"? That's not a PPA thing, right?19:53
bigjoolsrockstar: no, it's more like a main archive presented like a PPA.  It allows us to copy source from somewhere else and rebuild it19:54
rockstarbigjools, presented like a PPA?  What's the use case there?19:55
bigjoolsUI code re-use :)19:55
rockstar(Patch looks fine, just trying to squeeze a few user stories out)19:55
bigjoolswe didn't want it looking like the normal distro pages19:55
bigjoolsthe main use case is to find failed builds19:55
rockstarWhy shouldn't it look the same?19:55
rockstarEr, why shouldn't it look like the normal distro pages?19:56
bigjoolsthey're overkill for this type of archive, mainly19:56
rockstarbigjools, so I guess the thing I'm not understanding is what type of archive this is.20:08
rockstarIs it a distro archive or a PPA or something else I don't know about.20:08
james_wit's a third type20:12
rockstarjames_w, which is?20:14
rockstarIs it like a throwaway archive just for rebuilding copies of existing packages?20:14
james_wrockstar: pretty much20:16
rockstarjames_w, ah, okay.  That parses better now.20:16
james_wit'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 there20:16
james_wso the bug in question is where it copies a bunch of sources, and then fails while creating the build records for them20:17
rockstarYeah, that makes sense.20:17
james_win particular because the new archive doesn't have i386 enabled, and i386 is required to build arch "all" packages, it's the nominatedarchindep20:17
* bigjools watches james_w explain how my branch works :)20:22
bigjoolssorry - had to dash off, child-related poo emergency20:22
bigjoolsactually it wasn't :(20:22
rockstarYeah, I very much understand nominatedarchindep.  Remember that time that recipe builds were using the arm builder...20:22
bigjoolsunfortunately yes20:24
james_wdamn you bigjools, I've now started typing poopy instead of poppy20:25
james_wrockstar: a small branch of my own if you would: https://code.launchpad.net/~james-w/launchpad/move-file-is-a-directory/+merge/2903320:27
rockstarjames_w, I live serve...20:28
rockstarEr, I live to serve20:28
rockstarI apparently don't live to type correctly.20:28
bigjoolsjames_w: haha20:29
bigjoolsrockstar: thanks for the review20:38
rockstarjames_w, your branch is made of awesomeness.  Thanks for doing that.20:39
bigjoolsrockstar: aw jeez you Quoted me!20:39
rockstarOne day, we'll have all our dependencies nice and neat.20:39
rockstarbigjools, yeah, it had to be done.20:39
rockstarbigjools, also, you're like, the 6th person today to tell me about poo related mishaps that occurred today.20:40
bigjoolsone day you will have kids and will enthusiastically join in20:40
james_wthanks rockstar20:40
=== matsubara is now known as matsubara-afk
=== salgado is now known as salgado-afk

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