/srv/irclogs.ubuntu.com/2010/10/18/#launchpad-reviews.txt

thumperhttps://code.edge.launchpad.net/~thumper/launchpad/code-import-event-garbo/+merge/3867500:02
thumperlifeless: this branch adds garbo for code import evenst00:04
thumperlifeless: which I'd love to get cp'ed to loganberry asap00:04
mwhudsondrop table codeimportevent?00:12
mwhudsonit was kind of a nice idea, but never fully implemented and now it seems to be just a hindrance00:12
lifelessthumper: it looks fine to me; no need to cp it in a hurry though - just put the rev that it lands in on LPS special deployment instructions00:35
thumpermwhudson: useful really for machines, not for code imports themselves00:36
thumpermwhudson: I've been thinking of refactoring the code import model code to not use it00:36
mwhudsonyeah, that's true00:36
=== 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
lifelesshi henninge08:11
henningeHi lifeless ! ;)08:13
lifelessI have some parallel testing stuff that is moderately working08:14
lifelesswould you like to eyeball it ?08:14
henningelifeless: I can have a look ...08:14
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/uniqueconfig/+merge/38689 and https://code.edge.launchpad.net/~lifeless/launchpad/databasefixture/+merge/3869408:15
lifelessI'm not particularly worried about polish here - we've such a long trip to go, I'm primarily interested in *getting there* and then filling things out08:18
lifelessotoh I don't want to land stuff that isn't understandable either08:19
henningelifeless: for starters, why are you adding stuff to lib/canonical when the doctring says it's launchpad specific? ("Create a unique launchpad config.")08:21
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lifeless || queue: [lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
lifelesshenninge: because, for all intents and purposes, canonical.config *is* launchpad specific.08:22
lifelesshenninge: its a YAGNI to claim otherwise. lazr.config is generic, and its in a different tree08:22
lifelesshenninge: that said, my code is -so far at least- agnostic, so i can alter the docstring if you like08:23
henningelifeless: why not put it in lp.testing?08:25
lifelesshenninge: two reasons; its able to be used outside of test code, and its tightly coupled to the config concept, which lives in canonical.config08:25
lifelessit would split a single concept across the code base to split them up08:26
henningelifeless: do you know what tha project is called that holds the production configs?08:37
lifelesslaunchpad-production-configs08:38
lifelessbzr+ssh://bazaar.launchpad.net/~launchpad-pqm/lp-production-configs/trunk/08:38
henningeCheers08:38
henningelifeless: where does "EnvironmentVariableFixture" come from? I cannot find it in the fixtures source code?09:07
henningeah, you are using 0.3.209:07
bachi henninge09:31
lifelesshenninge: hey, so how are you going with those branches; I worry that I've sent you off into a tailspin or something :)10:31
henningelifeless: otp10:38
lifelessjml: you can see in backlog two branches for parallel testing; henninge has had them for a few hours; I don't know what outcome will be; you might be interested regardless, or if henninge is too busy, to just review em ;)11:02
lifelessgnight all11:02
jmllifeless: sure :)11:03
henningelifeless: sorry11:04
bachi henninge, will you have time for another review?12:22
bachenninge: unfortunately i probably won't be around to chat about it.  but if you do have time:12:23
bachttps://code.edge.launchpad.net/~bac/launchpad/bug-652149/+merge/3870512:23
=== bac changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lifeless || queue: [lifeless,bac] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
henningebac: I am off to lunch now but can look at it later ...12:24
bachenninge: ok12:24
=== Ursinha-afk is now known as Ursinha
=== jelmer_ is now known as jelmer
=== matsubara-afk is now known as matsubara
leonardrhenninge, if you get to it i'd appreciate a review of https://code.edge.launchpad.net/~leonardr/launchpad/oauth-doctest-to-unit-test/+merge/3871514:04
henningeleonardr: sure14:17
=== leonardr changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lifeless || queue: [lifeless,bac,leonardr] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
abentleyhenninge: could you please review https://code.edge.launchpad.net/~abentley/launchpad/detect-xen/+merge/38718 ?14:26
=== Ursinha is now known as Ursinha-afk
=== henninge changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: lifeless || queue: [abentley,bac,lifeless,lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== Ursinha-afk is now known as Ursinha
=== sinzui changed the topic of #launchpad-reviews to: On call: leonardr || Reviewing: lifeless || queue: [abentley,bac,lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bigjoolsjml: your comment about "If you change the constructor of NewBuildersScanner ..." makes no sense to me.  Can you explain please?15:22
jmlbigjools: the constructor currently does a query to populate current_builders15:23
jmlbigjools: basically, you don't have to do that15:23
jml*and*15:23
jmlyou don't have to have most of the code in startService15:23
jmljust schedule the NewBuilders... call15:24
jmland the first run will be exactly equivalent to the query in the constructor15:24
bigjoolsjml: ok - I don't want to change this right because there's a bug somewhere that causes the manager to crash when a new builder is detected15:25
bigjoolss/right/right now/15:25
bigjoolsI can address this with the bug if you think that's ok?15:26
jmlbigjools: sure.15:26
bigjoolswell I say crash - it stops dispatching builds15:26
bigjoolsbut the log continues as if there were no issues15:26
bigjoolswhich is utterly bizarre15:26
=== salgado is now known as salgado-lunch
=== Ursinha is now known as Ursinha-lunch
abentleyrockstar: could you please review https://code.edge.launchpad.net/~abentley/launchpad/detect-xen/+merge/38718 ?16:04
rockstarabentley, sure.16:04
abentleyrockstar: thanks.16:04
henningeleonardr: approved16:09
leonardrhenninge, great16:09
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [abentley,bac,lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
henningeabentley: you just found yourself another reviewer?16:09
henningeHi rockstar! ;-)16:09
abentleyhenninge: yes.16:10
henningeok, I'll move on to bac then.16:10
rockstarhenninge, hi!16:10
leonardrhenninge: i think i need to recreate request_token a lot because the lifespan of a request_token is not very long16:10
leonardrwell, i guess that's not what you were saying...16:10
leonardrhenninge: if i should use a mixin instead of a base class, but a mixin can't have a setup method, what should i do?16:26
henningeleonardr: I am just saying that this pattern (a TestCase base class) cannot be used if the base class defines test methods, too.16:27
henningeThe test machinery will try to run them ,too.16:27
henningeleonardr: for your case, you can leave it as it is. Sorry for the confusion.16:27
leonardrok, thanks16:27
leonardrhenninge: sorry, i'm going to keep bugging you16:28
leonardrwhat do you mean that that "key = " line is needed for all tests? i don't think it's ever used again16:29
leonardrdo you want that to be the consumer key for self.consumer?16:29
henningelet me look again, maybe I got mixed up.16:29
henningeI mean ... never mind.16:30
henninge^ignore that line16:30
henningeleonardr: yes, if you split the tests (in TestConsumerSet) up, you will need "key" in all but the "verifyObject" test.16:31
leonardrhenninge: i see what you mean. but, if i split up the tests, i can use self.consumer for all but the first one16:32
henningeleonardr: My main theme is that the tests can be split up.16:33
henningeEach test will need to be set up of course.16:34
leonardrok, i will split them up as much as possible and have you take another look16:34
henningeSo you'll have to decide if the objects you need for the tests to run are created in the test method or in the setUp.16:34
leonardrgot it16:34
henningegood ;-)16:35
=== henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: bac || queue: [lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== benji is now known as benji-lunch
=== Ursinha-lunch is now known as Ursinha
deryckadeuring1, hey, my MP is up to date now.  https://code.edge.launchpad.net/~deryck/launchpad/better-testing-for-status-changes/+merge/3855217:25
adeuring1deryck: ok, I'll look17:25
deryckadeuring1, I went with two tests, one for regular user and one for privileged users, even though there is some overlap because it read better.  And too avoid `if regular_user` type constructions.17:26
deryckadeuring1, but if you want one test with that type check, I can do that.17:26
adeuring1deryck: nah, I think that's fine17:26
=== henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [lifeless,lifeless, sinzui, sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
adeuring1deryck: r=me; two nitpicks17:47
deryckadeuring1, great, thanks!17:47
deryckadeuring1, ah, I forgot about the other celebrities.  Sorry.17:51
adeuring1deryck: no problem -- that's what reviews are for ;)17:51
deryckindeed! :-)17:51
bigjoolsjml: I pushed up a load of changes - I've not done everything you talked about, my will to live is slowly being sapped by this branch.  I've got some more changes to make tomorrow as described in the MP reply.17:52
jmlbigjools: sure17:53
jmlbigjools: I've got to go now, I think I've reviewed all of your changes from today though.17:53
bigjoolsjml: thanks muchly17:54
bigjoolsI am offski too17:54
=== benji-lunch is now known as benji
=== salgado-lunch is now known as salgado
=== matsubara is now known as matsubara-lunch
henningeleonardr: can you please give me an incremental diff?18:16
leonardrhenninge, sure18:16
leonardrhenninge_: http://pastebin.ubuntu.com/515781/18:41
leonardrsorry for the delay18:41
=== henninge_ is now known as henninge
abentleyrockstar: ping?18:55
rockstarabentley, hi18:56
abentleyrockstar: chat?18:56
rockstarabentley, sure.18:57
abentleyrockstar: Skype or mumble?18:57
rockstarabentley, skype18:57
=== bdmurray changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [lifeless,lifeless, sinzui, sinzui, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
=== matsubara-lunch is now known as matsubara
abentleysinzui, bdmurray: please ask someone to perform an on-call review before adding yourself to the on-call review queue.19:23
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: - || queue: [lifeless,lifeless, sinzui, sinzui, bdmurray] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
bdmurrayabentley: okay19:26
abentleybdmurray: thanks.19:26
abentleysinzui: I do not know what foaf means.  Can you please explain it?19:28
abentleysinzui: why are you writing the same module for each import, e.g. 25-26?19:30
sinzuiabentley, FOAF is an RDF vocabulary to describe a Friend of a Friend. It about 7 years old. It is one of the oldest RDF vocabs and many tools like python-rdflib support it out-of-the-bix19:30
abentleysinzui: What is Friend of a Friend?19:30
sinzuiabentley, that is a by-product of the script's expansion of identifiers identifiers19:31
sinzuiabentley, http://xmlns.com/foaf/spec/19:31
sinzuiabentley, our RDF for projects and people mix our proprietary vocab with standard vocabs19:32
sinzuiWe do not want to reinvent FOAF.19:33
sinzuiWe do not want to reinvent DOAP (Description of a Project), but DOAP does not define concepts like series19:33
sinzuiSo we ignore DOAP19:34
abentleysinzui: Several of your examples say "Accoun" rather than "Account".19:36
sinzuioh!19:36
sinzuiI think that means the script is bad19:36
sinzuiabentley, I ran the entire test suite without error Sunday!19:37
abentleysinzui: I guess you need to make sure those scripts are being run, then...19:38
sinzuiabentley, sorry. I just had a panic. I was thinking of my apocalypse branch. That is importing correctly.19:41
abentleysinzui: Ah, sorry.19:41
=== matsubara is now known as matsubara-afk
abentleysinzui: for the apocalypse branch, have you considered updating the script to avoid repeating modules?  It seems like a small effort to avoid lots of work later.19:42
sinzuiabentley, I did and considered it an extra 4 hours to run and test to fix imports in tests I would rather delete19:43
sinzuiI certainly will investigate sorted multiline imports it you want me too19:44
abentleysinzui: would you please?  I agree that it would be nice to convert/delete these.19:47
sinzuiokay19:48
sinzuiabentley, I must have been running tests in the wrong branch. This RDF branch has another idiotic problem be defining a user as a group. the tests rightly fails20:00
sinzuis/be defining/by defining/20:00
abentleysinzui: Ah.  Okay.  Please ping me when it's working.20:01
=== abentley changed the topic of #launchpad-reviews to: On call: abentley || Reviewing: bdmurray|| queue: [lifeless,lifeless] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews
abentleybdmurray: was there a preimplementation call for https://bugs.edge.launchpad.net/malone/+bug/114766 ?20:08
_mup_Bug #114766: Only bug supervisor should be able to nominate a bug for a release <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/114766>20:08
abentleybdmurray: or rather https://code.edge.launchpad.net/~brian-murray/launchpad/bug-supervisor-nominate-for-release/+merge/38733?20:08
bdmurrayabentley: yes, deryck and I discussed it20:09
abentleybdmurray: it's helpful to note that in your merge proposall.20:09
abentleybdmurray: did you check lint?20:09
bdmurrayabentley: no, doing so now20:10
abentleybdmurray: If you install lpreview_body, then lp-propose will supply a cover letter template with the lint stuff filled in.20:11
bdmurrayabentley: where is lpreview_body?  I hadn't heard of this20:11
abentleybdmurray: it's in the Launchpad team PPA and on Launchpad.20:13
abentleybdmurray: https://edge.launchpad.net/lpreview-body20:14
abentleybdmurray: on line 9, why are you checking target.bug_supervisor before doing check_permission("launchpad.Driver", target)?20:16
bdmurrayabentley: hmm, that does seem backwards20:18
abentleybdmurray: The idea is that the BugDriver or bug supervisor can always do this, but if the bug supervisor is set, normal users cannot?20:20
bdmurrayabentley: yes, that is correct20:20
abentleybdmurray: that seems a) complicated and b) like it could be provided as a permission.20:22
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/databasefixture/+merge/3869420:23
abentleybdmurray: it also means error messages that say "Only bug supervisors can nominate bugs." are not entirely accurate.20:24
bdmurrayabentley: okay, its not clear to me what you mean by providing it as a permission20:26
abentleybdmurray: I mean "launchpad.NominateBug" as a permission that encapsulates the logic you're doing in the view code.20:27
bdmurrayabentley: hmm, if you look at security.py there is a "launchpad.BugSupervisor" permission that checks to see if the user is in obj.owner or in_admin. so perhaps the check_permission for launchpad.Driver is redundant?20:30
abentleybdmurray: lemme see...20:31
sinzuiabentley, I fixed the two tests and fixed the person-rdf template: https://code.launchpad.net/~sinzui/launchpad/rdf-links-1/+merge/3857320:31
abentleybdmurray: It seems like launchpad.Driver is meant to be "launchpad.NominateBug", doesn't it?20:33
abentleysinzui: r=me.  There are some blank lines introduced at 64.  If they were not added deliberately, you may want to remove them before submitting.20:34
bdmurrayabentley: I was under the impression that sinzui was reluctant to add lots of permissions20:34
sinzuiabentley, thanks. I also need to fix the 2-space indentation in several of the tests. I will fix blank lines too20:35
abentleybdmurray: I don't know that sinzui's preferences are authoritative here.  My own preference is to have a single point of truth.  We can't reuse that browser code everwhere we might want to check that permission.20:37
bdmurrayabentley: to be clear this is the modification I'm suggesting https://pastebin.canonical.com/38756/20:39
abentleybdmurray: in fact, you could eliminate most of 100-113 if you did that.20:39
sinzuibdmurray, abentley. We should not introduce new kinds of permissions [flacoste]. But we have always had bug_supervisor and it is not clearly defined in security.py, so I advocate defining it so I do not need to guess20:39
abentleysinzui: I am getting confused about the difference between launchpad.Driver and this.  Is it the ability to unset bugs that would differentiate launchpad.Driver?20:41
sinzuiabentley, bdmurray: I believe there is agreement that models can define security checks that are imported in security.py20:41
abentleysinzui: I would be fine with that.20:42
sinzuibug_supervisors are a specialisation of drivers.20:42
sinzuiI think (because there is know authoritative checker) that bug_supervisors are effectively drivers with elevated permission to manage bugs20:43
sinzuiThere is no requirement that a bug_supervisor be a driver, or vice-versa20:43
abentleysinzui: This permission would allow all to nominate bugs unless a supervisor is set, in which case only the supervisor and driver would be able to.  Does that seem reasonable?20:45
bdmurrayabentley: all can already nominate bugs for a release, this restricts it if a bug supervisor is set20:46
abentleybdmurray: that is what I was trying to day.20:46
abentleys/day/say20:46
bdmurrayokay cool20:46
sinzuiabentley, I must defer to deryck, gmb, and allenap on that issue. I certain do not want anyone to ever make a nomination in any project I run20:47
deryckyes, I feel convinced after talking to bdmurray that nominations are useless since anyone can make them.20:48
* sinzui looks for a bat to hit someone every time someone uses +nominate to vote for his personal issue20:48
abentleyderyck: if so, is it a good idea to allow anyone to make nominations if the bug supervisor is unset?20:49
sinzuiAnd why can someone nominate to fix a bug in trunk or an obsolete series? +nominate only works for projects that create forward series or have to manage backports to maintained series20:50
deryckabentley, yeah, that's a good point.  I think bdmurray was trying to have as little impact as possible on other projects.20:51
deryckabentley, bdmurray -- I think only driver or bug supervisor should be able to nominate for series.20:51
deryckwe ACL assign to milestone, why not series?20:51
bdmurrayderyck: drivers have their nominations approved automatically so that effectively removes nominations if there is no bug supervisor20:52
deryckbdmurray, right20:52
deryckthat's how milestones work.  Shouldn't this be the same?20:52
bdmurraybut nominations are a way of escalating (in my mind) a bug report.  this removes this escalation path for projects w/o bug supervisors20:53
bdmurrays/this removes/this would remove/20:53
* deryck is thinking....20:54
abentleybdmurray: I don't know what nominations are, but according to the bug, normal people don't know how to use them correctly.20:54
deryckI don't think anyone actually uses them as escalation.  ubuntu users do, but that's why we're adding this restriction. :-)20:55
derycksinzui, do you see any issue in treating series like milestone, and not make it available at all if you're not the owner or bug supervisor?20:56
sinzuideryck, I think they should be the same20:57
deryckok, cool.20:57
sinzuideryck, I think they are not the same because we implemented 2, not 1, way of planing bugs and features20:58
deryckbdmurray, since sinzui and I are agreed, I think we should do as abentley suggests and make this only for owners and bug supervisors.20:59
derycksinzui, right, agreed20:59
bdmurrayokay, got it21:00
sinzuiI favour one implementation that allows users to say this should be fix by z, or is inclusive in x without needing to know how milestones or series are implemented21:00
derycksinzui, I get what you mean now.  but until we can get to that, do you have any concerns about having series behave slightly more like milestones in this way?21:01
sinzuino. similar is easier to learn21:01
deryckok, cool.21:02
deryckthanks21:02
sinzuideryck, if we made series "look like" milestones, I could target a bug to series and then undo it!21:02
deryckindeed!  Though that wouldn't be covered by bdmurray's current work.21:02
deryckok, so I need to bail for the day.21:03
deryckCatch you all tomorrow.21:04
bdmurrayabentley: so is okay to use check_permission in model/bug.py to cleanup lines 100-113 ?21:05
rockstarabentley, https://code.edge.launchpad.net/~rockstar/launchpad/merge-queues-model/+merge/3876221:20
abentleybdmurray: works for me.21:20
henningeleonardr: Hi! Now I know what got me confused - you did not push you changes.21:35
henningeleonardr: but what I see in the incremental diff looks good ;-)21:36
=== salgado is now known as salgado-afk
rockstarabentley, hello.22:05
abentleyrockstar: hi.  OTP22:05
rockstarabentley, ack. Just wanted to say "Can I please get a review?"  That is all.  :)22:06
=== 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
abentleyrockstar: You need another blank line at 7322:12
rockstarabentley, indeed I do.22:13
abentleyrockstar: I would rather see BranchMergeQueue.new as a classmethod, not a staticmethod, so you don't need to repeat the classname.22:16
rockstarabentley, ack.  Will make that change as well.22:16
abentleyrockstar: r=me with those changes.22:19
rockstarabentley, thank you sir!22:19
=== Ursinha is now known as Ursinha-afk
wallyworld_lifeless: you may want to claim this one https://code.edge.launchpad.net/~wallyworld/launchpad/fix-hardcoded-test-urls/+merge/3877923:52
wallyworld_i decided to land it independent of your uniqueconfig one - i can come back and fix the hard coded config name once both have landed23:53
lifelessthats great23:53

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