#launchpad-reviews 2010-03-29
* wgrant changed the topic of #launchpad-reviews to: on call: - || reviewing: - || queue: [wgrant(bug-540819-fix-builder-list-icons),adiroiban(bug-548999)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: wgrant(bug-540819-fix-builder-list-icons) || queue: [adiroiban(bug-548999)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: adiroiban(bug-548999) || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> on call: abentley || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> EdwinGrubbs: I had a few questions about your branch. I think in general it is ready to land: https://code.edge.launchpad.net/~edwin-grubbs/launchpad/bug-490593-configure-involvement-portlet/+merge/21524
<sinzui> I can change the status after your reply
<EdwinGrubbs> sinzui: the relation between the "Report a bug" link and the "Configure bug tracker" link is obvious, but a user might not understand that the "Ask a question" will be enabled with "Configure support tracker". This is one reason it was simpler to just put edit buttons next to the original links instead of at the bottom where they need their own verbage. Perhaps, "Ask a question" should be changed to "Get support" or "Ask
<EdwinGrubbs> for support".
<sinzui> EdwinGrubbs: yes. true. You had remarked in the review earlier that you did not know what to do with questions since other projects do not have them. This is wrong since answers is a support trackers and lots of project and companies have the,
<sinzui> EdwinGrubbs: We do not need to change the term, but that term is more understandable for all projects, not just the few that use Launchpad for hosting
<sinzui> EdwinGrubbs: The matter is not important since support tracking is not important to Ubuntu
<EdwinGrubbs> sinzui: Here is the full list of link name pairs, since I don't want to fix all the tests until these are agreed upon: "Report a bug"->"Configure bug tracker", "Ask a question"->"Configure support tracker", "Help translate"->"Configure translations", "Submit code"->"Configure upstream branch", "Register a blueprint"->"Coinfigure blueprints"
<sinzui> EdwinGrubbs: s/Coinfigure/Configure/
<sinzui> EdwinGrubbs: "Configure upstream branch" is strictly from a distro perspective, not the project. We can go back to "project branch" or be correct in launchpad terms and mean "development branch"
<sinzui> EdwinGrubbs: I do not think we need to speculate on the terms futher until we get user feedback on edge.
<EdwinGrubbs> sinzui: I'll use "Configure project branch" then, since "development branch" could mean anything as opposed to "development focus branch"
<sinzui> okay
* abentley changed the topic of #launchpad-reviews to: on call: abentley || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> on call: - || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
#launchpad-reviews 2010-03-30
<leonardr> abentley, can you take a look at https://code.edge.launchpad.net/~leonardr/launchpad/launchpadlib-pagetests-take-2/+merge/22444 ?
<wgrant> leonardr: The catastrophic test failures did actually occur on EC2, buildbot and locally. But they were sufficiently catastrophic that they killed the test suite in such a way that the failure wasn't detected by the outer test process.
<mwhudson> leonardr: did you see https://code.edge.launchpad.net/~mwhudson/launchpad/aiee-everything-is-broken/+merge/22185 ?
<mwhudson> mind you i think your branch is mostly better
<leonardr> mwhudson: i hadn't seen that. it sucks that we duplicated work
<mwhudson> leonardr: well, if i'd actually run the tests after my last commit on friday...
<leonardr> mwhudson: shall i go ahead with my branch? is there anything you'd like me to change?
<mwhudson> leonardr: the work i did since friday took about 5 minutes, so i don't mind really
<leonardr> ok
<mwhudson> leonardr: i didn't see anything to change, let me have another look
<leonardr> all right
<leonardr> you can do the review if you want, i haven't heard from abentley
<wgrant> Looks like he tried to unassign himself from the topic many hours ago, but instead spoke the new topic into the channel.
<leonardr> ahh
* leonardr 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
<mwhudson> leonardr: done
<leonardr> mwhudson: fyi, the xx-wadl thing
<leonardr> previously there was a pre-cached 'testrunner wadl file' which was used in xx-wadl.txt
<leonardr> if you tried to use launchpadlib from a pagetest, it would pick up the testrunner wadl file and believe that launchpad had no capabilities at all, since the testrunner wadl file was basically empty
<mwhudson> leonardr: ah
<mwhudson> that makes sense
<leonardr> so i removed the testrunner wadl file, but then i needed a bigger test to prove that launchpad's wadl caching worked
<wgrant> The WADL's not generated for every test, right?
<leonardr> the wadl is generated when it's requested and not present on disk
<leonardr> and not present in the in-memory cache
<wgrant> Great.
<leonardr> that's one wadl generation per version requested per launchpad restart
<wgrant> Excellent.
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> morning everyone.  bring out yer dead.
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: I have a short branch when you have time. I will have spm test it to verify we want to land the trivial fix.
<salgado> bac, can you review https://code.launchpad.net/~salgado/launchpad/bug-547054/+merge/22460 for me?
* salgado changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [sinzui,salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> sinzui:  are you seeking a r-c for your branch?
<sinzui> no
<bac> ok, i'll mark it approved then
* bac changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> some brave soul might want to review https://code.edge.launchpad.net/~jml/launchpad/ssh-key-auth/+merge/21801 -- but please don't feel obliged
<jml> I'll look at it later to try to break it down into smaller branches.
<jml> mentioning in case someone would prefer to spare me the trouble
<salgado> thanks for the review, bac. :)
<bac> salgado: np
<bac> jml: is it mostly mechanical?
<jml> bac, sadly no.
<bac> jml: ick
<jml> bac, I mean, a lot of it is just moving stuff into another place and then tweaking it a little
<jml> bac, but it deserves quite a thorough review, IMO.
<bac> jml: right
<bac> jml: sadly for you i was experimenting with using a smallest-first approach to +activereviews today
<jml> bac, heh, that's ok :)
<jml> bac, I'll leave it in the IRC backlog just in case someone is feeling really really generous.
* sinzui changed the topic of #launchpad-reviews to: on call: bac || reviewing: -|| queue: [sinzui] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<sinzui> bac: https://code.launchpad.net/~sinzui/launchpad/auto-yui-tests/+merge/22485 could unblock you
<bac> sinzui: and you move to the front of the queue
<bac> well, you were already at the front
<sinzui> I am also at the back of the que :(
<sinzui> And I lost a "ue"
<bac> potato/potato
<sinzui> potatoe!
<sinzui> thanks you happy campers
 * sinzui ponders how two Bush presidents could pick such diametrically opposed VPs
<bac> sinzui: your yui test runner only works for lp.registry, right?
<sinzui> yes
<bac> but my JS for product series was moved to lp.code.
<bac> :(
<sinzui> I can make a small change to include other tests
<bac> yeah, we'll have to.  but it would've been nice to keep it isolated
<sinzui> I need to set the  layer dynamically
<sinzui> Subclass the test runner for code
 * sinzui could move the test class to lp.testing
<sinzui> bac: Do you want me to move the class into lp.testing then subclass it for registry?
<bac> yes i guess that makes the most sense.
<bac> then i'll do it for code
<sinzui> or move into lp.testing and dynamically set the test name/layer part
<bac> the former makes more sense to me
<sinzui> okay
<sinzui> bac ping
<bac> hi
<sinzui> bac: I moved the base test class to lp.testing...
<sinzui> I got a circular import for canonical.launchpad.testing.pages...
<bac> ok, can you paste a diff and put a link in the MP?
<bac> doh
 * bac hides
<sinzui> https://pastebin.canonical.com/29881/
<sinzui> bac: I think paste of the actual code is easier to read
<sinzui> https://pastebin.canonical.com/29883/
<bac> yeah, the diff is a mess since the file got gutted
<bac> but the change looks great
<sinzui> I think I know how to make timelines work...
<sinzui> maybe I'll do that for edwin to review tomorrow
* bac 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
#launchpad-reviews 2010-03-31
<thumper> sinzui: timelines?
<thumper> sinzui: as in project and person timelines?
<sinzui> bac: ping
<bac> hi sinzui
<sinzui> spm tested my cache-country-mirrors fix. The fix revealed another problem that was also a known bug. This is my permission fix: https://pastebin.canonical.com/29899/
<bac> sinzui: were the permissions wrong on the orginal file or did shutil.move not copy them properly?
<sinzui> The original file I assume since this is a bug that was already reported
<sinzui> The dir mask was correct though
<bac> ah, ok.  r=bac
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: -|| queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<bac> sinzui: it doesn't look like you pushed your changes to the auto-yui branch
<sinzui> bac: pushed
<bac> thx
<allenap> EdwinGrubbs: Can you take a look at my branch please? https://code.edge.launchpad.net/~allenap/launchpad/dynamic-batch-size-bug-546085/+merge/22538
* allenap changed the topic of #launchpad-reviews to: on call: Edwin, allenap || reviewing: -, - || queue: [-] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<EdwinGrubbs> allenap: sure
* allenap changed the topic of #launchpad-reviews to: on call: Edwin, allenap || reviewing: -, - || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> jml: I can review your ssh-key-auth branch. As far as I can tell, only the first 156 lines are worth reviewing; the rest are sampledata changes. Does that sound right?
<jml> allenap, uhhh, no.
<jml> allenap, it's actually about 4200 lines of relevant diff. It needs to be re-proposed to have the correct merging though.
<jml> allenap, and I'm convinced that I can do it with fewer lines of diff by being smart about renaming
<jml> allenap, thank you very much for the offer though.
 * jml takes it off the list
<allenap> jml: Okay, cool :)
<allenap> Thanks EdwinGrubbs :)
* EdwinGrubbs changed the topic of #launchpad-reviews to: on call: Edwin, allenap || reviewing: -, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<abentley> jtv, I'm almost inclined to think makeCurrentUpstream and makeCurrentUbuntu should be the same function with different parameters.
<jtv> abentley: yes, they are probably going to become wrappers for the same meta-function
<jtv> But we'll see what shape it takes before we try to get clever.
<abentley> jtv, you could use storm.locals rather than storm.store, which would simplify future imports a bit.  What do you think?
<jtv> abentley: oh, is storm.store obsolescent?
<abentley> jtv, no, but storm.locals is meant to provide access to everything you need.
<jtv> abentley: I didn't knowâwhy and how is that?
<abentley> jtv, "why" is to simplify imports; most everything a Storm client needs can be imported from there.  I assume for "how", they just import from the relevant modules and list things in __all__.
<abentley> jtv, r=me either way.
<jtv> abentley: then sure, I'm pushing the change now.
<jtv> abentley: thanks!
<leonardr> salgado, can you review the incremental diff of https://code.edge.launchpad.net/~leonardr/launchpad/launchpadlib-pagetests-take-2/+merge/22444 ? it's all oauth stuff
<salgado> leonardr, is that r10609?
<salgado> iow, how do I get the incremental diff?
<leonardr> salgado: i pasted it in the mp
<salgado> oh, the pastebin
<salgado> +    return context
<leonardr> salgado: the function wasn't returning its result
<salgado> leonardr, I suppose that's fixing an oversight from a previous revision?
<leonardr> right
<salgado> ok, looks good to me
* allenap changed the topic of #launchpad-reviews to: on call: Edwin || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<allenap> EdwinGrubbs: Are you still around to do an sort-of-emergency review? https://code.edge.launchpad.net/~allenap/launchpad/debbugs-should-not-sync-comments-bug-552725/+merge/22566
<EdwinGrubbs> allenap: sure, I can do that.
<allenap> Thanks!
<allenap> EdwinGrubbs: Actually, can you hold on that; I've realised I've made a mistake.
<allenap> EdwinGrubbs: Actually, it's okay. Sorry for the confusion.
<EdwinGrubbs> allenap: your test says that debbugs supports back-linking, but the implements() doesn't contain ISupportsBackLinking.
<allenap> EdwinGrubbs: Ah, yes, you're right; I just counted three interfaces. Wow, you're thorough :)
<EdwinGrubbs> allenap: you just need to set the module docstring in the test file. r=me
<allenap> EdwinGrubbs: Thanks!
#launchpad-reviews 2010-04-01
<jtv> wgrant: how does the self.home[1:] work?  Just strip off the leading slash to get a relative path?
<wgrant> jtv: Right. There's probably a better way to do that, though.
<jtv> wgrant: my first choice would probably have been re.sub, but I guess a key point is that there's nothing sensible to do in cases where it's a relative path.  So I'm thinking an assertion may be enough to turn weird "known path with missing letter" errors into clearer ones.
<jtv> o-kayyyy... feature request for lucid: plug-n-play USB mouse support.
<wgrant> jtv: It works. Restart X -- you probably had an incompatible evdev driver update.
<wgrant> So it will have tried to dynamically load the driver, and failed.
<jtv> I'll do without for now.  The macbook pro has a wonderful huge, buttonless touchpad.  Now if only the "disable touchpad while typing" worked a little more reliably.  :)
<jtv> wgrant: there is a test for updateBuild_WAITING...  I'd guess that all it needs is a tarball to upload and you'll have an integration test that exercises the _uploadTarball fix.
<wgrant> jtv: Oh, I forgot you actually had useful tests.
<jtv> \o/
<wgrant> Soyuz is rather deprived.
<jtv> Rather.  :)
<jtv> It's funny how that seems to be infectious.  Sometimes in Wellington I heard things like "we'll know when it breaks."  But me, I often find myself stumped by _how_ it breaks when it does.
<jtv> Have a look at lib/lp/translations/tests/test_translationtemplatesbuildbehavior.py, test_updateBuild_WAITING
<wgrant> jtv: That explicitly clobbers _uploadTarball.
<wgrant> It sounds like I'm going to need to set up a real branch with translation-capable projects attached.
<jtv> wgrant: hmm... yes, that sounds very true.
<jtv> I don't know of any good way to inject "pretend this is the project we're looking for" into the utility.
 * wgrant doesn't actually know how Rosetta works.
