| jelmer | lifeless: Sorry, I got distracted and forgot about the review. :-/ | 00:32 |
|---|---|---|
| lifeless | thats alright | 00:33 |
| noodles775 | Thanks for the db-review lifeless, I've just responded and will be around if you want to chat about it. | 08:19 |
| lifeless | I'm hereish, I'll be in and out - its getting late here. | 08:24 |
| noodles775 | Yeah, I replied to the MP so you could leave it 'til tomorrow if you wish :) | 08:25 |
| 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:30 |
| 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:31 |
| 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:33 |
| stub | eh? I was wondering why you made (distroseriesdifference, message) UNIQUE. Making just message UNIQUE would make more sense. | 08:34 |
| stub | So a Message is linked to no more than one distroseriesdifference. | 08:35 |
| * noodles775 needs coffee. | 08:36 | |
| noodles775 | Yes. | 08:36 |
| === 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 | ||
| StevenK | henninge: O hai! Review my branch? I have cookies .... | 09:43 |
| henninge | ... COOKIES .... | 09:44 |
| henninge | StevenK: is it "ifp-use-istore" ? | 09:45 |
| StevenK | henninge: Indeed | 09:45 |
| === 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 | ||
| henninge | StevenK: in progress ... | 09:46 |
| === 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 | ||
| 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:51 |
| 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:52 |
| henninge | The script could, of course, be changed to enforce your way if that was the consensus ... ;) | 09:53 |
| StevenK | henninge: Script ran, changes pushed | 09:54 |
| henninge | StevenK: r=me ;-) Thanks | 09:54 |
| StevenK | henninge: Thanks! | 09:56 |
| henninge | StevenK: ah see, the script also found some other glitches. | 09:56 |
| StevenK | henninge: I did notice, yes :-) | 09:58 |
| * wgrant looks for more support for putting it in 'make lint' or similar. | 10:04 | |
| === 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 | ||
| 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:03 | |
| henninge | benji: sure, I'll do it ;-) | 16:04 |
| benji | heh :) | 16:04 |
| === 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 | ||
| henninge | benji: r=me with a few comments ;-) | 16:46 |
| === 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 | ||
| benji | cool, taking a look | 16:46 |
| 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:53 |
| benji | maybe you got cought in the recent edge flappage | 16:54 |
| henninge | benji: it just came in! ;-) | 16:55 |
| benji | yep, I just got it too; I guess I need to be more patient | 16:55 |
| noodles775 | henninge: I'll add one to the queue, but understand that you'll probably not get to it before finishing :) | 16:56 |
| === 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 | ||
| noodles775 | Ah, you've finished :) | 16:56 |
| henninge | noodles775: I have already finished but abentley is here, too. ;-) | 16:56 |
| henninge | Hi abentley! ;) | 16:56 |
| 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. | 16:57 |
| henninge | benji: hope you can handle my review because I have to go now. ;-) | 17:02 |
| benji | henninge: I had one minor rebuttle. Shall I redirect it to abentley? | 17:03 |
| === 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 | ||
| 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 | 18:32 |
| === deryck[lunch] is now known as deryck | ||
| === Ursinha-lunch is now known as Ursinha | ||
| abentley | benji, it sounds like all you care about is the return value-- the parsed_lines value. Is that right? | 19:50 |
| === 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 | ||
| 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:53 |
| 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:54 |
| 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:55 |
| * 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. | 19:59 |
| 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:00 |
| 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:03 |
| 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:08 |
| 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:10 |
| 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:13 |
| abentley | benji, works for me. | 20:15 |
| === 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 | ||
| 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:40 |
| benji | done, thanks | 20:41 |
| === matsubara is now known as matsubara-afk | ||
| leonardr | abentley, i think https://code.edge.launchpad.net/~leonardr/launchpad/publish-tokens-2/+merge/34123 is ready for review | 20:54 |
| === 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 | ||
| 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:00 |
| 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:04 |
| 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:06 |
| 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:09 |
| abentley | leonardr, could you explain why these removeSecurityProxies are needed? | 21:24 |
| 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:25 |
| 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:26 |
| 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:27 |
| 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:28 |
| 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:29 |
| 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? | 21:31 |
| === 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 | ||
| 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:32 |
| abentley | leonardr, right. I meant that for the current one, access is mandatory, but for others, it depends on your permissions. | 21:33 |
| 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:34 |
| 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:35 |
| abentley | leonardr, could we make this verification internal to OauthAccessToken? Then we wouldn't need external access to its protected members. | 21:36 |
| 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:40 |
| 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:43 |
| 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:45 |
| 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:49 |
| abentley | leonardr, see "Multiline function calls" in https://dev.launchpad.net/PythonStyleGuide#Whitespace%20and%20Wrapping | 21:51 |
| abentley | leonardr, LaunchpadWebServiceCaller is not exposed in the API, right? | 21:56 |
| leonardr | i don't understand the question. when you say 'api' do you mean the web service or some internal python api? | 21:57 |
| leonardr | LWSC is a test class defined in testing/pages.py. it is not available except when the test suite is running | 21:59 |
| 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:01 |
| abentley | leonardr, so it seems like the risk of assigning the token to LWSC.access_token is minimal. | 22:06 |
| 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:07 |
| 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:13 |
| leonardr | ok | 22:14 |
| leonardr | i'll work on it tomorrow. thanks | 22:14 |
| === 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 | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!