| StevenK | thumper: Hey, can you look at db-add-ifp-job again, when you have a tick? | 04:06 |
|---|---|---|
| thumper | StevenK: ok | 04:14 |
| jtv | mwhudson: maybe you can review this urgent but near-trivial fix? https://code.edge.launchpad.net/~jtv/launchpad/bug-631259/+merge/34651 | 04:49 |
| jtv | (Some would call it trivial but the term is overused) | 04:49 |
| jtv | mwhudson: you move quietly but surely; hadn't noticed your approval. Thanks! | 05:05 |
| jtv | I'm just adding a test actually. :) | 05:05 |
| mwhudson | jtv: heh | 05:08 |
| mwhudson | yeah | 05:08 |
| mwhudson | oops | 05:08 |
| jtv | I don't think it matters much since I just restored something that's been in production for years, and the tests have to pass since we just removed everything that relies on this field, but it looks more proper | 05:10 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| henninge | jtv: I have to admit I was not really familiar with the fact that UPDATEs could be fed their values from SELECTs. cool | 08:23 |
| henninge | jtv: What happens if a SELECT returns more than one row? | 08:23 |
| jtv | There shouldn't be any SELECTs… did I make a mistake? | 08:24 |
| jtv | Nope, there are no SELECTs. | 08:24 |
| henninge | jtv: sorry, I am talking about anything after FROM ... | 08:24 |
| henninge | jtv: like the second query (for ideal cases) where the condition could be true for more than one POFILE.id (and it's not unlikely I think.) | 08:26 |
| jtv | henninge: picks an arbitrary one. | 08:27 |
| henninge | Ah, good for us ;_) | 08:27 |
| jtv | May even do multiple updates of the same record, but frankly I just don't care. The result is still the same, apart from performance. | 08:28 |
| henninge | jtv: I know we don't care, I was more worried about it breaking. | 08:28 |
| henninge | the query, I mean | 08:28 |
| jtv | It'll work—I did it this way to keep the query simple to reduce risk of mistakes. | 08:28 |
| henninge | jtv: I think you should leave the condition explicitly in the last two queries. | 08:30 |
| jtv | henninge: I don't understand | 08:30 |
| henninge | you know, POTemplate.iscurrent IS NOT TRUE and TranslationTemplateItem.sequence = 0 | 08:31 |
| henninge | jtv: I am just thinking that will make it more transparent to others that will read these. | 08:32 |
| jtv | henninge: that only introduces more complexity and so more danger of mistakes. The condition "TranslationMessage.pofile" takes care of it. | 08:32 |
| henninge | jtv: ok, it was just a thought. | 08:33 |
| henninge | jtv: r=me | 08:33 |
| jtv | henninge: great, thanks. | 08:34 |
| jtv | Now to get my branch in somehow. :-( | 08:34 |
| henninge_ | jtv: as you can tell, I did not do anything about it yet ... | 08:34 |
| henninge_ | but I heard your thanks. | 08:35 |
| jtv | henninge_: I was thanking you for the review of my queries | 08:35 |
| === henninge_ changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| lifeless | henninge_: hi | 09:20 |
| lifeless | https://code.edge.launchpad.net/~lifeless/launchpad/private-librarian/+merge/31020 | 09:20 |
| === henninge_ is now known as henninge | ||
| henninge | lifeless: I am sorry but I will have to refuse this one. | 09:28 |
| lifeless | henninge: ok | 09:28 |
| lifeless | henninge: may I ask why ? | 09:28 |
| henninge | lifeless: besides it being oversized it would take me all day to get into it and I think Stuart can do that much more efficiently. | 09:29 |
| lifeless | fair enough | 09:29 |
| henninge | sorry | 09:29 |
| lifeless | thats fine | 09:29 |
| lifeless | stub: ping | 09:29 |
| stub | lifeless: pong | 09:29 |
| lifeless | stub: the librarian rework is ready for another review | 09:30 |
| stub | k | 09:30 |
| lifeless | stub: code & db & all | 09:30 |
| lifeless | stub: I've just pushed up 9588 making the hostname check a little cleaner and fixing the build. | 09:32 |
| stub | k | 09:32 |
| stub | There are still changes in session.sql that should be in launchpad_session.sql | 09:32 |
| stub | This is all Launchpad specific stuff | 09:33 |
| lifeless | I must have missed that | 09:33 |
| lifeless | moving around | 09:33 |
| lifeless | pushing | 09:34 |
| stub | Putting session_store() in sqlbase is naughty cause it should really be in lp.*mumble*. But I do that too because foundations has never really been clear on where its stuff should live. | 09:34 |
| lifeless | I figured consolidated it into a function anywhere was better than making another callsite with the full spelling. | 09:35 |
| stub | re: the comment in MixedFileAliasView, you might want to consider opening bugs on the tech-debt work (remove the old code one we are sure things work with the feature flag on, consolidate it into LibraryFileAliasView). The bug can then be referenced as an XXX: | 09:37 |
| stub | Your XXX just below that is naughty as it isn't referencing a bug. If we need to fix it, open a bug. If not, it isn't an XXX but a comment. | 09:38 |
| stub | contecxts is not spelled that way | 09:39 |
| stub | I think assert is fine over 404, as the view should never be called for deleted files and we want bad links like that to show up on OOPS reports. | 09:41 |
| lifeless | I'll use 395960 for the xx bugs here | 09:41 |
| lifeless | all the same thing, its not done till its done | 09:41 |
| lifeless | the xxx policy of needing a bug, is, IMCO a bit noddy, still, linked it to 395960 | 09:42 |
| lifeless | re the assert | 09:42 |
| lifeless | if someone opens a page with an attachment | 09:42 |
| lifeless | and someone else deletes the attachment | 09:42 |
| lifeless | then the user clicks on the link | 09:42 |
| lifeless | we'll trigger an assert for a normal situation; that makes no sense to me. | 09:43 |
| lifeless | I've put that usecase in. | 09:44 |
| stub | line 239 of the diff is engrish | 09:49 |
| lifeless | +from hashlib import md5? | 09:49 |
| stub | + LibraryFileAlias directly is OK as at that point the status of the file239+ is know. | 09:50 |
| * stub reloads the diff | 09:50 | |
| stub | ProxiedLibraryFileAlias should be rolled into LibraryFileAlias eventually? | 09:52 |
| lifeless | I think it should be renamed | 09:52 |
| lifeless | LibraryFileAliasReference | 09:52 |
| lifeless | not sure about roll-in or not | 09:53 |
| lifeless | Proxied... dates to sept last year | 09:53 |
| stub | lifeless: Your database patch uses 'timestamp with time zone' but your storm class is using UtcDateTime. I think your DB patch should be 'timestamp without time zone' to match the storm class and our existing timestamp columns. | 09:54 |
| stub | I don't think Storm uses notNull in the column definitions? IIRC that is SQLObject specific. | 09:56 |
| stub | lifeless: I don't understand why a 16 character hex string is preferable to str(random.random()) | 09:58 |
| lifeless | stub: running make schema with the tz change | 10:00 |
| stub | If you want to make them less guessable, md5(baseline + str(time.time()) + url + thread.get_ident()) | 10:01 |
| stub | I suspect the store.commit() is unnecessary as I think that connection is running in autocommit mode. I haven't double checked though. | 10:01 |
| lifeless | stub: it seemed nicer | 10:02 |
| stub | So the only purpose it serves it to issue a redundant call to the database. | 10:02 |
| lifeless | the store.commit won't hit the db will it? no changes - noop ? | 10:02 |
| stub | I guess the alternative is flush | 10:02 |
| stub | lifeless: Hmm... I'm not sure. I guess Storm will know its a noop and noop it. | 10:03 |
| lifeless | anyhow, its there as belt and bracers, can definitely take it out | 10:03 |
| stub | lifeless: Any reason we are only using multidomain stuff for restricted files? | 10:10 |
| lifeless | yes | 10:11 |
| lifeless | dns lookups | 10:11 |
| lifeless | librarian files are used for branding images | 10:11 |
| lifeless | we don't want to cause dns lookups *and* SSL handshaking for every one of those | 10:11 |
| stub | ok. I was thinking it might be desirable to trick browsers to load things in parallel, like team emblems. | 10:12 |
| lifeless | three-way handshake + dns will swamp the benefits there | 10:14 |
| lifeless | what we should do is have a spite generator sort of thing | 10:14 |
| lifeless | we can revisit in future obviously | 10:14 |
| stub | What happens now if a test module doesn't have a test_suite() function? | 10:14 |
| lifeless | the tests are loaded via TestLoader().loadTestsFromModule | 10:14 |
| jtv | henninge: could you review this testfix branch? https://code.edge.launchpad.net/~jtv/launchpad/testfix/+merge/34663 | 10:15 |
| lifeless | i.e. it DTRT if the test_suite was just doing loadTestsFromName | 10:15 |
| stub | Ok. I think I added test_suite to the template not to long ago :) | 10:15 |
| lifeless | :P | 10:16 |
| jtv | henninge: the diff has appeared | 10:19 |
| henninge | jtv: yes, I am on it | 10:19 |
| jtv | thanks | 10:19 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| stub | lifeless: So + def getAlias(self, aliasid, token, path): accepts path as a parameter, and then proceeds to pretend it is a URL. | 10:22 |
| stub | lifeless: I think you need to make your mind up and change the parameter to URL and tweak the documentation so it matches reality. | 10:22 |
| lifeless | its a path only as per your previous comments | 10:23 |
| lifeless | I will update the table definition | 10:23 |
| stub | Imports on diff line 1407 are badly formatted (layers.py) | 10:35 |
| stub | Annoying that Librarian stuff has spread from just canonical.librarian to canonical.launchpad.database.librarian and other files | 10:37 |
| stub | lifeless: But done. Nothing major I can see. | 10:37 |
| lifeless | thanks | 10:38 |
| jtv | henninge: any progress with my branch? | 10:43 |
| henninge | jtv: sorry, distracted. | 10:43 |
| jtv | henninge: not a good thing while reviewing a testfix! :) | 10:43 |
| henninge | jtv: I was just looking closer at one thing. | 10:43 |
| henninge | sorry | 10:43 |
| henninge | jtv: r=me | 10:44 |
| jtv | thanks | 10:44 |
| jtv | It's in pqm. I'm taking a break for urgent non-work stuff. | 10:45 |
| jtv | gah | 10:46 |
| jtv | gmb, I'm not sure but I _think_ the PQM error message says I need RC to land this testfix: https://code.edge.launchpad.net/~jtv/launchpad/testfix/+merge/34663 | 10:47 |
| gmb | jtv, Landing it on devel, perchance? | 10:49 |
| jtv | gmb: yes | 10:50 |
| jtv | that's the build it fixes | 10:50 |
| gmb | jtv, Right. Yes, you need RC. And you have it. | 10:50 |
| gmb | :) | 10:50 |
| jtv | gmb: many thanks! | 10:50 |
| jtv | …on the way. | 10:50 |
| gmb | #\0/ | 10:51 |
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: lunch || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| === lindbohm.freenode.net changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| === Ursinha-afk is now known as Ursinha | ||
| === matsubara-afk is now known as matsubara | ||
| === bigjools is now known as bigjools-lunch | ||
| === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| === bigjools-lunch is now known as bigjools | ||
| === Ursinha is now known as Ursinha-afk | ||
| === Ursinha-afk is now known as Ursinha | ||
| === Ursinha is now known as Ursinha-lunch | ||
| === henninge changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews || gmb is RM; for RC reviews please add him as a reviewer | ||
| === matsubara is now known as matsubara-lunch | ||
| === salgado is now known as salgado-lunch | ||
| === matsubara-lunch is now known as matsubara | ||
| === Ursinha-lunch is now known as Ursinha | ||
| === benji is now known as benji-lunch | ||
| === salgado-lunch is now known as salgado | ||
| === benji-lunch is now known as benji | ||
| gary_poster | hm. no-one is here to review my poor little MP https://code.edge.launchpad.net/~gary/launchpad/bug627442/+merge/34701 :-/ | 19:58 |
| lifeless | looking | 20:04 |
| gary_poster | thank yoU! | 20:08 |
| lifeless | gary_poster: needs an rc stamp | 20:09 |
| lifeless | also I need to sit on thumper and convince him to ok the use of queues. | 20:09 |
| gary_poster | lifeless, thanks. will ping gmb. | 20:10 |
| lifeless | if he's not around, any other TL will do | 20:10 |
| lifeless | or we can get someone else to do the code review and I'll make my stamp a release-critical | 20:10 |
| lifeless | (lp only allows one type of review per reviewer at the moment, ce la BUG | 20:10 |
| === Ursinha is now known as Ursinha-brb | ||
| thumper | lifeless, gary_poster: it is ok for someone to give an rc and a review | 21:04 |
| thumper | it is just the landing script that is not smart enough | 21:04 |
| lifeless | thumper: it would be nice if lp could store it though | 21:06 |
| lifeless | gary_poster: hi | 21:26 |
| gary_poster | lifeless, hi | 21:26 |
| lifeless | I've fixed the remaining test failure in my librarian MP, and need the commits-for-that to be reviewed ;) | 21:27 |
| gary_poster | heh, ok, where do I look? | 21:27 |
| lifeless | https://code.edge.launchpad.net/~lifeless/launchpad/private-librarian/+merge/31020 is the MP | 21:27 |
| lifeless | I'll get you specific commit links in a sec | 21:27 |
| gary_poster | k thanks | 21:28 |
| lifeless | (lp is just scanning it) | 21:28 |
| lifeless | http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9594 was abels fix to a test script I've reviewed this, so strictly it doesn't need more eyeballs. OTOH eyeballs are valuable ;) | 21:29 |
| lifeless | http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9595 I found a missing/unclear thing in feature flags which I just tweaked. | 21:30 |
| lifeless | http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9596 | 21:30 |
| lifeless | http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9597 | 21:30 |
| lifeless | http://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9598 | 21:30 |
| lifeless | they are all tiny, just split up for clarity | 21:31 |
| lifeless | however, loggerhead looks to be on the way down | 21:32 |
| lifeless | so perhaps just pull lp:~lifeless/launchpad/private-librarian and do 'bzr log -p | less and look at the top 4 patches | 21:32 |
| lifeless | gary_poster: ^ | 21:34 |
| gary_poster | ack, looking | 21:34 |
| lifeless | thanks | 21:36 |
| gary_poster | lifeless, IIUC, the current thought is that we want to make sure that .annotations is reliably on every request Launchpad code sees, and then we can do away with other thread locals that are compensating for that lack. Right? If so, +1 | 21:40 |
| lifeless | gary_poster: yes | 21:40 |
| gary_poster | cool | 21:40 |
| lifeless | gary_poster: we need to extend the concept of annotatable things to scripts | 21:40 |
| lifeless | gary_poster: thats the sticking pointy bit | 21:40 |
| gary_poster | scripts, or the requests that scripts use? | 21:41 |
| lifeless | well, scripts don't used requests today. | 21:41 |
| gary_poster | "interactions" | 21:41 |
| gary_poster | no, "participations" | 21:41 |
| lifeless | yes, participation is the leverage point we should use to sort this mess out | 21:41 |
| gary_poster | cool, agreed | 21:41 |
| lifeless | right now scripts with with a special security policy | 21:42 |
| lifeless | and a single noddy participation | 21:42 |
| gary_poster | heh | 21:42 |
| lifeless | what they need to do is switch participations when doing work-on-behalf-of-something-else | 21:42 |
| gary_poster | absolutely | 21:42 |
| lifeless | the special security policy says 'anything goes' | 21:42 |
| gary_poster | ...is that special security policy desirable, or legacy? Would a more careful policy buy us something worthwhile? | 21:43 |
| lifeless | legasheee | 21:44 |
| lifeless | basically, it went something like this | 21:44 |
| lifeless | early scripts used different domain objects - e.g. sql | 21:45 |
| lifeless | that was bad | 21:45 |
| lifeless | so bits of zope were stubbed up enough to use the same sqlobject objects | 21:45 |
| lifeless | 'zopeless' | 21:45 |
| lifeless | that had issues | 21:45 |
| lifeless | so more was included | 21:45 |
| lifeless | 'parseZCMLForScripts' or whatever | 21:45 |
| lifeless | anyhow, its all grown organically. | 21:45 |
| gary_poster | Gotcha. yeah, t happens. | 21:46 |
| gary_poster | from a review perspective, is any other test code currently doing the dance that you do in librarian.txt to work around bug 631884? If so, should it be a test helper? If not, never mind. | 21:46 |
| _mup_ | Bug #631884: feature flags get out of sync easily <Launchpad Foundations:New> <https://launchpad.net/bugs/631884> | 21:46 |
| lifeless | so we should IMO use the same security framework and policies regardless of context. | 21:46 |
| gary_poster | Sounds great to me. | 21:46 |
| lifeless | gary_poster: no other code is doing it yet. | 21:46 |
| gary_poster | ok | 21:46 |
| lifeless | next one along can make it a helper - or we may have the bug fixed to not need it earlier. | 21:47 |
| gary_poster | I have a had a card to try to audit the security we have in views and see what we can do to rip it out | 21:47 |
| gary_poster | it is a problem | 21:47 |
| gary_poster | i have not gotten around to it | 21:47 |
| lifeless | what sort of stuff are you referring to there? | 21:47 |
| lifeless | things like stashing 'this is ok' directly into the permission cache? | 21:48 |
| gary_poster | like, direct calls in views to checkPermission, where it is effectively an actual security protection. I have rarely been in the situation in which this made sense, but canWrite and canRead are safer when you need it, and I'm pretty sure we are abusing these sorts of things. | 21:49 |
| lifeless | ah yes | 21:49 |
| lifeless | they seem ugly to me | 21:49 |
| gary_poster | This makes our automated security worse than nothing in some cases | 21:49 |
| gary_poster | Because it lulls into a false sense of security | 21:49 |
| gary_poster | for the webservice for instance | 21:49 |
| lifeless | I'd like to get us really using zope permissions as properly as possible. | 21:50 |
| gary_poster | but also across domain knowledge, or even across forgetfullness | 21:50 |
| lifeless | *if* they aren't sufficient/performant-enough we can talk about what to do, but we can't really assess that while stuff is wedged in every which way | 21:50 |
| lifeless | gary_poster: I agree completely | 21:50 |
| gary_poster | cool | 21:51 |
| gary_poster | back to review ;-) | 21:51 |
| gary_poster | lifeless: typo "acces" in lib/canonical/launchpad/database/librarian.py : """Return the token path used for authorising acces to url.""" Everything else looks good. | 21:59 |
| lifeless | gary_poster: thanks! | 21:59 |
| gary_poster | np | 22:00 |
| === Ursinha-brb is now known as Ursinha | ||
| === salgado is now known as salgado-afk | ||
| === gary_poster_ is now known as gary_poster | ||
| === matsubara is now known as matsubara-afk | ||
| lifeless | I need a) a TL to release-critical and b) a code review for this 5-liner https://code.edge.launchpad.net/~lifeless/launchpad/oops/+merge/34709 | 23:41 |
| lifeless | thumper: mwhudson: I pick thee ^ | 23:44 |
| thumper | lifeless: done | 23:44 |
| thumper | rc anyway | 23:44 |
| thumper | you could submit with rc and r = me if you like | 23:44 |
| * lifeless does that | 23:52 | |
| lifeless | damn the torpedoes! | 23:52 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!