<wgrant> It's the one part of LP that I remained completely ignorant of until Saturday.
<jtv> wgrant: you could consider cross-breeding this test with an existing one for _uploadTarball.
<jtv> wgrant: then welcome!
<wgrant> jtv: Is there an existing one for _uploadTarball?
<jtv> Has to be...
<jtv> test_uploadTarball may be our baby; same file.
<wgrant> Oh. That's conveniently short.
<jtv> Most of the meat is in the setup.  :-)
<wgrant> yeah.
<wgrant> Any hints?
<jtv> You need all of the setup.
<jtv> Whereas the updateBuild_WAITING test does not rely on any implicit setup at all.
<wgrant> Although it does need makeBehavior.
<jtv> So I'd create the integration test by copying test_updateBuild_WAITING right behind the _uploadTarball test.
<wgrant> Ah, but that's in the mixin.
<wgrant> Didn't notice that.
<wgrant> That's easy, then.
<jtv> yay for mixins  :)
<jtv> It's already in the ancestry of the test class we're looking to extend, I believe.
<wgrant> Exactly.
<jtv> So you can copy test_updateBuild_WAITING in there, remove all the assertions, and tack on the final paragraph from test_uploadTarball.
<wgrant> Yep.
<noodles775> stub: Do you know if there is any performance benefit to using CREATE TABLE AS SELECT... to create and populate a table from *lots* of existing data, as opposed to the normal method of create table, then a migration function to iterate and insert lots of data?
<wgrant> Pfft, it's only 1.6 million rows.
<noodles775> Just interested :)
<stub> inserting rows one at a time is slower. Inserting them in bulk is faster. Building the indexes after populating the table with data is a win.
<noodles775> Thanks.
<stub> CREATE TABLE AS SELECT is the same as CREATE TABLE; INSERT INTO [...]
<noodles775> stub: except that the indexes are added afterwards right? (and the INSERT INTO is * 1.6 million for the data migration).
<noodles775> stub: here's a very early WIP that I was toying with: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-build-generalisation-db-changes/+merge/22527
<stub> Indexes are created when you create them. You might create them as part of the CREATE TABLE statement, or you might create them after.
<noodles775> Right.
<noodles775> stub: OK, so there's no difference between the above create table as select... and the equiv. create table, followed by a function that does 1.6 million inserts into those tables, as long as the indexes are created afterwards in both cases?
<noodles775> s/tables/table
<stub> Yup. But 1.6 million doesn't take long anyway unless the rows are particularly wide.
<noodles775> Yep. Thanks for the info!
<noodles775> jtv: I know it should be the other way around, but do you mind r or rs'ing a trivial testfix for the db-devel failure: https://code.edge.launchpad.net/~michael.nelson/launchpad/db-test-fix-timebomb/+merge/22595
<jtv> noodles775: hadn't noticed the failure, but hang on, I'm looking
<noodles775> Thanks. It's what's holding up devel being re-synced after the rollout.
<jtv> noodles775: when you say it should be the other way around, do you mean I/we broke it?
<noodles775> jtv: no, I should be reviewing right now :/
<noodles775> But there's just been a whole bunch of un-expected things, so I haven't started yet.
<jtv> noodles775: well you couldn't have reviewed your own, and I don't recommend sitting around waiting for someone else to fix it either.  :)
<jtv> same here
<jtv> noodles775: one question: doesn't this risk undermining the test?  A ... could be hiding an entire line of output.
<noodles775> Yes, if an extra line suddenly appeared in the output, it would be swallowed.
<noodles775> Any other easy fixes?
<jtv> Either leaving the date out of the output altogether, I guess, or replacing it with a yes/no
<noodles775> OK, doing so now.
<jtv> thanks for bearing with me.
<jtv> wgrant: any luck with that test?
<wgrant> jtv: Sorry, got distracted.
<wgrant> Yes, it works.
<jtv> cool!
 * jtv always feels a warm glow whenever integration-testing of the build farm in the test suite is extended
