| === EdwinGrubbs changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| james_w | Could I get a review on https://code.edge.launchpad.net/~james-w/launchpad/get-requested-reviews/+merge/18592 please? | 03:22 |
|---|---|---|
| === al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| stub | al-maisan: https://code.edge.launchpad.net/~stub/launchpad/page-performance-report/+merge/18522 | 08:49 |
| al-maisan | stub: looking | 08:49 |
| === noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -/- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: stub/- || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| al-maisan | stub: r=me | 09:05 |
| stub | ta | 09:07 |
| === al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, - || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, mthaddon || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, BjornT || queue [] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, BjornT || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | Hi BjornT , I'm loving your persistent test services branch! | 09:55 |
| noodles775 | I was wondering if there was a reason for not doing a property BaseLayer.persist_test_services rather than a module-level function? | 09:55 |
| stub | Is this branch leaving things like the librarian running between test.py invocations? | 10:04 |
| * stub wonders if it is useful for per-branch postgresql instances | 10:05 | |
| noodles775 | stub: yes it is. | 10:12 |
| BjornT | noodles775: i guess it could go to BaseLayer, even though it doesn't have any services. i'd be happy to move it to a property there; it makes it a bit cleaner | 10:15 |
| noodles775 | BjornT: ok. I've just sent the review, I only had one other thought. Thanks! | 10:16 |
| BjornT | noodles775: cool, thanks! | 10:16 |
| mrevell | noodles775, ping | 11:04 |
| noodles775 | Hi mrevell | 11:04 |
| mrevell | hey | 11:04 |
| === jpds changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, BjornT || queue [noodles775,jpds] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === mrevell is now known as mrevell-lunch | ||
| jpds | Review for https://code.launchpad.net/~jpds/launchpad/fix_517020/+merge/18608 please. :) | 12:34 |
| === noodles775 changed the topic of #launchpad-reviews to: on-call: al-maisan,noodles775 || reviewing: -, jpds || queue [noodles775] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | jpds: on it. | 12:57 |
| === al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: noodles775, jpds || queue [al-maisan] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| al-maisan | noodles775: this is the one: https://code.launchpad.net/~michael.nelson/launchpad/create-source-recipe-build2/+merge/18535 ? | 13:12 |
| noodles775 | al-maisan: yep, thanks! | 13:12 |
| al-maisan | no problem | 13:12 |
| leonardr | al-maisan, noodles775, i have an oversize branch that needs review (about 1200 lines, complicated but has lots of tests) | 13:20 |
| leonardr | can one of you take it? | 13:20 |
| noodles775 | pop it on the queue leonardr, if we don't get to it, the next reviewers will. | 13:20 |
| leonardr | noodles775, ok | 13:21 |
| === leonardr changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: noodles775, jpds || queue [al-maisan,leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| al-maisan | leonardr: I am just starting on noodles775's branch so not sure whether I'll get to it. | 13:21 |
| leonardr | ok, np | 13:21 |
| al-maisan | noodles775: why did you replace super(CannotUploadToPocket, self).__init__() with Exception.__init__() in CannotUploadToPocket? | 13:22 |
| noodles775 | ...hrm, I think lint complained about not calling the super class (in one of the others that didn't even call Exception.__init__()), so I unified them. | 13:23 |
| === mrevell-lunch is now known as mrevell | ||
| al-maisan | noodles775: if I understand https://dev.launchpad.net/PythonStyleGuide "Chaining method calls" correctly these __init__() calls will need to use the super() builtin | 13:36 |
| * al-maisan counts at least 3 occurrences | 13:36 | |
| noodles775 | al-maisan: I thought I had switched them to super()? | 13:36 |
| * noodles775 looks again. | 13:36 | |
| noodles775 | al-maisan: All the __init__ calls in my actual diff (the linked paste-bin) do use super(). But I'll go back and update the ones left over from the prerequisite branch too. | 13:38 |
| al-maisan | noodles775: Oh, I see .. I just merged your branch into devel .. that was probably wrong | 13:39 |
| noodles775 | al-maisan: yeah, see the note on the MP. Thanks. | 13:39 |
| al-maisan | yup | 13:39 |
| al-maisan | noodles775: has the pre-requisite branch landed? | 13:41 |
| noodles775 | al-maisan: nope, it was reviewed, approved with some suggestions (you can click on the link to see them). I updated the branch with those suggestions as well as fixed the bug identified there. | 13:42 |
| noodles775 | jpds: sorry, I got sidetracked writing a script to automate some of my review setup. So in addition to the stuff mentioned on the bug, | 13:46 |
| noodles775 | you've also updated the reviewer attribute so that it's now required=True? | 13:46 |
| noodles775 | jpds: nm, I miss-read the diff. | 13:48 |
| al-maisan | noodles775: can you please explain the lines 268-271 in http://pastebin.ubuntu.com/368292/ | 13:58 |
| al-maisan | noodles775: while you are at it, I am also mystified by lines 42-46 :P | 13:59 |
| noodles775 | al-maisan: Regarding 268-271, It's probably easier to comment those lines out and run the test, from m | 14:00 |
| noodles775 | woops, | 14:00 |
| noodles775 | So IBuild.calculated_buildstart has some asserts in it... | 14:00 |
| noodles775 | They assert that datebuilt and buildduration are actually set. | 14:00 |
| noodles775 | Calling assertProvides on a build accesses the calculated_buildstart property and the asserts are triggered. | 14:01 |
| jtv | rockstar: still pining to hear from you re my half-finished review: https://code.edge.launchpad.net/~jtv/launchpad/bug-499405-translationtemplates-buildmanager/+merge/17811 | 14:01 |
| noodles775 | (I don't personally think we should have asserts there inside a property) | 14:01 |
| al-maisan | noodles775: hrrmm .. what is the purpose of actually setting these 2 properties on lines 268-271? | 14:02 |
| noodles775 | al-maisan: run the test with those lines commented out. | 14:02 |
| al-maisan | OK | 14:02 |
| noodles775 | al-maisan: regarding 42-46, it might be easier to explain over the phone (although I'd like to ensure that docstring makes sense) | 14:03 |
| al-maisan | noodles775: so assertProvides() is trying to access the properties introduced on the interfaces .. and that fails for `calculated_buildstart` unless the other 2 are set | 14:05 |
| jpds | noodles775: No problem. | 14:05 |
| al-maisan | noodles775: r=me | 14:14 |
| noodles775 | Thanks al-maisan | 14:14 |
| noodles775 | jpds: did you mean to make owner readonly=False? If so, it'd be great to see tests of that (for security, who can and can't set it). | 14:15 |
| jpds | noodles775: Err, that should be readonly=False. | 14:21 |
| noodles775 | jpds: great. | 14:21 |
| jpds | So, I'll need tests, right. | 14:22 |
| jpds | noodles775: These should go into the stories/webservice/ right? | 14:23 |
| noodles775 | jpds: yep. | 14:23 |
| noodles775 | jpds: also, what's the reason for the change of vocabulary on reviewer? | 14:24 |
| jpds | noodles775: Should still be ValidPersonOrTeam. | 14:25 |
| noodles775 | jpds: gar, sorry. | 14:26 |
| noodles775 | jpds: I can't currently update the MP, so here's the review: http://pastebin.ubuntu.com/368904/ | 14:40 |
| noodles775 | Mainly just the tests that we chatted about earlier. | 14:40 |
| al-maisan | noodles775: I guess you won't be able to access my merge-proposal either, so here's the diff: http://pastebin.ubuntu.com/368906/ | 14:45 |
| jpds | noodles775: Sure, am working on the tests. | 14:45 |
| === al-maisan changed the topic of #launchpad-reviews to: on-call: al-maisan, noodles775 || reviewing: -, jpds || queue [al-maisan,leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === matsubara is now known as matsubara-lunch | ||
| === salgado is now known as salgado-lunch | ||
| === al-maisan changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: jpds || queue [al-maisan,leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === noodles775 changed the topic of #launchpad-reviews to: on-call: noodles775 || reviewing: al-maisan || queue [leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| noodles775 | al-maisan: it's a shame that there isn't a way to record what actually happened. It's not worth a new status FAILED_SOURCES_DELETED? | 15:46 |
| al-maisan | noodles775: maybe | 15:47 |
| al-maisan | noodles775: what do you think? Is it worth adding that new enum member? | 15:48 |
| noodles775 | If it means that it will be communicated to someone looking at the diff, then yeah, I'd think so. Otherwise not. Up to you. | 15:49 |
| al-maisan | noodles775: my gut feeling is that this is something we can add if enough users ask for it. | 15:50 |
| noodles775 | OK. | 15:50 |
| === matsubara-lunch is now known as matsubara | ||
| === salgado-lunch is now known as salgado | ||
| noodles775 | al-maisan: is the test case setup code that you've got there *identical* to the code in test_processpendingpackagediffs.py | 16:00 |
| al-maisan | noodles775: more or less | 16:01 |
| noodles775 | You're pushing up the line-count of LP there then ;) | 16:01 |
| === noodles775 changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [leonardr] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| al-maisan | noodles775: good point .. I can refactor and share the code. | 16:04 |
| noodles775 | al-maisan: great. | 16:05 |
| === henninge_ is now known as henninge | ||
| james_w | could I get a review of https://code.edge.launchpad.net/~james-w/launchpad/get-requested-reviews/+merge/18592 please? | 16:22 |
| === james_w changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [leonardr, james_w] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| james_w | it will make Daniel very happy | 16:23 |
| intellectronica | james_w: sure, i'll review it | 16:27 |
| thekorn | james_w, not that I have review powers, but there is a typo in xx-branchmergeproposal.txt: `Gettting` ;) | 16:27 |
| intellectronica | james_w: we don't use line continuation in the launchpad codebase. instead use parentheses to group different constituents of an expression appearing on different lines | 16:28 |
| intellectronica | i can see that the same problem appears in an existing lines. no idea why the author of that line decided to format it like that. if you feel like fixing that too, i won't complain :) (but don't feel obliged) | 16:29 |
| === jelmer_ changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [leonardr, james_w, jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| intellectronica | james_w: the tuple on line 103 of the diff is formatted a bit funny. why not just put it on one line? | 16:31 |
| intellectronica | james_w: also, maybe rename 'collection' in the same function to something more meaningful, like 'user_visible_branches'? | 16:32 |
| intellectronica | james_w: using title capitalization, isn't "Gettting Merge Proposals a Person has been Asked to Review" the correct form? | 16:34 |
| intellectronica | james_w: why the final comma on line 129? | 16:35 |
| intellectronica | james_w: other than that it all looks good | 16:35 |
| james_w | intellectronica: sorry, got caught in a conversation, thanks for the comments, I'll fix up now | 16:45 |
| intellectronica | cool | 16:45 |
| james_w | intellectronica: I'm happy with the logic etc. Do you have any concerns about things such as performance? | 16:45 |
| === rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| james_w | I'm guessing it's quite an efficient query, as it is based on that used to build +activereviews, but we will be using it for teams that may in the future have >100 reviews | 16:46 |
| intellectronica | james_w: no, i don't see anything that makes me worried. anything in particular you're concerned about? | 16:46 |
| intellectronica | rockstar! | 16:46 |
| rockstar | intellectronica, hi. | 16:46 |
| intellectronica | rockstar: i owe you guys a coke or something for making MPs generate correct diffs from dependent branches. it's wonderful! | 16:47 |
| rockstar | intellectronica, it's been doing that for a few months now actually. :) | 16:48 |
| rockstar | intellectronica, do you use pipes? | 16:48 |
| intellectronica | really?! | 16:48 |
| james_w | intellectronica: nothing in particular, but you have far more experience of this sort of thing than me | 16:49 |
| intellectronica | rockstar: i was always under the impression that it doesn't and pasted a diff manually whenever i had a serious of branches. | 16:49 |
| intellectronica | rockstar: and no, i don't use pipes yet | 16:49 |
| rockstar | intellectronica, if you use pipes (and thus have dependent branches), pipes has a lp-submit command that creates a merge proposal and sets the dependent branch and everything. | 16:49 |
| intellectronica | cool. i still have to check it out | 16:49 |
| al-maisan | noodles775: you have mail :) | 16:49 |
| noodles775 | al-maisan: checking | 16:59 |
| al-maisan | noodles775: thank you! | 16:59 |
| noodles775 | al-maisan: You don't need a TestProcessPendingPackageDiffsScript.setUp() anymore (it's just calling the superclass method which it will do by default). | 17:03 |
| al-maisan | noodles775: ah, I see. | 17:04 |
| al-maisan | let me remove it then | 17:04 |
| al-maisan | the same holds for the other test class | 17:04 |
| noodles775 | Yep, other than that, r=me. | 17:04 |
| noodles775 | al-maisan: ^^ | 17:04 |
| al-maisan | noodles775: thank you! | 17:04 |
| al-maisan | noodles775: http://pastebin.ubuntu.com/368974/ | 17:07 |
| noodles775 | Great. | 17:08 |
| intellectronica | BjornT: why are all the stub setup methods in the new test layer @profiled? | 17:31 |
| BjornT | intellectronica: just to be safe. i think that if they aren't there, the parent class' setup/teardown will be run twice | 17:35 |
| intellectronica | BjornT: oh? what does the profiling decorator have to do with that? | 17:36 |
| intellectronica | sorry, my question was a grammatical atrocity | 17:36 |
| BjornT | intellectronica: oh. they are there so that we don't forget to add them if we ever add some specific setup, and maybe to make the data make more sense (i.e. not omit some layers) | 17:38 |
| intellectronica | gotcha | 17:38 |
| intellectronica | BjornT: r=me | 17:39 |
| BjornT | thanks! | 17:39 |
| james_w | intellectronica: diff refreshed, could you look again please? | 17:40 |
| intellectronica | james_w: sure. looking | 17:41 |
| intellectronica | james_w: r=me | 17:43 |
| james_w | thanks | 17:43 |
| intellectronica | james_w: would you like me to land it on your behalf? | 17:43 |
| james_w | could you submit for me please? | 17:43 |
| james_w | yes please, thank | 17:43 |
| james_w | s | 17:43 |
| intellectronica | james_w: commit message? | 17:43 |
| james_w | intellectronica: set | 17:44 |
| intellectronica | james_w: fanks | 17:44 |
| james_w | no, thank you | 17:45 |
| intellectronica | are you correcting my english or being english? | 17:46 |
| leonardr | rockstar, feel free to ask me questions about my huge branch | 18:30 |
| rockstar | leonardr, yeah, I was getting ready to, still trying to take it all in. | 18:30 |
| === jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer,jamalta :)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jamalta :)] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | leonardr, does this actually give all the rest api versioning support needed to start versioning? | 18:39 |
| === jpds changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jamalta :), jpds] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| leonardr | rockstar: almost all of it. i am working on support for multiversioned mutators and destructors now | 18:40 |
| rockstar | leonardr, ah, okay. | 18:40 |
| leonardr | in earlier branches i added support for multiversioned collections and named operations | 18:40 |
| leonardr | this is the main branch for entry fields | 18:40 |
| rockstar | leonardr, why do you create a VersionedDict and then push None to it? | 18:40 |
| rockstar | Yea, I think I reviewed an earlier branch. | 18:40 |
| leonardr | rockstar: "None" is shorthand for the earliest version (we don't know the real name of the earliest version until the second zcml stage) | 18:40 |
| rockstar | leonardr, could you add a comment accordingly then? That just looks funky. | 18:41 |
| leonardr | so anything added to the VersionedDict will count as an annotation on the earliest version, until a specific version number is encountered | 18:41 |
| leonardr | rockstar, sure | 18:41 |
| leonardr | what file is that in? | 18:43 |
| leonardr | nm, found it | 18:43 |
| rockstar | leonardr, the tests here are really quite good. They've been a huge help in understanding the patch. | 18:43 |
| leonardr | thanks | 18:43 |
| leonardr | i had to write them to understand how to write the patch | 18:43 |
| rockstar | leonardr, I do see "[XXX leonardr I'll add these tests in my next branch; this branch is already way too large." What tests are those? | 18:45 |
| leonardr | there are a couple more paragraphs in those square brackets | 18:45 |
| leonardr | they are describing tests i need to write | 18:45 |
| === jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jpds] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | Ah, okay. Those paragraphs were ambiguous as to whether they were part of the documentation or if they were part of the comment. They make more sense in the latter context. | 18:48 |
| leonardr | that's why i did the square brackets... | 18:48 |
| leonardr | anyway, they won't last long | 18:49 |
| rockstar | Yea, I hadn't noticed the closing bracket. | 18:49 |
| rockstar | Thanks for being considerate on the patch size. I think making a note is probably okay for now. | 18:49 |
| rockstar | I'm assuming that there won't be any releases between the time this lands and the time that those other tests land. Am I safe in assuming that? | 18:50 |
| leonardr | rockstar: yeah, it's pretty certain | 18:51 |
| rockstar | leonardr, great. | 18:51 |
| rockstar | leonardr, r=me then. | 18:51 |
| leonardr | whew | 18:51 |
| rockstar | leonardr, would you mind if I also ask that you request me to review the followup branch? | 18:52 |
| leonardr | rockstar, sure | 18:53 |
| leonardr | i discovered today it's not going to be just more tests | 18:53 |
| leonardr | making multiversion mutators work is turning out to be very difficult | 18:53 |
| rockstar | leonardr, yeah, I think it might help for consistency's sake if I looked at the next branch as well. | 18:53 |
| leonardr | sure | 18:54 |
| === jamalta changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: leonardr || queue [james_w, jelmer, jpds, jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jelmer || queue [james_w, jpds, jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | jelmer_, ping | 19:03 |
| === rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: james_w || queue [jpds, jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | james_w, ping | 19:09 |
| james_w | hey rockstar | 19:09 |
| rockstar | james_w, where's the branch you need reviewed? | 19:09 |
| james_w | rockstar: oh, it's done, sorry | 19:09 |
| rockstar | james_w, great! That makes my life easier. :) | 19:10 |
| === rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jpds || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | jpds, ping | 19:11 |
| jpds | rockstar: Hi. | 19:11 |
| jpds | rockstar: I've fixed noodles' comments on https://code.edge.launchpad.net/~jpds/launchpad/fix_517020/+merge/18608 | 19:11 |
| rockstar | jpds, oh, you'll need to follow up with noodles then. | 19:12 |
| jpds | rockstar: Oh, right. | 19:13 |
| === jpds changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: - || queue [jamalta] || This channel is logged: http://irclogs.ubuntu.com || https://code.edge.launchpad.net/launchpad/+activereviews | ||
| === rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: jamalta || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | jamalta, you and me now. | 19:24 |
| jamalta | rockstar: hey! that was quicker than i thought | 19:24 |
| jamalta | did you have a chance to look at the bug? | 19:24 |
| rockstar | jamalta, looking. | 19:25 |
| jamalta | rockstar: alright | 19:25 |
| jamalta | based on Francis' reply, I'm thinking adding View classes in security.py for the missing interfaces should do the trick, right? | 19:25 |
| rockstar | jamalta, hm, I'm not sure if I'm the best person to have a pre-imp about that. | 19:36 |
| rockstar | leonardr, ^^ | 19:36 |
| jamalta | rockstar: oh alright | 19:37 |
| leonardr | rockstar: i don't know what you're talking about, looking at bug 517020 | 19:37 |
| jamalta | leonardr: bug #515761 | 19:38 |
| mup | Bug #515761: Anonymous API access to some collections returns nothing <Launchpad Foundations:Confirmed> <https://launchpad.net/bugs/515761> | 19:38 |
| rockstar | leonardr, honestly, I don't know what I'm talking about. | 19:38 |
| leonardr | jamalta: i can confirm that the problem is that lazr.restful is filtering out those items due to lack of permission | 19:41 |
| leonardr | i don't know enough about launchpad/zope to say how to fix it, but adding View classes seems reasonable | 19:41 |
| jamalta | leonardr: right, well adding View classes seems to work | 19:41 |
| leonardr | ok, cool | 19:41 |
| jamalta | so long as checkUnauthorized returns True | 19:42 |
| jamalta | (and checkAuthorized as well, of course) | 19:42 |
| jamalta | so are you ok with that solution? | 19:42 |
| jamalta | also, i can't figure out how to test anonymous api access | 19:42 |
| jamalta | all the tests dealing with the api seem to already be authenticated | 19:43 |
| leonardr | jamalta: in a pagetest you can use anon_webservice instead of webservice | 19:43 |
| jamalta | so that throws me off a bit | 19:43 |
| jamalta | leonardr: oh! thanks! | 19:43 |
| jamalta | that would be why i couldn't find it | 19:43 |
| jamalta | i was searching for anonym | 19:43 |
| jamalta | so are you good with this solution leonardr ? | 19:49 |
| leonardr | jamalta: yes, with the caveat that i don't know very much about that system | 19:49 |
| leonardr | if the security decision is 'make it visible to everyone', then what you describe should work | 19:49 |
| jamalta | leonardr: well, i haven't talked to anyone relating to security, but this data is available through unauthenticated users in the actual site | 19:51 |
| jamalta | is there someone i should ask about this? | 19:51 |
| jamalta | or should this wait until the review? | 19:51 |
| leonardr | jamalta: implement it and mention it as a topic for the review | 19:51 |
| === rockstar changed the topic of #launchpad-reviews to: on-call: rockstar || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| jamalta | leonardr: sounds good, thank you | 19:52 |
| jamalta | rockstar: thank you as well | 19:52 |
| === matsubara is now known as matsubara-afk | ||
| abentley | rockstar, I can has review? https://code.edge.launchpad.net/~abentley/launchpad/twisted-environ/+merge/18626 | 20:01 |
| rockstar | abentley, looking | 20:12 |
| rockstar | abentley, r=me | 20:14 |
| abentley | rockstar, thanks. | 20:14 |
| === salgado is now known as salgado-afk | ||
| EdwinGrubbs | rockstar: can you review a super simple branch for me? https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-516071-setup_add_member_handler/+merge/18629 | 20:24 |
| rockstar | EdwinGrubbs, looking. | 20:26 |
| jamalta | rockstar: ready for me again? | 21:01 |
| rockstar | jamalta, sure. | 21:01 |
| jamalta | rockstar: give me a min, forgot to finish my proposal | 21:01 |
| jamalta | and run lint for that matter | 21:02 |
| jamalta | rockstar: ok, now i'm ready.. sorry about that https://code.edge.launchpad.net/~jamalta/launchpad/515761/+merge/18638 | 21:05 |
| jamalta | wowah, not quite sure how i ended up committing wadl-testrunner.xml | 21:07 |
| jamalta | rockstar: i guess we can hold off on that, i have a few screw ups to take care of, sorry again | 21:09 |
| rockstar | jamalta, it's okay. | 21:10 |
| leonardr | rockstar, i have a very short branch continuing my series if you want to review it | 21:16 |
| rockstar | leonardr, sure. | 21:16 |
| leonardr | ok, writing mp now | 21:16 |
| leonardr | rockstar: https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-destructors/+merge/18640 | 21:24 |
| jamalta | rockstar: ok, this time i think i'm good | 21:35 |
| rockstar | leonardr, um, this branch has the changes from the previous branch. | 21:46 |
| leonardr | rockstar: argh, i haven't merged that yet | 21:47 |
| leonardr | i can give you a diff | 21:47 |
| leonardr | or give me am inute and i'll merge | 21:47 |
| rockstar | leonardr, if you set the dependent branch for the mp, it'll generate the diff properly. | 21:48 |
| leonardr | rockstar: i don't know how to do that. is that the same as 'merge into'? | 21:49 |
| leonardr | no, nm... | 21:49 |
| leonardr | no, i stand by my ignorance. how do i set that? | 21:49 |
| rockstar | leonardr, it looks like you can't set it after it's created. Lemme try to resubmit. | 21:50 |
| rockstar | Oh crap, that only made your comments go away... :/ | 21:50 |
| leonardr | rockstar: fortunately i have that page open | 21:51 |
| rockstar | leonardr, could you delete it and re-create it, and in the "more options" section, set the dependent branch there? | 21:51 |
| leonardr | rockstar: i've merged the other branch, so maybe it's now moot? | 21:51 |
| rockstar | leonardr, it won't update unless you push a new revision to the target branch. | 21:51 |
| leonardr | ok | 21:52 |
| rockstar | Er, push a new revision to the source branch. | 21:52 |
| leonardr | i'll just recreate it | 21:52 |
| leonardr | rockstar: perhaps this will work better | 21:53 |
| leonardr | https://code.edge.launchpad.net/~leonardr/lazr.restful/multiversion-destructors/+merge/18643 | 21:53 |
| rockstar | leonardr, thanks. | 21:54 |
| rockstar | Sorry it's a pain in the butt. We should be able to set the dependent branch after the fact. | 21:55 |
| jamalta | rockstar: i had a few changes that bac pointed out.. because of it have to change my proposal.. should i add it as a new comment or resubmit? | 22:39 |
| rockstar | jamalta, just add a new comment and push your changes. That should be fine. | 22:40 |
| jamalta | rockstar: salright | 22:41 |
| jamalta | alright, even | 22:43 |
| rockstar | jamalta, did you change wadl-testrunner.xml? | 22:48 |
| rockstar | Er, did you mean to? | 22:48 |
| rockstar | jamalta, it also says it has conflicts. Something is not right with that mp. | 22:49 |
| jamalta | rockstar: sorry, this mp https://code.edge.launchpad.net/~jamalta/launchpad/515761-anonymrelease/+merge/18641 | 22:51 |
| jamalta | i fixed those issues | 22:51 |
| jamalta | i pushed to the wrong branch by accident so i just made a new MP | 22:51 |
| === rockstar changed the topic of #launchpad-reviews to: on-call: - || reviewing: - || queue [] || This channel is logged: http://irclogs.ubuntu.com ||https://code.edge.launchpad.net/launchpad/+activereviews | ||
| rockstar | jamalta, hm, maybe you should have a single class that inherits from AuthorizationBase that deals with checkAuthenticated and checkUnauthenticated - There's a lot of repetitive code here. | 23:01 |
| jamalta | rockstar: i was thinking of doing that but no other class does that | 23:07 |
| rockstar | jamalta, well, that may be for hysterical reasons. I don't see why new code should be like that. | 23:08 |
| jamalta | and then there's the message on launchpad-dev from henning talking about refactoring security.py altogether | 23:08 |
| jamalta | rockstar: i could do that if you would prefer though | 23:08 |
| rockstar | jamalta, I say do it. Talk on a mailing list gets trumped by actual code. | 23:10 |
| jamalta | rockstar: haha, sounds good :) | 23:11 |
| jamalta | ViewByAnyUser a good name? | 23:12 |
| rockstar | jamalta, AnonymousAuthorization ? | 23:13 |
| jamalta | rockstar: even better | 23:13 |
| jamalta | rockstar: i'm going to push this now but have to head out so will hold off the review until tomorrow | 23:17 |
| jamalta | rockstar: thanks for the help and input :) | 23:17 |
| rockstar | jamalta, sure. | 23:17 |
| jamalta | rockstar: g'night | 23:18 |
| === jamalta is now known as jamalta-afk | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!