[00:00] <wgrant> It's too easy for a factory bug or change to invalidate the test otherwise.
[00:00] <thomi> right
[00:03] <thomi> wgrant: new diff ready...
[00:04] <thomi> I guess I got the zcml stuff correct? I wasn't really sure about that...
[00:04] <wgrant> Yep, that's right.
[00:06] <thomi> is securedutility something launchpad specific? I couldn't find it in http://docs.zope.org/zope.component/zcml.html
[00:09] <wgrant> Yes, that's one of ours.
[00:10] <wgrant> See SecuredUtilityDirective in lp.services.webapp.metazcml
[00:10] <wgrant> But in general you shouldn't have to care about the details.
[00:15] <thomi> ok.. blindly copy + paste. check :D
[00:17] <blr> wgrant: regarding the ugly list comprehension, were you suggesting it would be better to return a dict of dicts, rather than the list of ref dicts?
[00:20] <wgrant> blr: Right, a list of dicts with a unique "name" field feels like it should be a dict.
[00:21] <wgrant> (though that becomes more questionable if we ever batch that endpoint, that'll be an API break for other reasons anyway)
[00:21] <blr> wgrant: yep, that would be better. Also, prepending 'refs/' will still be necessary in the view, but you're entirely right that it shouldn't in store.
[00:21] <wgrant> Right, the view presumably keys on refs/, so it doesn't get that part of the path.
[00:21] <blr> wgrant: well, when we need to address paging we can revisit it
[00:21] <wgrant> Yep.
[00:22] <blr> thanks
[00:33] <wgrant> thomi: Approved with a couple of trivial fixes.
[00:33]  * thomi looks
[00:33] <wgrant> thomi: Can you file an RT ticket to get your OpenPGP key added to the PQM keyring, so you can land things yourself?
[00:33] <wgrant> Mail it to launchpad@ and I'll approve it.
[00:34] <thomi> wgrant: I file the rt by emailing launchpad@rt.canonical.com ?
[00:34] <wgrant> yep
[00:36] <thomi> wgrant: filed
[00:38] <wgrant> Got it, thanks.
[00:41] <thomi> wgrant: OK, I made those changes. Should to top-approve that MP, or would you like to look at it again?
[00:41] <thomi> I guess the PQM keyring won't get updated today
[00:46] <wgrant> thomi: Top-approval doesn't do anything special for us today, as we don't use Tarmac. But I tend to use it as if we did.
[00:46] <wgrant> Looks fine to me. I'll land it unless you have objections to your own branch.
[00:46] <thomi> no objections here :D
[00:46] <wgrant> Good, I'd be worried.
[00:47] <thomi> heh
[01:17] <thomi> wgrant: I have another MP for you... the docs for zope_isinstance are rather lacking, so I'm not convinced that it's this easy... but the function is reasonably well tested, and the tests all pass, so... : https://code.launchpad.net/~thomir/launchpad/devel-fix-import-violations-buildfarmjob/+merge/251397
[01:19] <wgrant> thomi: zope_isinstance is just an isinstance that looks through security proxies.
[01:19] <wgrant> Do the tests pass? I think that change is incorrect.
[01:19] <wgrant> The structure here is a little bit confusing.
[01:19] <thomi> wgrant: the tests pass
[01:20] <wgrant> Odd
[01:20] <thomi> I'm running lp.soyuz.browser.tests.test_builder_views.TestgetSpecificJobs
[01:20] <thomi> wgrant: I'm not sure what the word 'specific' means in that docstring
[01:20] <wgrant> thomi: What about test_builder_views as a whole?
[01:21] <wgrant> thomi: There are four types of build farm jobs. IBinaryPackageBuild, ISourcePackageRecipeBuild, ITranslationTemplatesBuild and ILiveFSBuild are all derivatives of IBuildFarmJob.
[01:21] <wgrant> But, crucially, they are *specific* jobs.
[01:22] <wgrant> They each have their own table named after them, and the data is primarily stored there.
[01:22] <thomi> wgrant: I'm running those tests now
[01:22] <wgrant> But each specific job also references a non-specific row in the BuildFarmJob table, which is used for builder histories etc. that show all job types.
[01:22] <thomi> I'm still not sure what specific means in that context.
[01:22] <thomi> wgrant: those tests all pass as well :D
[01:22] <blr> wgrant: just as a wee experiment, all of the refs in the linux git repo, serialized are 9.6k
[01:22] <wgrant> BuildFarmJob is effectively an abstract base class.
[01:22] <wgrant> Except it happens to actually exist for DB reasons.
[01:24] <wgrant> thomi: So, the BuildFarmJob class is this non-specific DB table, which needs to be resolved to a concrete specific job for display. But IBuildFarmJob is implemented by both specific and non-specific classes, so your check changes the behaviour.
[01:24] <wgrant> I was hoping a test would fail to prove that, but to preserve the behaviour you can check against IBuildFarmJobDB instead.
[01:24] <wgrant> Only BuildFarmJob itself implements IBuildFarmJobDB.
[01:24] <wgrant> (BuildFarmJob should probably be renamed to BuildFarmJobDB, but historical reasons)
[01:25] <wgrant> Specific jobs have a non-specific BuildFarmJob.
[01:25] <wgrant> Specific and non-specific jobs implement IBuildFarmJob.
[01:25] <wgrant> Only non-specific jobs implement IBuildFarmJobDB.
[01:26] <thomi> so... that check should be 'IBuildFarmJob.providedBy(job) and not IBuildFarmJobDB.providedBy(job)' ?
[01:26] <wgrant> zope_isinstance(job, BuildFarmJob) is only true for a non-specific job.
[01:27] <wgrant> So I think it can be replaced with just "IBuildFarmJobDB.provideBy(job)"
[01:27] <thomi> oh, right.
[01:28] <thomi> hmmmm, I'll add a test for that failure case as well
[01:28] <wgrant> This code and schema has been through three or four serious redesigns over the years, so the tests may well be dodgy.
[01:29] <thomi> yeah... some of these could be better named
[01:29]  * thomi gets out his yak razor
[01:29] <wgrant> But basically the method is designed to take an arbitrary combination of non-specific and specific jobs, and turn them all into specific jobs.
[01:29] <wgrant> Obviously for a specific job that transformation is a no-op.
[01:30] <wgrant> It may be that the tests work because the non-specific and specific IDs match, which will happen if you only use one kind of job.
[01:35] <wgrant> blr: We'll need to handle non-ASCII refs soon, but that can be a subsequent iteration.
[01:37] <blr> wgrant: ugh, right.
[01:41] <blr> I think #git exists primarily to slag off other dvcses, sadly.
[01:42] <wgrant> Well, what else are they going to talk about? How good git's UI is? :)
[01:44] <blr> wgrant: noooo... don't do it.
[01:49] <xnox> they are quite helpful with git intrisics support.
[02:06] <wgrant> Evening xnox.
[02:18] <thomi> wgrant: if you have a moment: https://code.launchpad.net/~thomir/launchpad/devel-fix-import-violations-buildfarmjob/+merge/251397
[02:19] <wgrant> thomi: Done.
[02:19] <thomi> wgrant: I was tempted to rewrite that function, as I think i could be a lot clearer, but decided not to.
[02:20] <thomi> thanks... down to 5 violations now :D
[02:20] <wgrant> thomi: You should feel free to rewrite things when it makes sense to. A lot of this code is old and things have evolved around it.
[02:21] <thomi> yeah... it would have been an aesthetic change only, and the tests aren't the best, so I figured it was best to leave it alone :D
[02:21] <wgrant> Indeed.
[02:22] <wgrant> thomi: Oh, I was about to land it for you, but blahdeblah has added you to the keyring.
[02:22] <thomi> oh!
[02:22] <wgrant> thomi: 'bzr lp-land --no-qa' in the branch directory should do what you want.
[02:22] <wgrant> Run it from your host, not inside the container, for keyring access.
[02:22] <thomi> oh, well that's easy then
[02:22] <wgrant> You may need to grab bzr-pqm from the PPA, though.
[02:23] <wgrant> And you'll need SMTP server config.
[02:23] <thomi> the container has access to gpg keyring too...
[02:23] <thomi> oh.. smtp thing will be "fun"
[02:23] <wgrant> thomi: It needs launchpadlib keys too.
[02:23] <wgrant> Which are in your gnome-keyring.
[02:25] <wgrant> thomi: https://pastebin.canonical.com/126638/ has a superset of the relevant settings from my bzr configs.
[02:26] <thomi> how come no bzr-pqm package in utopic, but exists in saucy?
[02:26] <wgrant> thomi: Because bzr and LP are probably the only two projects in the world that still use PQM.
[02:26] <thomi> oh ok
[02:26] <wgrant> The saucy package should work, though.
[02:27] <thomi> the package in ppa:launchpad/ppa should as well I guess
[02:28] <wgrant> Ah yes, I copied that up, didn't I.
[02:35]  * wgrant lunches.
[02:58] <wgrant> thomi: Looks like you got lp-land working?
[02:58] <thomi> wgrant: maybe? The branch is still 'approved'
[02:58] <wgrant> (also, you don't actually need to top-approve at all; lp-land will be happy as long as it has valid reviews)
[02:58] <wgrant> thomi: Heh, check your email.
[02:58] <thomi> oh, right
[02:58] <wgrant> You set from Merged to Approved, I guess you raced with the scanner.
[02:58] <thomi> I was about 10 seconds too early :D
[02:58] <thomi> ahh
[02:59] <thomi> wgrant: I wanted to ask you about the final few import violations
[02:59] <thomi> they're (almost) all where we're using load_related for (I think) database efficiency
[02:59] <wgrant> Yup, that's what I thought they'd mostly be.
[03:00] <wgrant> That's usually done with a DecoratedResultSet using pre_iter_hook.
[03:00] <wgrant> Many methods in the model take an eager_load option, or a number of need_WHATEVER options to enable preloading of certain related objects.
[03:01] <wgrant> Or some other sets have a preloadForWhatever method.
[03:01] <thomi> I'm not sure what the fix is for that - just move the code into a SomethingSet ?
[03:01] <thomi> for example, lp.soyuz.browser.archivesubscription line 334 (or thereabouts)
[03:03] <wgrant> thomi: I'd fix getBySubscriberWithActiveToken. Depending on how many places it's used, you might just be able to push the load_related down into there without needing to make it optional.
[03:03] <wgrant> Loading a handful of archives is relatively cheap, so it's not terrible to do it where it's not necessary.
[03:03] <wgrant> But other types of preloading definitely want to be up to the caller to enable.
[03:09] <thomi> hmmm
[03:09] <wgrant> Do you understand what all the preloading code is for?
[03:10] <wgrant> It's to prevent the ORM from issuing a bazillion queries when accessing objects related to sets of other objects.
[03:11] <wgrant> Each ArchiveSubscription obviously has a related Archive, and naive code looping through some subscriptions would issue a separate query for each subscription's Archive.
[03:11] <wgrant> This is not good for performance.
[03:11] <wgrant> load_related collects the IDs and does a single query for them all, to get them into Storm's cache cheaply.
[03:11] <thomi> somehwat unrelated, but presumably it'd only load the archive details when you accessed that part of the subscription object, right?
[03:12] <wgrant> Right, code like "subscription.archive" pulls it in.
[03:12] <wgrant> But it checks in Storm's cache (keyed on (class, primary key)) before making the query, which is why load_related prevents the extra query.
[03:13] <thomi> ok.. I think I just need to get my head around how this code gets moved around
[03:13] <wgrant> You can probably just literally move the load_related call into getBySubscriberWithActiveToken, except that security proxies may ruin your day.
[03:14] <thomi> so getBySubscriberWithActiveToken would include the load_related call and throw away the result? I mean, it'd still be in the storm cache
[03:14] <thomi> oh, except we use the result from load_related, but I guess if it's in the cache already we can look it up again and it should be cheap?
[03:19] <wgrant> thomi: afk, but yes. once it's loaded the view can just loop through the subscriptions and get the archive out
[03:19] <thomi> awesome. There are some other things I can change in here as well.
[03:20] <wgrant> yep, clean away
[03:31] <thomi> my brain is melting
[03:31] <wgrant> :(
[03:33] <thomi> ugh. given that it's EOD for me, and I turn into a CI-team pumpkin tomorrow, I might back away sloooowly from this one :D
[03:49] <wgrant> thomi: heh, no worries. you got a couple of good branches landed today :)
[04:14] <thomi> yeah
[04:17] <thomi> wgrant: Does http://lpqateam.canonical.com/qa-reports/deployment-stable.html limit the number of grey revisions? I expected to see two of mine at the bottom of the list, but I only see one...
[04:17] <StevenK> qastaging may not have updated
[04:17] <StevenK> Check what rev is running on qas
[04:20] <thomi> StevenK: doh - thanks, that should have been obvious :-/
[04:20] <StevenK> thomi: devpad has a log of what qas is doing, but I suspect WADL'ing
[04:21] <thomi> devpad.canonical.com gives me an empty directory listing :D
[04:23] <StevenK> thomi: Via ssh
[04:23] <thomi> oh, gotchya
[04:24] <StevenK> Somewhere under /srv
[04:24] <thomi> anyway, it's EOD for me, I'll care about it tomorrow if I have to :D
[07:11] <blr> wgrant_: api-refs branch ready for re-review when you have a moment.
[11:47] <cjwatson> wgrant: Oh, um, could you have a look at https://code.launchpad.net/~cjwatson/turnip/pastescript/+merge/251303 ?  I just realised that I merged https://code.launchpad.net/~cjwatson/charms/trusty/turnip/api-server/+merge/251305 too early.
[11:47] <cjwatson> sigh Mondays
[11:48] <wgrant> cjwatson: You should totally have self-reviewed that, but done.
[11:48] <cjwatson> Fair enough
[11:48] <cjwatson> Ta
[15:30] <sil2100> Hey!
[15:31] <sil2100> I'm experiencing some strange problems, I don't seem to be able to subscribe the 'landing-team-trackers' team to bug LP: #1425737
[15:31] <mup> Bug #1425737: Wizard freezes on blank screen after language <vivid> <vivid-stab-candidate> <libqofono (Ubuntu):New> <ofono (Ubuntu):New> <unity8 (Ubuntu):New for mterry> <https://launchpad.net/bugs/1425737>
[15:31] <sil2100> ...aaaand nevermind!
[15:32] <sil2100> Scratch  that, it seems to have been some temprorary issue
[15:32] <sil2100> I was worried since refreshing didn't help and I tried like 5 times
[15:32] <sil2100> But now it's working again ;)
[17:39] <thomi> morning
[17:53] <cjwatson> Hi
[17:54] <cjwatson> I was wondering if we could have our team meeting today again this week, but neglected to ask people in my morning ...
[17:54]  * cjwatson grumbles at complex and not-entirely-predictable personal life conflicts but that's evening meetings for you
[17:54] <thomi> cjwatson: well I'm officially a CI person this week, so don't mind me :D
[17:55] <cjwatson> Aha
[17:55] <cjwatson> Splitter
[17:55] <thomi> and I doubt blr is up yet
[17:55] <thomi> yeah
[18:04] <cjwatson> I've spent half the day fighting (successfully, eventually) with juju, and the other half writing LP git xmlrpc tests
[18:05] <thomi> "xmlrpc" - that's a technology I haven't heard in a long time :D
[18:06] <thomi> ...reminds me of automating chyron devices
[18:08] <cjwatson> Yeah, well, we don't use it for talking from LP to turnip, but when talking from turnip to LP it's a lot easier than inventing a new endpoint stack
[18:08] <thomi> ahh, right
[18:09] <cjwatson> (Because we already have a private XML-RPC port that's used by several privileged services)
[22:46] <wgrant> cjwatson, blr: Today works for me.
[22:50] <blr> morning wgrant :)
[22:50] <blr> missing context, but I'm guessing that was our HO today rather than tomorrow?
[22:52] <wgrant> blr: Right, Colin suggested it.
[22:52] <wgrant> Though he may be gone now.
[22:55] <blr> sure, I'm easy
[22:59] <cjwatson> Here
[22:59] <wgrant> Aha, good morning.
[22:59] <cjwatson> Was just getting a bit of relaxing reading in beforehand
[23:01] <blr> cjwatson: oh?
[23:03] <cjwatson> "Spin State", Chris Moriarty