<jtv> wgrant: not showing up in the diff yet though
<wgrant> jtv: No, just pushed now.
<wgrant> I wrote it, fired off the tests, and forgot.
<jtv> wgrant: you're just not pushy enough :P
<wgrant> Heh.
<jtv> (I assume there's an implicit "committed" in that story)
<wgrant> Yes.
<jtv> Just being pedantic, never mind me anyway
<wgrant> jtv: Can you think of a way to test the slave change?
<wgrant> I'm not sure of what tests you have around there.
<jtv> wgrant: maybe you could do a test where you fake addWaitingFile, and see that it gets a sensible path?
<jtv> Feel free to extend FakeMethod to record its last args and kwargs.
<wgrant> jtv: I was hoping there was existing infrastructure around there.
 * jtv searches
<jtv> Not really infrastructure, but if you extend FakeMethod in this way, then lib/canonical/buildd/tests/test_translationtemplatesbuildmanager.py already fakes gatherResults for testingâshould provide a usable base.
<noodles775> jtv: I'm push the changes, but the MP hasn't updated yet.
<jtv> noodles775: thanks
<noodles775> jtv: http://pastebin.ubuntu.com/407482/
<noodles775> That's the incremental.
<jtv> noodles775: approved.
<noodles775> jtv: Thanks.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775 || reviewing: wgrant || 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: noodles775 || reviewing: wgrant || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* ctrlsoft changed the topic of #launchpad-reviews to: on call: noodles775,jelmer || reviewing: wgrant, jtv || 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: noodles775,jelmer || reviewing: mvo, jtv || queue: || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: noodles775,jelmer || reviewing: mvo, jtv || queue: [wgrant,wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<mrevell> Hello. I have a fairly simple text change branch, if anyone would like to review it! https://code.edge.launchpad.net/~matthew.revell/launchpad/maintenance-notice
<noodles775> Sure! Just add it to the queue pls.
<noodles775> wgrant: would you have time to go over a review I'm doing and adding any comments (I'll be finished in a minute, but I'd like a second opinion from someone more familiar with germination etc.)
<noodles775> s/adding/add
<noodles775> And it doesn't need to be today.
<wgrant> noodles775: Sure. mvo's?
<noodles775> wgrant: thanks, yep. I'll be finished soon and will add you as a reviewer.
* ctrlsoft changed the topic of #launchpad-reviews to: on call: noodles775,jelmer || reviewing: mvo, wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<ctrlsoft> hmm, this is my third week of reviews and so far I've only done soyuz reviews for some reason :-)
<noodles775> ctrlsoft: yeah, and active reviews is all soyuz stuff too!
<ctrlsoft> wgrant: do you need me to land move-lots-of-buildd-master-to-queuebuilder ?
<noodles775> wgrant: ok, I've posted my comments and requested a review from you.
<wgrant> ctrlsoft: When PQM opens, that would be lovely.
<wgrant> For now, maybe just mark it Approved so it drops off +activereviews.
<wgrant> Thanks.
<wgrant> Ah, you already have.
* noodles775 changed the topic of #launchpad-reviews to: on call: noodles775,jelmer || reviewing: mrevell, wgrant || queue: [wgrant] || 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,jelmer || reviewing: lunch, wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<wgrant> noodles775: I don't like the way it uses python-apt when it could just use internal LP APIs, but I guess that's reasonable given that it's not really part of LP.
<wgrant> Otherwise it's fine.
<cjwatson> noodles775: I'd like to work on fixing bug 294846, since it's annoying me.  Do you have a moment for a pre-impl chat?
<mup> Bug #294846: Setting to Won't Fix is ACLed but unsetting it isn't <ubuntu-qa> <Launchpad Bugs:Triaged> <https://launchpad.net/bugs/294846>
<wgrant> Is it actually desirable? It's unprecedented.
<cjwatson> How so?  We have existing ACLs
<wgrant> Nothing that prevents users from rectifying triage mistakes and reopening bugs.
<wgrant> (there's of course the obvious problem that that sometimes isn't wanted, but...)
<cjwatson> The current pattern is: developer says they don't intend to fix a bug, and rather than trying to persuade them that they're wrong in comments, the user just slams it back to New
<cjwatson> repeat until very bored
<wgrant> Well, there's discussion about introducing a new status for that, which looks less obviously closed.
<wgrant> So users will hopefully not try to reopen it.
<cjwatson> That seems like a silly workaround to me
<wgrant> Unless Mark starts Won't Fixing my bugs without comment again :P
<cjwatson> users will sooner or later figure out that it means closed, and then we're back where we started
<wgrant> Hmm.
<cjwatson> reopening bugs rather than engaging in discussion with the developer is a community anti-pattern
<wgrant> Indeed.
<cjwatson> the Debian BTS even says in the mail you get when a bug is closed that if you think it's wrong you should discuss it with the developer, who'll reopen if needed
<cjwatson> that said I don't think people should be prevented from reopening Fix Released - Won't Fix is different because it's generally the result of a genuine difference of opinion, in which case simply reopening it is unlikely to be useful
<cjwatson> still, if it's controversial, then I suppose a pre-impl chat is premature
<wgrant> https://dev.launchpad.net/LEP/CloseBugsLeaveDiscussionsOpen
<cjwatson> aha
<cjwatson> bizarre summary though.  Discussion remains open on Won't Fix bugs too.
<cjwatson> I'll just link to that from the bug, then
<wgrant> Yes, I thought that a little odd too.
<cjwatson> distinction between socially open and technically open, I suppose
<cjwatson> While I'm here, is it reasonable/acceptable/common to submit Launchpad patches having run *only* EC2 tests, not local tests?  I find that I don't have the resources at the moment to keep enough local infrastructure for running Launchpad tests, since I do it only occasionally
<wgrant> Not common, but if it passes EC2 then it's fine... but that sounds like an inefficient way of running tests unless your change is tiny and you're really sure.
<cjwatson> mm, it tends to take me at least an hour to clear enough space for Launchpad tests, update all the dependencies and such, and then I'm less efficient at whatever else I'm meant to be doing
<cjwatson> if I were doing it all the time then that would be different
<wgrant> Yeah.
<noodles775> cjwatson: is it worth updating that LEP or chatting with jml regarding the bug? (at least so the LEP indicates why the decision has been made to allow unsetting from Wont Fix or similar)?
 * noodles775 can't see a comment related to it there.
<jml> we hadn't even thought of ACLs for the status change
 * jml makes a note
<jml> wgrant, cjwatson, I've just commented on the bug. I think the ACL on wontfix is separate from the OPINION status discussion.
<deryck> interesting discussion.
 * deryck looks at bugs....
<jml> deryck, oh yeah :) I was surprised to find a discussion like this on #launchpad-reviews
<deryck> yeah, I don't watch this channel unless pinged ;)
<jml> but I'm firmly stuck in my "fewer channels" heresy
<jml> deryck, the keyword highlight on "bug" not working out for you so well? :P
<deryck> So I fall in the camp of bdmurray and cjwatson on this.  The ACL is pointless when unset.  I didn't realize we had this bug even.  Sorry.
<deryck> jml, I droped that after 4 hours ;)
<jml> it's been pleasing to me watching my "package branch" keyword watch become increasingly useless as people use them more.
<deryck> heh
<deryck> jml, I take it you favor the ACL on unsetting WONTFIX as well?
<jml> deryck, yep.
<cjwatson> I was working on http://paste.ubuntu.com/407546/, but then it occurred to me that I probably ought to talk about it with somebody, and I hadn't got close to testing it yet
<jml> deryck, wgrant has a good point that it makes it gardening mistakes harder
<cjwatson> just means you have to find somebody minimally competent to fix it for you ...
<jml> deryck, I can't think of how to resolve that tension though.
<jml> cjwatson, for Ubuntu, sure.
<cjwatson> mm, true
<cjwatson> (I wouldn't have brought it up in this channel if I'd realised it was controversial, sorry)
<cjwatson> do many other LP pillars have Ubuntu's problem of simply being unable to keep up with bug mail?
<deryck> jml, yes, I definitely see wgrant's point.  but it's a "mistake" of semantics, though.  Do bug supervisors regularly accidentally set something to WONTFIX?
<deryck> or is it a matter of "no, you're wrong, this should be fixed." :-)
<jml> cjwatson, too much in LP is controversial. some strange alchemy makes them less controversial once they are done though.
<deryck> cjwatson, I don't think anyone has the level of problem Ubuntu has.  Both others certainly complain about the mail.
<jml> deryck, you mean "LP cannot be expected to tell the difference between an accidental status change and a genuine difference of opinion"?
<deryck> jml, exactly.
<jml> deryck, I'd agree. How do users discover who has bug-triaging powers for a given pillar?
<cjwatson> if accidental setting to WONTFIX is a concern, then leave it out of the AJAX thing?
<cjwatson> (not that that wouldn't be confusing in other ways ... but most of the accidental changes seem to be AJAX mis-clicking)
<jml> cjwatson, tbh, I don't think it's a major concern.
<deryck> cjwatson, I'm also suggesting that I don't think there is much if any accidental setting of WONTFIX.  bug supervisors aren't generally careless.
<deryck> jml, ^^
<jml> agreed.
<jml> ok, I'll drop it.
<deryck> I think if a user disagrees and we have this ACL preventing unsetting, then a user should add a comment:  "hey, you dofus this should be fixed." :-)
<deryck> hyperbole, but you get the idea.
<cjwatson> deryck: while I'm generally in your camp, there's quite a lot of bug triager carelessness in Ubuntu, unfortunately
<cjwatson> (but this is a problem we should fix!)
<deryck> cjwatson, ah, true.  You have a large group of people doing this.
<cjwatson> and a social misdesign in which triage is marketed as something that's an easy way to start helping out Ubuntu
<deryck> agreed!
<cjwatson> but I could rant about this for hours
<deryck> every project does that.  and it's sooooo bad.
<deryck> I wonder, too, if it's not self correcting, i.e. once people realize they can't easily unset it if bug traigers become more careful.
* noodles775 changed the topic of #launchpad-reviews to: on call: jelmer || reviewing: wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> hmm
<jml> that's given me a completely irrelevant idea
<deryck> cjwatson, so I would welcome the branch here for this.  I don't think you need the BUG_SUPERVISOR_OLD_BUGTASK_STATUSES, though.
<deryck> just self.status not in BUG_SUPERVISOR_BUGTASK_STATUSES and new_status not in BUG_SUPERVISOR_BUGTASK_STATUSES, right?
<cjwatson> deryck: I felt that it should be OK to set status away from Triaged
<deryck> ah, right
<cjwatson> this is just a gut feel
<cjwatson> if you disagree I don't feel strongly
<deryck> cjwatson, what do people normally transition to from triaged?
<cjwatson> but it seemed to me that we might want those to be independently controllable
<cjwatson> either in-progress etc. (developer) or something like "oh, hang on, this isn't clear after all" incomplete (not sure)
<deryck> right
<cjwatson> it just didn't seem as obviously final as wontfix is meant to be
<deryck> I agree with you on this.  it's just the CONSTANT for WONTFIX that I'm not crazy about.
<cjwatson> CONSTANT?
<deryck> maybe just `self.status is not BugTaskStatus.WONTFIX`
<cjwatson> oh right
<cjwatson> if you like, sure
<deryck> instead of adding BUG_SUPERVISOR_OLD_BUGTASK_STATUSES
<deryck> keep someone from chasing down what that is later :-)
<deryck> cjwatson, now one issue that will complicate this...
<deryck> cjwatson, if it's in WONTFIX, I think the AJAX widget should not allow selecting any other statuses.  and I'm not sure how the js code consults canTransitionToStatus
<cjwatson> I think it excludes anything for which that returns False
<cjwatson> browser/bugtask.py:status_widget_items
<deryck> yes, good spot.
<deryck> cjwatson, so when you're ready, I'll happily review this for you. :-)  many thanks for the fix!  jml -- are you ok with this?  Or further discussion is required?
<jml> not from me.
<deryck> excellent.
<cjwatson> I'll need to set up ec2test, so ... see you in a bit ;-)
<deryck> ok :-)
* salgado changed the topic of #launchpad-reviews to: on call: jelmer || reviewing: wgrant || queue: [wgrant,salgado] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<jml> salgado, why the 'tee' change?
<salgado> jml, to use sudo, I guess
<salgado> before that command was executed as root
<jml> oh, duh. I should have seen that :)
<salgado> not it's as ubuntu
<jml> *nod*
<jml> salgado, r=me
<salgado> thanks jml!
* salgado changed the topic of #launchpad-reviews to: on call: jelmer || reviewing: wgrant || queue: [wgrant] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: on call: jelmer || reviewing: wgrant || queue: [wgrant, henninge] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* henninge changed the topic of #launchpad-reviews to: on call: jelmer || reviewing: wgrant || queue: [wgrant, henninge(oversized)] || 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, jelmer || reviewing: henninge, wgrant || queue: [wgrant] || 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: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<rockstar> bac, can I have you do a quick review for me?
<bac> rockstar: for you?  sure!
<rockstar> bac, https://code.edge.launchpad.net/~rockstar/launchpad/code-js-move/+merge/22656
<bac> rockstar: :(
<bac> your rename of the code js directory is going to break branches that sinzui and i have in progress
<bac> rockstar:  so, lp-deps.py is meant to be general purpose, right?  each app will have to add themselves to JS_DIRSET?
<rockstar> bac, yes.
<bac> rockstar: can i get you to talk to sinzui tomorrow before landing this?  i want to ensure his changes to discover JS tests will still work.
<rockstar> bac, well, we talked about it at the UI meeting, but yes.
<bac> we may have to move our js too.
<bac> thanks.
<rockstar> In fact, I think I'm going to have to move everyone's javascript is moved soon.
<rockstar> bac, does that me r=you then?  :)
<bac> yeah, that makes sense
<bac> rockstar: yeah, r=bac but chat with curtis
<bac> he won't say not to land it but may need to coordinate some stuff
<rockstar> Yeah, I suspected as much.
<rockstar> bac, although I bet he's tired of that branch.  :)
<bac> yes
<bac> we all are
<cjwatson> should I be expecting to get a mail from ec2-abuse@amazon.com about going over a limit on volume of sent e-mail after a full ec2test run?
<salgado> I've never got one
<cjwatson> seems odd for it to be sending any mail out of the instance
<salgado> indeed; on test runs we use the stub mailer
<ctrlsoft> I've never had one either; it shouldn't be sending email other than the success/failure email and email to PQM
<mwhudson> it certainly shouldn't
<wgrant> bac: Can you please re-approve https://code.edge.launchpad.net/~wgrant/launchpad/refactor-slavestatus/+merge/22285?
<bac> wgrant: it looks fine
<bac> wgrant: do you want me to send it off to ec2 and land now?
<wgrant> bac: Ah, I didn't realise it had reopened. Yes please!
<wgrant> Thanks.
<wgrant> Can someone please also send https://code.edge.launchpad.net/~wgrant/launchpad/move-lots-of-builddmaster-to-queuebuilder/+merge/22009, https://code.edge.launchpad.net/~wgrant/launchpad/bug-540819-fix-builder-list-icons/+merge/22288 and https://code.edge.launchpad.net/~wgrant/launchpad/filter-apt_pkg-deprecationwarnings/+merge/22317 off to EC2?
<bac> wgrant: off she goes.  let me know if you don't get an email in 4-5 hours
<wgrant> bac: Thansks.
<bac> wgrant: ask rockstar about those others.  part of ocr, you know.
<wgrant> bac: Hm, true. rockstar ^^
<jpds> bac: Can you send: https://code.edge.launchpad.net/~jpds/launchpad/fix_361650_model_changes/+merge/20785 ?
<jpds> bac: Actually, I should probably let sinzui re-review.
#launchpad-reviews 2010-04-02
* salgado changed the topic of #launchpad-reviews to: on call: salgado,rockstar || reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
<salgado> (completely forgot to add my name to the topic, sorry)
<salgado> rockstar, you up for reviewing some massive removals?
<rockstar> salgado, I'm not actually reviewing, but yes!
<salgado> rockstar, oh, no need to worry, then.  it's not urgent, but if you'd like I certainly wouldn't mind. :)
<salgado> https://code.edge.launchpad.net/~salgado/launchpad/remove-logintoken-unused-code/+merge/22719
<jpds> sinzui: Hey, did you have time to look at my c_d_m model branch?
<sinzui> jpds, yes
<jpds> sinzui: Could we possibly land it now?
<sinzui> yes, sorry. I was distracted by my son. I will land it now. I think your changes were great
<jpds> sinzui: No worries, thanks!
#launchpad-reviews 2010-04-03
<wgrant> sinzui: Up for reviewing a branch that reverts those sampledata changes?
* wgrant changed the topic of #launchpad-reviews to: on call: salgado,rockstar || reviewing: - || queue: [wgrant(revert-soyuz-sample-data-changes),wgrant(bug-538844-master-side)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
* wgrant changed the topic of #launchpad-reviews to: on call: salgado,rockstar || reviewing: - || queue: [wgrant(ttbb-slavestatus-testfix),wgrant(revert-soyuz-sample-data-changes),wgrant(bug-538844-master-side)] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.edge.launchpad.net/launchpad/+activereviews
