[00:01] <wgrant> StevenK: Indeed :(
[00:02] <wgrant> StevenK: It's a bit unfortunate that Storm doesn't let us have lazy columns.
[00:06] <wgrant> And that review is done.
[00:50] <StevenK> wgrant: How do I declare an array col?
[00:53] <wgrant> StevenK: List
[00:53] <wgrant> List(type=Int()) is probably what you want
[01:07] <huwshimi> wgrant: Did you fix the search speed?
[01:16] <huwshimi> Well, whoever did, it's amazing!
[01:16] <rick_h_> huwshimi: adeuring worked on that recently
[01:16] <rick_h_> the fulltext search stuff?
[01:17] <huwshimi> rick_h_: Ah right. It's so good!
[01:17] <huwshimi> It might just be the fastest action on Launchpad...
[01:23] <StevenK> wgrant: I have a bunch of failures because the test suite is assuming the owner can access private branches
[01:28] <wgrant> huwshimi: Bug searches? Yeah, they're a few times faster now, it's nice.
[01:28] <wgrant> The speed improvements are mine, the less obtuse text search is Abel's.
[01:29] <huwshimi> wgrant: Well, thanks a lot!
[01:30] <wgrant> It's still about twice as slow as it's meant to be, but I'll hopefully work that out some time this year :)
[01:32] <huwshimi> wgrant: I didn't know we could load pages that quickly, already.
[01:32] <wgrant> Heh
[01:33] <huwshimi> Just with the roundtrip, template render etc.
[01:35] <StevenK> wgrant: Considering throwing in an owner clause to get_branch_privacy_filter()
[01:35] <wgrant> StevenK: Shouldn't the owner have a grant on creation, because it has a subscription automatically?
[01:36] <lifeless> huwshimi: hah
[01:36] <lifeless> huwshimi: its not all doom and gloom; lots of hard work tho
[01:37] <wgrant> huwshimi: The minimum server-side render time for a full page with auth and template rendering is around ~200-250ms. We really need to fix our stack.
[01:37] <StevenK> wgrant: Hah, I've just had a thought -- I wonder if these test failures will die due to the sampledata changes in drop-branch-populate-aag.
[01:37] <wgrant> And we also need slaves in the US :)
[01:37] <wgrant> StevenK: Indeed, probably!
[01:38]  * StevenK wields make schema to find out
[01:38] <huwshimi> lifeless: Well, it's paying off!
[01:39] <StevenK> huwshimi: The sharing pages are among the quickest in LP, too
[01:50] <StevenK> wgrant: Running makeBranch(owner=owner, information_type=USERDATA) in a harness gives access_grants = {}
[01:50] <wgrant> StevenK: I suggest working out why.
[01:51] <wgrant> StevenK: It's possibly because it transitions the information type after creating the subscription, and the transition doesn't create grants.
[01:54] <StevenK> wgrant: IBranchNamespace.createBranch will only subscribe if there is a BVP
[01:54] <StevenK> Which doesn't help much for the testsuite
[01:54] <StevenK> We can subscribe the owner if information_type is not None in the factory
[01:55] <wgrant> Ah
[01:56] <wgrant> Not only does it not help for the test suite, but it also ruins all of our plans.
[01:58] <StevenK> Well, it pulls teams from the BVP and subscribes them all
[01:59] <wgrant> StevenK: Huh?
[01:59] <wgrant> That's not the relevant bit here.
[02:00] <wgrant> StevenK: I've just checked, and even public branches get a default subscription for the owner.
[02:00] <wgrant> The BVP subscription stuff is for granting others access.
[02:00] <wgrant>         branch.subscribe(
[02:00] <wgrant>             self.owner,
[02:00] <wgrant> Right after the BVP sub.
[02:00] <StevenK> Ah
[02:00] <StevenK> Then why is branch.access_grants = {} :-(
[02:01] <wgrant>         if information_type is not None:
[02:01] <wgrant>             naked_branch.transitionToInformationType(
[02:01] <wgrant>                 information_type, registrant, verify_policy=False)
[02:01] <wgrant> Probably because of that.
[02:02] <wgrant> See the         if information_type in PRIVATE_INFORMATION_TYPES:
[02:02] <wgrant> section of Bug.transitionToInformationType
[02:02] <wgrant> It's probably missing from Branch.transitionToInformationType
[02:03] <StevenK> wgrant: And missing_subscribers = set([who, self.owner]) ?
[02:05] <wgrant> StevenK: That's a difficult question.
[02:05] <wgrant> StevenK: I don't think so.
[02:05] <wgrant> The owner certainly shouldn't be implicitly subscribed.
[02:05] <wgrant> The actor probably should be.
[02:10] <StevenK> wgrant: Adding a missing_subscribers = set([who]) and subscribing still ends up with access_grants = {}
[02:11] <wgrant> StevenK: Did you add the ensureAccessGrants bits?
[02:31] <StevenK> wgrant: I have now, still {}
[02:37] <StevenK> Woot, access_grants = {243652}
[02:52] <wgrant> StevenK: What'd you change?
[02:53] <StevenK> wgrant: Moved the ensureAccessGrants() section after we set information_type and call _reconcileAccess()
[02:53] <wgrant> Ah, yes, that's fairly critical :)
[04:28] <StevenK> wgrant: Is test_owner_member_sees_own_branches in test_branchcollection complete crack?
[04:29] <StevenK> It creates a private branch by stacking on a existing private branch, then unsubscribes the owning team and then asserts the team owner can still see the branch.
[04:29] <wgrant> StevenK: Right, that test no longer makes sense, as we've changed the rules.
[04:29]  * StevenK bins it
[04:30] <StevenK> wgrant: The only failure left is test_initial_branches_does_not_contain_private_dev_focus_branch in code/browser/tests/test_product.py
[04:34] <wgrant> StevenK: I would suggest making it not fail.
[04:35] <StevenK> Obviously.
[04:35] <StevenK> I just don't get why it does fail, I thought CIV() with no principal was anonymous
[04:36] <lifeless> Its been replaced.
[04:36] <lifeless> With CV()
[04:37] <StevenK> *facepalm*
[04:46] <StevenK> Oh, DUH
[04:47] <StevenK> The bloody tests calls login() in the method to create the branch
[04:48] <StevenK> wgrant: So it looks like this branch is creating a private development focus for a product and then assuming it can't be seen.
[04:48] <StevenK> s/branch/test/
[04:48] <wgrant> StevenK: I got that idea from the name, yaeh.
[04:49] <StevenK> Considering replacing the login() call with 'with person_logged_in(...)' but that may have other knock-on effects.
[04:49] <wgrant> The bigger question is perhaps why this only makes it break now.
[04:50] <StevenK> It calls check_permission on the dev focus, which calls visibleByUser() and I have changed that method a lot
[04:51] <wgrant> Right.
[04:51] <wgrant> But that suggests that the login() call is not the problem.
[04:51] <wgrant> Or the test was buggy before.
[04:51] <StevenK> Tossing a login(ANONYMOUS) before the call to create_initialized_view() fixes the test.
[04:53] <StevenK> I'm not sure how this test ever worked properly.
[04:57] <wgrant> StevenK: Ah
[04:57] <wgrant> I just read the code
[04:57] <wgrant> Now it is clear
[04:58] <wgrant> StevenK: It explicitly logs in as the product owner, not the branch owner.
[04:58] <wgrant> StevenK: Recall that product owners now have an APG by default.
[04:58] <StevenK> Haha, which we now allow
[04:58] <StevenK> Due to the visibleByUser() changes
[04:59] <wgrant> Precisely.
[04:59] <wgrant> So you can either log in as another user or remove the APG
[04:59] <StevenK> login(ANONYMOUS) it is then
[05:21] <StevenK> wgrant, wallyworld: https://code.launchpad.net/~stevenk/launchpad/new-branch-visibility/+merge/113484
[05:23] <wgrant> StevenK: Did you look into using BranchCollection to implement Branch.visibleByUser?
[05:24] <StevenK> Not really, I didn't see how it could be a win
[05:24] <wgrant> It removes code duplication.
[05:28] <StevenK> wgrant: By calling IAllBranches.visibleByUser?
[05:30] <wgrant> StevenK: BranchCollection sadly doesn't provide a means to filter by ID or URL, so it may not be doable at this stage.
[05:32] <StevenK> wgrant: branch_filter_expressions=[Branch.id == self.id] ?
[05:33] <wgrant> StevenK: Oh, I lie, getUtility(IAllBranches).withIds(self.id).visibleByUser(user).is_empty() should work
[05:36] <StevenK> wgrant: http://pastebin.ubuntu.com/1075921/ ?
[05:37] <StevenK> Hm, we can probably refactor further, and feed a list of the current branch and all branches stacked on it
[05:39] <wgrant> Well
[05:39] <wgrant> It's not clear whether we should preserve that behaviour at that level.
[05:39] <StevenK> It's current behaviour at least
[05:43] <StevenK> wgrant: http://pastebin.ubuntu.com/1075926/
[05:43] <StevenK> Sigh, modulo a one character typo
[05:43] <wgrant> StevenK: That doesn't make much sense.
[05:44] <wgrant> The visibility of a branch might depend on the branches it's stacked on.
[05:44] <wgrant> It certainly does not depend on the branches that are stacked on it.
[05:45] <StevenK> That was the intent
[05:50] <StevenK> wgrant: Does http://pastebin.ubuntu.com/1075932/ make you hate me?
[05:51] <wgrant> StevenK: That probably won't work, since it's unlikely you can check the stacked_on of a branch you don't have access to.
[05:51] <wgrant> You probably need to preserve the original behaviour.
[05:52] <wgrant> Just replacing _checkBranchVisibleByUser with the branchcollection thing
[05:52] <wgrant> s/behaviour/structure/
[05:52] <StevenK> Yeah :-(
[05:53] <StevenK> I suspect WithIds wants a list, though
[05:54] <StevenK> Hmm, not sure
[06:09] <StevenK> wgrant: Diff updated.
[06:12] <wgrant> StevenK: r=me, but please do some data checks on prod before landing
[06:13] <StevenK> wgrant: Such as checking all private branches have some AAGs? Or what did you have in mind?
[06:13] <wgrant> StevenK: Actually, why not inline the branchcollection usage into visibleByUser?
[06:13] <StevenK> Hm, good point.
[06:13] <wgrant> _checkBranchVisibleByUser is now one statement, and it only has one callsite
[06:13] <StevenK> Let's do that.
[06:13] <wgrant> Might as well inline
[06:13] <wgrant> Save another few lines.
[06:13] <wgrant> Get you to -15 or so
[06:14] <StevenK> -17
[06:16] <StevenK> -18, in fact, because I'm an idiot
[06:18] <StevenK> wgrant: Which is fantastic, because I've wanted _checkBranchVisibleByUser dead for a while.
[06:22] <StevenK> wgrant: What data checks did you have in mind?
[06:23] <wgrant> StevenK: The ones I was thinking of don't really work
[06:23] <wgrant> So let's JFDI
[06:24]  * StevenK tosses it at ec2
[07:41] <adeuring> good morning
[08:41] <StevenK> _StringException: Text attachment: garbage
[08:41] <StevenK> ------------
[08:41] <StevenK> [<storm.databases.postgres.PostgresConnection object at 0x168662d0>]
[08:41] <StevenK> wgrant: ^ have you seen that on ec2?
[08:44] <wgrant> StevenK: buildbot saw that twice last week.
[08:44] <wgrant> It's spurious.
[08:47] <StevenK> But it will mark the branch as failed, which is annoying
[10:07] <cjwatson> Any thoughts on a nice way to export the list of custom files in a PackageUpload?  lp.soyuz.scripts.queue currently does:
[10:07] <cjwatson>         for queue_custom in queue_item.customfiles:
[10:07] <cjwatson>             self.display("\t | * %s Format: %s"
[10:07] <cjwatson>                          % (queue_custom.libraryfilealias.filename,
[10:07] <cjwatson>                             queue_custom.customformat.name))
[10:08] <cjwatson> I exported URLs for the library files as PU.customFileUrls(), but maybe it should've been .customFiles() that returns a list of (URL, format) tuples or something ...
[10:09] <wgrant> That might be best, indeed.
[10:09] <cjwatson> (It's on devel exactly so that we can change it ;-) )
[10:09] <wgrant> Yup
[11:00] <cjwatson> Hmm, OTOH that would mean every 'queue info' would have to make a customFiles call just to find out whether there were any
[11:00] <cjwatson> Maybe it would make more sense to fold it into getBinaryProperties
[12:04] <danilos> heya, I've got 3 MPs up for different workitem-related stuff
[12:04] <danilos> if anyone feels review-happy, there you go :)
[12:11] <jam> gmb: for the addSkip thing
[12:11] <jam> should it be falling back to addSuccess?
[12:11] <gmb> Um.
[12:11] <jam> AIUI that is what testtools in 2.6 is doing
[12:11] <gmb> jam. A thousand botherations. Yes, it should. Good catch (and that'll teach me for self-reviewing)
[12:11] <jam> otherwise, I think you break the subunit stream
[12:11] <jam> in the way that it broke for 2.7
[12:12] <gmb> jam, I don't - it handles the skip properly
[12:12] <gmb> It just doesn't call up to the parent class.
[12:12] <gmb> Actually, hmm..
[12:12] <gmb> jam, They way it is now, it skips properly when using --subunit
[12:12] <jam> it is possible that self.options.output.test_skip is doing the right output.
[12:13] <gmb> jam, It is... it's just not incrementing the counter on unittest.TestResult, which doesn't exist.
[12:13] <gmb> (For py 2.6)
[12:14] <jam> gmb: ah ok. the other thing to watch out for is double-counting, but yeah, I think calling something on the super() class is useful.
[12:14] <jam> gmb: we just need a # py 2.6 comment there, so we know we can nuke all that code in the future
[12:14] <jam> :)
[12:14] <gmb> Right :)
[12:14] <gmb> Amusingly, the zope.testing test suite breaks horribly under 2.6.
[12:14] <gmb> *sigh*
[12:14] <gmb> But I'll go and make that change so that we fall back.
[12:15] <jam> gmb: so you can't actually run the zope.testing suite in 2.6? ugh
[12:15] <gmb> Exactly.
[12:15] <gmb> You can run bits of it
[12:15] <gmb> But not all of it.
[12:15] <gmb> So "assertIn()" for example has to not be used :/
[12:18] <jam> mgz: I see a bunch more of your patches have landed. kudos
[12:31] <mgz> jam: I shall run the kanban update again and see if they're done-done
[12:31] <jam> mgz: well, it hasn't been deployed to prod
[12:31] <jam> so I think it shouldn't be
[12:32] <mgz> yup, Deployment::Ready is the state
[12:42] <rick_h_> jcsackett: just soyuz and translation left :) https://code.launchpad.net/~rharding/launchpad/registry_yui35/+merge/113407
[12:42] <rick_h_> jcsackett: I tried to comment the actual changes vs moving as much as possible so it's a long MP, but itemized to help.
[13:11] <cjwatson> wgrant: https://code.launchpad.net/~cjwatson/launchpad/queue-api-custom-files/+merge/113574 ?
[13:30] <deryck> adeuring, https://plus.google.com/hangouts/_/040bd7afe73bdcaf379359f9eb707488709b7abd?authuser=0&hl=en
[13:32] <deryck> rick_h_, https://plus.google.com/hangouts/_/040bd7afe73bdcaf379359f9eb707488709b7abd?authuser=0&hl=en
[14:19] <danilos> jcsackett, hi, I've got a couple of MPs up, can you please take a look at them on +activereviews?
[14:19] <danilos> jcsackett, fwiw, I am around for any questions in the next few hours
[14:19] <jcsackett> danilos: dig. i'm working through the queue.
[14:19] <danilos> jcsackett, cool, thanks
[14:58] <nigelb> Did the bzr team get merged back into LP?
[14:58] <nigelb> Or something of that sort.
[14:59] <mgz> yeah, of that sort.
[15:14] <rick_h_> abentley: so all three JS tests just passed in yui3.5 w/o even opening them.
[15:15] <abentley> rick_h_: Good news: all passed.  Bad news: all = 3. :-)
[15:15] <rick_h_> hah
[15:15] <rick_h_> well three files
[15:15] <rick_h_> looks like 28 actual tests
[15:17] <abentley> rick_h_: Well, that's good then.
[16:53] <czajkowski> mpt: where would we be without you :0
[16:53] <czajkowski> :)
[16:54] <rick_h_> jcsackett: quick one for you for a change when you get a chance: https://code.launchpad.net/~rharding/launchpad/fix_attr_quotes/+merge/113610
[16:54] <mpt> In a world that was slightly more confusing but otherwise just the same
[16:54] <czajkowski> and no spam :)
[17:17] <ivory> jcsackett: i could use a rievew: https://code.launchpad.net/~ivo-kracht/launchpad/bug-728129/+merge/113612
[17:18] <jcsackett> ivory: your up next as soon as i finish my current.
[17:22] <ivory> jcsackett: thank you, chances are that i'm not online anymore by then since it's quite late now and it's my last day of my internship. questions go to adeuring then :)
[17:25] <cjwatson> $ bzr damage
[17:25] <cjwatson> Using submit branch /home/cjwatson/src/canonical/launchpad/lp-branches/devel
[17:25] <cjwatson> Healing: 2007 lines of code
[17:25] <cjwatson> ^- I like the look of this
[17:36] <cjwatson> And I can even land that after my three queue-api-* MPs are reviewed and deployed ;-)
[17:58] <rick_h_> jcsackett: ty much for the trio of reviews today
[17:58] <rick_h_> cjwatson: LoC credit coming down the pipe?
[17:58] <cjwatson> rick_h_: stand back
[17:59] <rick_h_> cjwatson: LoC assassnator!
[17:59] <rick_h_> escept with better spelling and all that
[19:20] <adeuring> jcsackett: could you please do a "follow-up" review of Ivo's MP: https://code.launchpad.net/~adeuring/launchpad/bug-728129/+merge/113631
[19:21] <jcsackett> adeuring: cool, can you verify if the test_decoratedresultset.py file is even needed? it look like it's to collect the doctest, but that doctest already exists and runs without thie test_*py file.
[19:22] <adeuring> jcsackett: yes, we need some way to get the doc test in the tests/ directory executed. WIthout this file, the doc test is simply ignored.
[19:23] <adeuring> jcsackett: note that there is another fle decoratedresultset.txt, but in the directory doc/, which is at oresent executed.
[19:25] <jcsackett> adeuring: dig. r=me then.
[19:25] <adeuring> jcsackett: cool, thanks, also in Ivo's name!
[20:04] <abentley> lifeless: Have you seen https://dev.launchpad.net/Code/RecipeBuildDebversioning ?  The more I think about it, the more it seems like a recipe build should supersede any other packaging by default, because if you don't want the recipe version, you remove the package & PPA.
[20:05] <lifeless> I saw you make it yesterday, I think.
[20:05] <lifeless> Your logic makes sense to me. Its complicated by apt/dpkg behaviour but not something we can really solve.
[20:06] <lifeless> by which I mean, being a slightly higher version is a good idea. But removal is more complicated (and there are special tools to use to do that).
[20:22] <abentley> james_w: Could you comment on https://dev.launchpad.net/Code/RecipeBuildDebversioning ?
[21:52] <lifeless> bbiab
[22:31] <cjwatson> jcsackett: thanks for the cluster of reviews
[22:32] <jcsackett> cjwatson: you're welcome. :-)
[22:32]  * cjwatson runs three parallel ec2 lands, what could possibly go wrong
[22:43] <jcsackett> cjwatson: generally, that all works fine. as long as your branches weren't secretly dependent on each other. :-P
[22:44] <cjwatson> I don't *think* they were
[22:44] <cjwatson> will be a fun QA opportunity tomorrow though
[23:09] <StevenK> wgrant, wallyworld: http://pastebin.ubuntu.com/1077299/
[23:20] <StevenK> FAILED (id=1, failures=662 (-3231), skips=2)
[23:20] <StevenK> Progress!