wgrantIt's too easy for a factory bug or change to invalidate the test otherwise.00:00
thomiwgrant: new diff ready...00:03
thomiI guess I got the zcml stuff correct? I wasn't really sure about that...00:04
wgrantYep, that's right.00:04
thomiis securedutility something launchpad specific? I couldn't find it in http://docs.zope.org/zope.component/zcml.html00:06
wgrantYes, that's one of ours.00:09
wgrantSee SecuredUtilityDirective in lp.services.webapp.metazcml00:10
wgrantBut in general you shouldn't have to care about the details.00:10
thomiok.. blindly copy + paste. check :D00:15
blrwgrant: 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
wgrantblr: 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
blrwgrant: 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
wgrantRight, the view presumably keys on refs/, so it doesn't get that part of the path.00:21
blrwgrant: well, when we need to address paging we can revisit it00:21
wgrantthomi: Approved with a couple of trivial fixes.00:33
* thomi looks00:33
wgrantthomi: Can you file an RT ticket to get your OpenPGP key added to the PQM keyring, so you can land things yourself?00:33
wgrantMail it to launchpad@ and I'll approve it.00:33
thomiwgrant: I file the rt by emailing launchpad@rt.canonical.com ?00:34
thomiwgrant: filed00:36
wgrantGot it, thanks.00:38
thomiwgrant: OK, I made those changes. Should to top-approve that MP, or would you like to look at it again?00:41
thomiI guess the PQM keyring won't get updated today00:41
wgrantthomi: 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
wgrantLooks fine to me. I'll land it unless you have objections to your own branch.00:46
thomino objections here :D00:46
wgrantGood, I'd be worried.00:46
thomiwgrant: 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/25139701:17
wgrantthomi: zope_isinstance is just an isinstance that looks through security proxies.01:19
wgrantDo the tests pass? I think that change is incorrect.01:19
wgrantThe structure here is a little bit confusing.01:19
thomiwgrant: the tests pass01:19
thomiI'm running lp.soyuz.browser.tests.test_builder_views.TestgetSpecificJobs01:20
thomiwgrant: I'm not sure what the word 'specific' means in that docstring01:20
wgrantthomi: What about test_builder_views as a whole?01:20
wgrantthomi: There are four types of build farm jobs. IBinaryPackageBuild, ISourcePackageRecipeBuild, ITranslationTemplatesBuild and ILiveFSBuild are all derivatives of IBuildFarmJob.01:21
wgrantBut, crucially, they are *specific* jobs.01:21
wgrantThey each have their own table named after them, and the data is primarily stored there.01:22
thomiwgrant: I'm running those tests now01:22
wgrantBut 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
thomiI'm still not sure what specific means in that context.01:22
thomiwgrant: those tests all pass as well :D01:22
blrwgrant: just as a wee experiment, all of the refs in the linux git repo, serialized are 9.6k01:22
wgrantBuildFarmJob is effectively an abstract base class.01:22
wgrantExcept it happens to actually exist for DB reasons.01:22
wgrantthomi: 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
wgrantI was hoping a test would fail to prove that, but to preserve the behaviour you can check against IBuildFarmJobDB instead.01:24
wgrantOnly BuildFarmJob itself implements IBuildFarmJobDB.01:24
wgrant(BuildFarmJob should probably be renamed to BuildFarmJobDB, but historical reasons)01:24
wgrantSpecific jobs have a non-specific BuildFarmJob.01:25
wgrantSpecific and non-specific jobs implement IBuildFarmJob.01:25
wgrantOnly non-specific jobs implement IBuildFarmJobDB.01:25
thomiso... that check should be 'IBuildFarmJob.providedBy(job) and not IBuildFarmJobDB.providedBy(job)' ?01:26
wgrantzope_isinstance(job, BuildFarmJob) is only true for a non-specific job.01:26
wgrantSo I think it can be replaced with just "IBuildFarmJobDB.provideBy(job)"01:27
thomioh, right.01:27
thomihmmmm, I'll add a test for that failure case as well01:28
wgrantThis code and schema has been through three or four serious redesigns over the years, so the tests may well be dodgy.01:28
thomiyeah... some of these could be better named01:29
* thomi gets out his yak razor01:29
wgrantBut 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
wgrantObviously for a specific job that transformation is a no-op.01:29
wgrantIt 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
wgrantblr: We'll need to handle non-ASCII refs soon, but that can be a subsequent iteration.01:35
blrwgrant: ugh, right.01:37
blrI think #git exists primarily to slag off other dvcses, sadly.01:41
wgrantWell, what else are they going to talk about? How good git's UI is? :)01:42
blrwgrant: noooo... don't do it.01:44
xnoxthey are quite helpful with git intrisics support.01:49
wgrantEvening xnox.02:06
thomiwgrant: if you have a moment: https://code.launchpad.net/~thomir/launchpad/devel-fix-import-violations-buildfarmjob/+merge/25139702:18
wgrantthomi: Done.02:19
thomiwgrant: I was tempted to rewrite that function, as I think i could be a lot clearer, but decided not to.02:19
thomithanks... down to 5 violations now :D02:20
wgrantthomi: 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
thomiyeah... 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 :D02:21
wgrantthomi: Oh, I was about to land it for you, but blahdeblah has added you to the keyring.02:22
wgrantthomi: 'bzr lp-land --no-qa' in the branch directory should do what you want.02:22
wgrantRun it from your host, not inside the container, for keyring access.02:22
thomioh, well that's easy then02:22
wgrantYou may need to grab bzr-pqm from the PPA, though.02:22
wgrantAnd you'll need SMTP server config.02:23
thomithe container has access to gpg keyring too...02:23
thomioh.. smtp thing will be "fun"02:23
wgrantthomi: It needs launchpadlib keys too.02:23
wgrantWhich are in your gnome-keyring.02:23
wgrantthomi: https://pastebin.canonical.com/126638/ has a superset of the relevant settings from my bzr configs.02:25
thomihow come no bzr-pqm package in utopic, but exists in saucy?02:26
wgrantthomi: Because bzr and LP are probably the only two projects in the world that still use PQM.02:26
thomioh ok02:26
wgrantThe saucy package should work, though.02:26
thomithe package in ppa:launchpad/ppa should as well I guess02:27
wgrantAh yes, I copied that up, didn't I.02:28
* wgrant lunches.02:35
wgrantthomi: Looks like you got lp-land working?02:58
thomiwgrant: 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
wgrantthomi: Heh, check your email.02:58
thomioh, right02:58
wgrantYou set from Merged to Approved, I guess you raced with the scanner.02:58
thomiI was about 10 seconds too early :D02:58
thomiwgrant: I wanted to ask you about the final few import violations02:59
thomithey're (almost) all where we're using load_related for (I think) database efficiency02:59
wgrantYup, that's what I thought they'd mostly be.02:59
wgrantThat's usually done with a DecoratedResultSet using pre_iter_hook.03:00
wgrantMany 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
wgrantOr some other sets have a preloadForWhatever method.03:01
thomiI'm not sure what the fix is for that - just move the code into a SomethingSet ?03:01
thomifor example, lp.soyuz.browser.archivesubscription line 334 (or thereabouts)03:01
wgrantthomi: 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
wgrantLoading a handful of archives is relatively cheap, so it's not terrible to do it where it's not necessary.03:03
wgrantBut other types of preloading definitely want to be up to the caller to enable.03:03
wgrantDo you understand what all the preloading code is for?03:09
wgrantIt's to prevent the ORM from issuing a bazillion queries when accessing objects related to sets of other objects.03:10
wgrantEach 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
wgrantThis is not good for performance.03:11
wgrantload_related collects the IDs and does a single query for them all, to get them into Storm's cache cheaply.03:11
thomisomehwat unrelated, but presumably it'd only load the archive details when you accessed that part of the subscription object, right?03:11
wgrantRight, code like "subscription.archive" pulls it in.03:12
wgrantBut 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
thomiok.. I think I just need to get my head around how this code gets moved around03:13
wgrantYou can probably just literally move the load_related call into getBySubscriberWithActiveToken, except that security proxies may ruin your day.03:13
thomiso getBySubscriberWithActiveToken would include the load_related call and throw away the result? I mean, it'd still be in the storm cache03:14
=== micahg_ is now known as micahg
thomioh, 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
wgrantthomi: afk, but yes. once it's loaded the view can just loop through the subscriptions and get the archive out03:19
thomiawesome. There are some other things I can change in here as well.03:19
wgrantyep, clean away03:20
thomimy brain is melting03:31
thomiugh. given that it's EOD for me, and I turn into a CI-team pumpkin tomorrow, I might back away sloooowly from this one :D03:33
wgrantthomi: heh, no worries. you got a couple of good branches landed today :)03:49
thomiwgrant: 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
StevenKqastaging may not have updated04:17
StevenKCheck what rev is running on qas04:17
thomiStevenK: doh - thanks, that should have been obvious :-/04:20
StevenKthomi: devpad has a log of what qas is doing, but I suspect WADL'ing04:20
thomidevpad.canonical.com gives me an empty directory listing :D04:21
StevenKthomi: Via ssh04:23
thomioh, gotchya04:23
StevenKSomewhere under /srv04:24
thomianyway, it's EOD for me, I'll care about it tomorrow if I have to :D04:24
blrwgrant_: api-refs branch ready for re-review when you have a moment.07:11
=== jamesh__ is now known as jamesh
cjwatsonwgrant: 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
cjwatsonsigh Mondays11:47
wgrantcjwatson: You should totally have self-reviewed that, but done.11:48
cjwatsonFair enough11:48
sil2100I'm experiencing some strange problems, I don't seem to be able to subscribe the 'landing-team-trackers' team to bug LP: #142573715:31
mupBug #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
sil2100Scratch  that, it seems to have been some temprorary issue15:32
sil2100I was worried since refreshing didn't help and I tried like 5 times15:32
sil2100But now it's working again ;)15:32
cjwatsonI 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 you17:54
thomicjwatson: well I'm officially a CI person this week, so don't mind me :D17:54
thomiand I doubt blr is up yet17:55
cjwatsonI've spent half the day fighting (successfully, eventually) with juju, and the other half writing LP git xmlrpc tests18:04
thomi"xmlrpc" - that's a technology I haven't heard in a long time :D18:05
thomi...reminds me of automating chyron devices18:06
cjwatsonYeah, 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 stack18:08
thomiahh, right18: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
wgrantcjwatson, blr: Today works for me.22:46
blrmorning wgrant :)22:50
blrmissing context, but I'm guessing that was our HO today rather than tomorrow?22:50
wgrantblr: Right, Colin suggested it.22:52
wgrantThough he may be gone now.22:52
blrsure, I'm easy22:55
wgrantAha, good morning.22:59
cjwatsonWas just getting a bit of relaxing reading in beforehand22:59
blrcjwatson: oh?23:01
cjwatson"Spin State", Chris Moriarty23:03

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