| wgrant | It's too easy for a factory bug or change to invalidate the test otherwise. | 00:00 |
|---|---|---|
| thomi | right | 00:00 |
| thomi | wgrant: new diff ready... | 00:03 |
| thomi | I guess I got the zcml stuff correct? I wasn't really sure about that... | 00:04 |
| wgrant | Yep, that's right. | 00:04 |
| thomi | is securedutility something launchpad specific? I couldn't find it in http://docs.zope.org/zope.component/zcml.html | 00:06 |
| wgrant | Yes, that's one of ours. | 00:09 |
| wgrant | See SecuredUtilityDirective in lp.services.webapp.metazcml | 00:10 |
| wgrant | But in general you shouldn't have to care about the details. | 00:10 |
| thomi | ok.. blindly copy + paste. check :D | 00:15 |
| 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:17 |
| wgrant | blr: Right, a list of dicts with a unique "name" field feels like it should be a dict. | 00:20 |
| 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:21 |
| blr | thanks | 00:22 |
| 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:33 |
| thomi | wgrant: I file the rt by emailing launchpad@rt.canonical.com ? | 00:34 |
| wgrant | yep | 00:34 |
| thomi | wgrant: filed | 00:36 |
| wgrant | Got it, thanks. | 00:38 |
| 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:41 |
| 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:46 |
| thomi | heh | 00:47 |
| 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:17 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:24 |
| 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:25 |
| 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:26 |
| wgrant | So I think it can be replaced with just "IBuildFarmJobDB.provideBy(job)" | 01:27 |
| thomi | oh, right. | 01:27 |
| 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:28 |
| 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:29 |
| 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:30 |
| wgrant | blr: We'll need to handle non-ASCII refs soon, but that can be a subsequent iteration. | 01:35 |
| blr | wgrant: ugh, right. | 01:37 |
| blr | I think #git exists primarily to slag off other dvcses, sadly. | 01:41 |
| wgrant | Well, what else are they going to talk about? How good git's UI is? :) | 01:42 |
| blr | wgrant: noooo... don't do it. | 01:44 |
| xnox | they are quite helpful with git intrisics support. | 01:49 |
| wgrant | Evening xnox. | 02:06 |
| thomi | wgrant: if you have a moment: https://code.launchpad.net/~thomir/launchpad/devel-fix-import-violations-buildfarmjob/+merge/251397 | 02:18 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| wgrant | thomi: https://pastebin.canonical.com/126638/ has a superset of the relevant settings from my bzr configs. | 02:25 |
| 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:26 |
| thomi | the package in ppa:launchpad/ppa should as well I guess | 02:27 |
| wgrant | Ah yes, I copied that up, didn't I. | 02:28 |
| * wgrant lunches. | 02:35 | |
| 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:58 |
| 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. | 02:59 |
| 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:00 |
| 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:01 |
| 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:03 |
| thomi | hmmm | 03:09 |
| wgrant | Do you understand what all the preloading code is for? | 03:09 |
| wgrant | It's to prevent the ORM from issuing a bazillion queries when accessing objects related to sets of other objects. | 03:10 |
| 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:11 |
| 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:12 |
| 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:13 |
| 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 |
| === micahg_ is now known as micahg | ||
| 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:14 |
| 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:19 |
| wgrant | yep, clean away | 03:20 |
| thomi | my brain is melting | 03:31 |
| wgrant | :( | 03:31 |
| 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:33 |
| wgrant | thomi: heh, no worries. you got a couple of good branches landed today :) | 03:49 |
| thomi | yeah | 04:14 |
| 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:17 |
| 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:20 |
| thomi | devpad.canonical.com gives me an empty directory listing :D | 04:21 |
| StevenK | thomi: Via ssh | 04:23 |
| thomi | oh, gotchya | 04:23 |
| StevenK | Somewhere under /srv | 04:24 |
| thomi | anyway, it's EOD for me, I'll care about it tomorrow if I have to :D | 04:24 |
| blr | wgrant_: api-refs branch ready for re-review when you have a moment. | 07:11 |
| === jamesh__ is now known as jamesh | ||
| 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:47 |
| wgrant | cjwatson: You should totally have self-reviewed that, but done. | 11:48 |
| cjwatson | Fair enough | 11:48 |
| cjwatson | Ta | 11:48 |
| sil2100 | Hey! | 15:30 |
| 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:31 |
| 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 ;) | 15:32 |
| thomi | morning | 17:39 |
| cjwatson | Hi | 17:53 |
| 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:54 |
| cjwatson | Aha | 17:55 |
| cjwatson | Splitter | 17:55 |
| thomi | and I doubt blr is up yet | 17:55 |
| thomi | yeah | 17:55 |
| cjwatson | I've spent half the day fighting (successfully, eventually) with juju, and the other half writing LP git xmlrpc tests | 18:04 |
| thomi | "xmlrpc" - that's a technology I haven't heard in a long time :D | 18:05 |
| thomi | ...reminds me of automating chyron devices | 18:06 |
| 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:08 |
| cjwatson | (Because we already have a private XML-RPC port that's used by several privileged services) | 18:09 |
| === anthonyf is now known as Guest30629 | ||
| === Guest30629 is now known as anthonyjf | ||
| wgrant | cjwatson, blr: Today works for me. | 22:46 |
| blr | morning wgrant :) | 22:50 |
| blr | missing context, but I'm guessing that was our HO today rather than tomorrow? | 22:50 |
| wgrant | blr: Right, Colin suggested it. | 22:52 |
| wgrant | Though he may be gone now. | 22:52 |
| blr | sure, I'm easy | 22:55 |
| cjwatson | Here | 22:59 |
| wgrant | Aha, good morning. | 22:59 |
| cjwatson | Was just getting a bit of relaxing reading in beforehand | 22:59 |
| blr | cjwatson: oh? | 23:01 |
| cjwatson | "Spin State", Chris Moriarty | 23:03 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!