[00:32] <jelmer> lifeless: Sorry, I got distracted and forgot about the review. :-/
[00:33] <lifeless> thats alright
[08:19] <noodles775> Thanks for the db-review lifeless, I've just responded and will be around if you want to chat about it.
[08:24] <lifeless> I'm hereish, I'll be in and out - its getting late here.
[08:25] <noodles775> Yeah, I replied to the MP so you could leave it 'til tomorrow if you wish :)
[08:30] <noodles775> stub: I've responded to your review (already approved) with an incremental to use the Message model for comments on DistroSeriesDifference when you've a chance to look over it: https://code.launchpad.net/~michael.nelson/launchpad/distro-series-difference-schema/+merge/33515
[08:30] <lifeless> ok, I'll need to look closely at the model which I haven't done yet (as I mentioned)
[08:31] <lifeless> please do drop the latest_ prefix; I'll get a more complete review for you tomorrow.
[08:31] <stub> noodles775: Any reason that two DistroSeriesDifference rows can share a Message?
[08:31] <lifeless> ish.
[08:33] <noodles775> stub: Right - my mistake. I think I was originally thinking I'd need extra meta-data per comment (ie. which versions the comment was made with), but ended up removing it. So I'll just remove that second create table?
[08:34] <stub> eh? I was wondering why you made (distroseriesdifference, message) UNIQUE. Making just message UNIQUE would make more sense.
[08:35] <stub> So a Message is linked to no more than one distroseriesdifference.
[08:36]  * noodles775 needs coffee.
[08:36] <noodles775> Yes.
=== StevenK changed the topic of #launchpad-reviews to: On call: - || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011,StevenK] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[09:43] <StevenK> henninge: O hai! Review my branch? I have cookies ....
[09:44] <henninge> ... COOKIES ....
[09:45] <henninge> StevenK: is it "ifp-use-istore" ?
[09:45] <StevenK> henninge: Indeed
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [https://code.edge.launchpad.net/~lifeless/launchpad/bug-618849/+merge/34011] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[09:46] <henninge> StevenK: in progress ...
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: StevenK || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[09:51] <henninge> StevenK: looks good but you are introducing extra new lines in the import section that should not be there.
[09:51] <StevenK> henninge: I like to seperate the canonical and lp sections, I should stop doing that?
[09:52] <henninge> Running "utilities/format-imports <path1> <path2>" will fixt that for you.
[09:52] <henninge> StevenK: yes as we'd like to standardize on what the script does.
[09:53] <henninge> The script could, of course, be changed to enforce your way if that was the consensus ... ;)
[09:54] <StevenK> henninge: Script ran, changes pushed
[09:54] <henninge> StevenK: r=me ;-) Thanks
[09:56] <StevenK> henninge: Thanks!
[09:56] <henninge> StevenK: ah see, the script also found some other glitches.
[09:58] <StevenK> henninge: I did notice, yes :-)
[10:04]  * wgrant looks for more support for putting it in 'make lint' or similar.
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== benji changed the topic of #launchpad-reviews to: On call: henninge || reviewing: - || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:03] <benji> henninge: I have a small one for you: https://code.edge.launchpad.net/~benji/launchpad/bug-622765/+merge/34077
[16:03]  * henninge looks
[16:04] <henninge> benji: sure, I'll do it ;-)
[16:04] <benji> heh :)
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== matsubara is now known as matsubara-lunch
=== abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || reviewing: benji, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:46] <henninge> benji: r=me with a few comments ;-)
=== henninge changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:46] <benji> cool, taking a look
[16:53] <benji> henninge: I don't see comments on the MP; are they somewhere else?
[16:53] <henninge> hm, should be. Did it by email, let me chek.
[16:53] <henninge> c
[16:54] <benji> maybe you got cought in the recent edge flappage
[16:55] <henninge> benji: it just came in! ;-)
[16:55] <benji> yep, I just got it too; I guess I need to be more patient
[16:56] <noodles775> henninge: I'll add one to the queue, but understand that you'll probably not get to it before finishing :)
=== noodles775 changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [noodles775] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[16:56] <noodles775> Ah, you've finished :)
[16:56] <henninge> noodles775: I have already finished but abentley is here, too. ;-)
[16:56] <henninge> Hi abentley! ;)
[16:57] <noodles775> abentley_: when you've time: https://code.edge.launchpad.net/~michael.nelson/launchpad/distro-series-difference-model/+merge/34086
[16:57] <henninge> noodles775: or was this an UI review?
[16:57] <henninge> probably not
[16:57] <noodles775> Nope.
[17:02] <henninge> benji: hope you can handle my review because I have to go now. ;-)
[17:03] <benji> henninge: I had one minor rebuttle.  Shall I redirect it to abentley?
=== benji is now known as benji-lunch
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: noodles775 || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha is now known as Ursinha-lunch
=== matsubara-lunch is now known as matsubara
=== deryck is now known as deryck[lunch]
=== abentley_ is now known as abentley
=== benji-lunch is now known as benji
=== benji changed the topic of #launchpad-reviews to: On call: abentley || reviewing: noodles775 || queue: [benji] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[18:32] <benji> abentley: you get to channel henning: please see my last message at https://code.launchpad.net/~benji/launchpad/bug-622765/+merge/34077/+index thanks
=== deryck[lunch] is now known as deryck
=== Ursinha-lunch is now known as Ursinha
[19:50] <abentley> benji, it sounds like all you care about is the return value-- the parsed_lines value.  Is that right?
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: benji || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[19:53] <benji> abentley: not quite; if parse_file respects logparser_max_parsed_lines then it will return 2 (the maximum in effect during the test run), I want to be sure that it both returns 2 (IOW, it hit the max) and show that it did not parse line 2 of the input file because that shows that it took the passed in value of lines_parsed into account when checking to see if the max has been reached
[19:53] <benji> it strikes me that (a variation) on the above would be a good comment to add just before the asserts to explain them
[19:54] <abentley> benji, then perhaps you should test that the parsed value of 2 is not in downloads.items()
[19:54] <abentley> benji, it's better to not need a comment if you can avoid it.
[19:55] <benji> I attempted to show that it wasn't parsed by asserting tha downloads.items() is empty.
[19:55] <benji> (maybe I misunderstood you)
[19:59]  * benji thinks he should have mentioned abentley's name.
[19:59] <abentley> benji, I realize that.
[19:59] <abentley> benji, I'm saying it's not clear what you're trying to demonstrate.
[20:00] <abentley> benji, If you're trying to demonstrate that the parsed value of line 2 isn't in download.items(), you can express that pretty directly in Python.
[20:00] <abentley> benji, self.assertTrue(line_2 not in download.items())
[20:03] <benji> If you would prefer that, I'll spell it that way.  Me, when I see something like that I think "well, there are lots of values that aren't there, I wonder what is actually there and why that particular value not being there is important"; I don't feel like fighting over it though
[20:08] <benji> abentley: wait, maybe I do feel like fighting ;)  If we assert that it isn't equal to something, then that test is unlikely to catch a bug that would make it parse more (say the test data changes and the negative assertion isn't updated or there is a bug in the negative assertion from the get-go (I typo it or something); therefore the assertion that the parsed data is empty is superior
[20:10] <abentley> benji, whatever you want to test, test that.  Don't test other things.  But if you want to test that that what gets parsed is what you expect, I think it would be much clearer to not have a 404 in the list.
[20:13] <benji> abentley: I can agree with that.  The 404 being the first thing is a mistake.  It should be something that generates a value in downloads.  I'll change the test accordingly.
[20:15] <abentley> benji, works for me.
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[20:40] <benji> abentley: it looks like henning forgot step 2: the Status is still "Needs review"
[20:40] <benji> (https://code.launchpad.net/~benji/launchpad/bug-622765/+merge/34077/+index)
[20:40] <abentley> benji, if you're satisfied that it's been approved, feel free to set it yourself.
[20:40] <benji> ok; I didn't know if that was kosher or not
[20:41] <benji> done, thanks
=== matsubara is now known as matsubara-afk
[20:54] <leonardr> abentley, i think https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123 is ready for review
=== abentley 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
[21:00] <abentley> leonardr, your doctest looks like a unit test, not documentation.  Could you please rewrite them as unit tests?
[21:00] <leonardr> webservice/oauth.txt?
[21:04] <abentley> I was thinking of lib/canonical/launchpad/doc/oauth.txt and webservice/oauth.txt both look like their main goal is to provide test coverage.
[21:06] <leonardr> i can turn webservice/oauth.txt into a unit test but i'd rather not have to convert doc/oauth.txt just now--i just made some minor changes to get it to work with the permission system
[21:09] <abentley> leonardr, you don't have to change the existing code, but I'd like to see lines 61-84 of the patch as a unit test.
[21:24] <abentley> leonardr, could you explain why these removeSecurityProxies are needed?
[21:25] <leonardr> abentley: sure
[21:25] <leonardr> we use OAuthAccessToken objects two different ways in the tests
[21:25] <leonardr> we use them to demonstrate the server-side behavior when someone tries to touch an OAuthAccessToken
[21:26] <leonardr> in that case the security proxy is left in place, and part of its job is to examine the request in which the server-side behavior is taking place
[21:27] <leonardr> but, we also use an OAuthAccessToken object as a convenient repository for OAuth authentication information when we create *client-side* objects like LaunchpadWebServiceCaller and launchpadlib Launchpad objects
[21:28] <leonardr> In those cases, we would like to look at OAuthAccessToken's data members without having the security check run, because the security check assumes there is a current request, and we're trying to _create_ a request
[21:28] <leonardr> a similar situation occurs when we look at the incoming request and try to determine if it's signed correctly
[21:29] <leonardr> we need to access OauthAccessToken's data members without the security proxy, because the security proxy assumes what we're trying to determine--that the request has been identified with a particular OAuth token
[21:31] <abentley> leonardr, so there's a pretty big difference between the active OathAccessToken, which we must be able to see, and arbitrary OauthAccessTokens which happen to be in the database?
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
[21:32] <leonardr> well, there are situations where you must be able to see access tokens from the database--that's the point of this branch
[21:33] <abentley> leonardr, right.  I meant that for the current one, access is mandatory, but for others, it depends on your permissions.
[21:34] <leonardr> sorry, i still don't understand
[21:34] <leonardr> the server gets some string data from the client
[21:34] <leonardr> which identifies an oauth access token
[21:35] <leonardr> as part of verifying whether the client is really authorized to use this access token, we need to strip the security proxy and look at its data members
[21:35] <leonardr> after the access token has been identified, all future attempts to look at _any_ access token are subject to the security proxy rules
[21:36] <abentley> leonardr, could we make this verification internal to OauthAccessToken?  Then we wouldn't need external access to its protected members.
[21:40] <leonardr> salgado, is this a good idea? take the code in servers.py, that calls checkNonceAndTimestamp, and everything else up to the end of the method? basically a OAuthAccessToken.getPrincipal method which we can leave unprotected?
[21:43] <abentley> leonardr, sorry to be more anal than usual, but you did ask for a review of the use of removeSecurityProxy, so I'm trying to make sure I understand the issues.
[21:43] <leonardr> sure
[21:45] <abentley> the best case is to avoid using it in production code, but I can see that bootstrapping the security foo can be a problem.
[21:49] <abentley> leonardr, I'm noticing that the line wrapping doesn't match our usual style.  Usually when we wrap, we put a newline after the opening parenthesis, then an indent, then the arguments.
[21:49] <leonardr> all right, i'll fix it
[21:51] <abentley> leonardr, see "Multiline function calls" in https://dev.launchpad.net/PythonStyleGuide#Whitespace%20and%20Wrapping
[21:56] <abentley> leonardr, LaunchpadWebServiceCaller is not exposed in the API, right?
[21:57] <leonardr> i don't understand the question. when you say 'api' do you mean the web service or some internal python api?
[21:59] <leonardr> LWSC is a test class defined in testing/pages.py. it is not available except when the test suite is running
[22:01] <abentley> leonardr, I mean the web service.
[22:01] <leonardr> no, LWSC is a test class that makes HTTP requests _to_ the web service
[22:01] <leonardr> it is not available _through_ the web service
[22:06] <abentley> leonardr, so it seems like the risk of assigning the token to LWSC.access_token is minimal.
[22:07] <leonardr> right. this is the 'client-side' code that uses an OAuthAccessToken as a data repository. but if you want it's easy to turn the token object into a dict or something to reduce the risk
[22:07] <leonardr> of carrying around an unprotected object
[22:13] <abentley> leonardr, that's okay, it looks like test code anyhow.
[22:13] <abentley> leonardr, I've given it a "needs fixing".  I'll be happy to re-review.  https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123
[22:14] <leonardr> ok
[22:14] <leonardr> i'll work on it tomorrow. thanks
=== abentley changed the topic of #launchpad-reviews to: On call: - || reviewing: leonardr || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== abentley 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