StevenKthumper: Hey, can you look at db-add-ifp-job again, when you have a tick?04:06
thumperStevenK: ok04:14
jtvmwhudson: maybe you can review this urgent but near-trivial fix?  https://code.edge.launchpad.net/~jtv/launchpad/bug-631259/+merge/3465104:49
jtv(Some would call it trivial but the term is overused)04:49
jtvmwhudson: you move quietly but surely; hadn't noticed your approval.  Thanks!05:05
jtvI'm just adding a test actually.  :)05:05
mwhudsonjtv: heh05:08
jtvI 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 proper05: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
henningejtv: I have to admit I was not really familiar with the fact that UPDATEs could be fed their values from SELECTs. cool08:23
henningejtv: What happens if a SELECT returns more than one row?08:23
jtvThere shouldn't be any SELECTs… did I make a mistake?08:24
jtvNope, there are no SELECTs.08:24
henningejtv: sorry, I am talking about anything after FROM ...08:24
henningejtv: 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
jtvhenninge: picks an arbitrary one.08:27
henningeAh, good for us ;_)08:27
jtvMay 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
henningejtv: I know we don't care, I was more worried about it breaking.08:28
henningethe query, I mean08:28
jtvIt'll work—I did it this way to keep the query simple to reduce risk of mistakes.08:28
henningejtv: I think you should leave the condition explicitly in the last two queries.08:30
jtvhenninge: I don't understand08:30
henningeyou know, POTemplate.iscurrent IS NOT TRUE and TranslationTemplateItem.sequence = 008:31
henningejtv: I am just thinking that will make it more transparent to others that will read these.08:32
jtvhenninge: that only introduces more complexity and so more danger of mistakes.  The condition "TranslationMessage.pofile" takes care of it.08:32
henningejtv: ok, it was just a thought.08:33
henningejtv: r=me08:33
jtvhenninge: great, thanks.08:34
jtvNow 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
jtvhenninge_: I was thanking you for the review of my queries08: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
lifelesshenninge_: hi09:20
=== henninge_ is now known as henninge
henningelifeless: I am sorry but I will have to refuse this one.09:28
lifelesshenninge: ok09:28
lifelesshenninge: may I ask why ?09:28
henningelifeless: 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
lifelessfair enough09:29
lifelessthats fine09:29
lifelessstub: ping09:29
stublifeless: pong09:29
lifelessstub: the librarian rework is ready for another review09:30
lifelessstub: code & db & all09:30
lifelessstub: I've just pushed up 9588 making the hostname check a little cleaner and fixing the build.09:32
stubThere are still changes in session.sql that should be in launchpad_session.sql09:32
stubThis is all Launchpad specific stuff09:33
lifelessI must have missed that09:33
lifelessmoving around09:33
stubPutting 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
lifelessI figured consolidated it into a function anywhere was better than making another callsite with the full spelling.09:35
stubre: 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
stubYour 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
stubcontecxts is not spelled that way09:39
stubI 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
lifelessI'll use 395960 for the xx bugs here09:41
lifelessall the same thing, its not done till its done09:41
lifelessthe xxx policy of needing a bug, is, IMCO a bit noddy, still, linked it to 39596009:42
lifelessre the assert09:42
lifelessif someone opens a page with an attachment09:42
lifelessand someone else deletes the attachment09:42
lifelessthen the user clicks on the link09:42
lifelesswe'll trigger an assert for a normal situation; that makes no sense to me.09:43
lifelessI've put that usecase in.09:44
stubline 239 of the diff is engrish09: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 diff09:50
stubProxiedLibraryFileAlias should be rolled into LibraryFileAlias eventually?09:52
lifelessI think it should be renamed09:52
lifelessnot sure about roll-in or not09:53
lifelessProxied... dates to sept last year09:53
stublifeless: 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
stubI don't think Storm uses notNull in the column definitions? IIRC that is SQLObject specific.09:56
stublifeless: I don't understand why a 16 character hex string is preferable to str(random.random())09:58
lifelessstub: running make schema with the tz change10:00
stubIf you want to make them less guessable, md5(baseline + str(time.time()) + url + thread.get_ident())10:01
stubI suspect the store.commit() is unnecessary as I think that connection is running in autocommit mode. I haven't double checked though.10:01
lifelessstub: it seemed nicer10:02
stubSo the only purpose it serves it to issue a redundant call to the database.10:02
lifelessthe store.commit won't hit the db will it? no changes - noop ?10:02
stubI guess the alternative is flush10:02
stublifeless: Hmm... I'm not sure. I guess Storm will know its a noop and noop it.10:03
lifelessanyhow, its there as belt and bracers, can definitely take it out10:03
stublifeless: Any reason we are only using multidomain stuff for restricted files?10:10
lifelessdns lookups10:11
lifelesslibrarian files are used for branding images10:11
lifelesswe don't want to cause dns lookups *and* SSL handshaking for every one of those10:11
stubok. I was thinking it might be desirable to trick browsers to load things in parallel, like team emblems.10:12
lifelessthree-way handshake + dns will swamp the benefits there10:14
lifelesswhat we should do is have a spite generator sort of thing10:14
lifelesswe can revisit in future obviously10:14
stubWhat happens now if a test module doesn't have a test_suite() function?10:14
lifelessthe tests are loaded via TestLoader().loadTestsFromModule10:14
jtvhenninge: could you review this testfix branch? https://code.edge.launchpad.net/~jtv/launchpad/testfix/+merge/3466310:15
lifelessi.e. it DTRT if the test_suite was just doing loadTestsFromName10:15
stubOk. I think I added test_suite to the template not to long ago :)10:15
jtvhenninge: the diff has appeared10:19
henningejtv: yes, I am on it10: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
stublifeless: So +    def getAlias(self, aliasid, token, path): accepts path as a parameter, and then proceeds to pretend it is a URL.10:22
stublifeless: 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
lifelessits a path only as per your previous comments10:23
lifelessI will update the table definition10:23
stubImports on diff line 1407 are badly formatted (layers.py)10:35
stubAnnoying that Librarian stuff has spread from just canonical.librarian to canonical.launchpad.database.librarian and other files10:37
stublifeless: But done. Nothing major I can see.10:37
jtvhenninge: any progress with my branch?10:43
henningejtv: sorry, distracted.10:43
jtvhenninge: not a good thing while reviewing a testfix!  :)10:43
henningejtv: I was just looking closer at one thing.10:43
henningejtv: r=me10:44
jtvIt's in pqm.  I'm taking a break for urgent non-work stuff.10:45
jtvgmb, 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/3466310:47
gmbjtv, Landing it on devel, perchance?10:49
jtvgmb: yes10:50
jtvthat's the build it fixes10:50
gmbjtv, Right. Yes, you need RC. And you have it.10:50
jtvgmb: many thanks!10:50
jtv…on the way.10:50
=== 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_posterhm.  no-one is here to review my poor little MP https://code.edge.launchpad.net/~gary/launchpad/bug627442/+merge/34701 :-/19:58
gary_posterthank yoU!20:08
lifelessgary_poster: needs an rc stamp20:09
lifelessalso I need to sit on thumper and convince him to ok the use of queues.20:09
gary_posterlifeless, thanks.  will ping gmb.20:10
lifelessif he's not around, any other TL will do20:10
lifelessor we can get someone else to do the code review and I'll make my stamp a release-critical20:10
lifeless(lp only allows one type of review per reviewer at the moment, ce la BUG20:10
=== Ursinha is now known as Ursinha-brb
thumperlifeless, gary_poster: it is ok for someone to give an rc and a review21:04
thumperit is just the landing script that is not smart enough21:04
lifelessthumper: it would be nice if lp could store it though21:06
lifelessgary_poster: hi21:26
gary_posterlifeless, hi21:26
lifelessI've fixed the remaining test failure in my librarian MP, and need the commits-for-that to be reviewed ;)21:27
gary_posterheh, ok, where do I look?21:27
lifelesshttps://code.edge.launchpad.net/~lifeless/launchpad/private-librarian/+merge/31020 is the MP21:27
lifelessI'll get you specific commit links in a sec21:27
gary_posterk thanks21:28
lifeless(lp is just scanning it)21:28
lifelesshttp://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
lifelesshttp://bazaar.launchpad.net/~lifeless/launchpad/private-librarian/revision/9595 I found a missing/unclear thing in feature flags which I just tweaked.21:30
lifelessthey are all tiny, just split up for clarity21:31
lifelesshowever, loggerhead looks to be on the way down21:32
lifelessso perhaps just pull lp:~lifeless/launchpad/private-librarian and do 'bzr log -p | less and look at the top 4 patches21:32
lifelessgary_poster: ^21:34
gary_posterack, looking21:34
gary_posterlifeless, 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, +121:40
lifelessgary_poster: yes21:40
lifelessgary_poster: we need to extend the concept of annotatable things to scripts21:40
lifelessgary_poster: thats the sticking pointy bit21:40
gary_posterscripts, or the requests that scripts use?21:41
lifelesswell, scripts don't used requests today.21:41
gary_posterno, "participations"21:41
lifelessyes, participation is the leverage point we should use to sort this mess out21:41
gary_postercool, agreed21:41
lifelessright now scripts with with a special security policy21:42
lifelessand a single noddy participation21:42
lifelesswhat they need to do is switch participations when doing work-on-behalf-of-something-else21:42
lifelessthe 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
lifelessbasically, it went something like this21:44
lifelessearly scripts used different domain objects - e.g. sql21:45
lifelessthat was bad21:45
lifelessso bits of zope were stubbed up enough to use the same sqlobject objects21:45
lifelessthat had issues21:45
lifelessso more was included21:45
lifeless'parseZCMLForScripts' or whatever21:45
lifelessanyhow, its all grown organically.21:45
gary_posterGotcha.  yeah, t happens.21:46
gary_posterfrom 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
lifelessso we should IMO use the same security framework and policies regardless of context.21:46
gary_posterSounds great to me.21:46
lifelessgary_poster: no other code is doing it yet.21:46
lifelessnext one along can make it a helper - or we may have the bug fixed to not need it earlier.21:47
gary_posterI have a had a card to try to audit the security we have in views and see what we can do to rip it out21:47
gary_posterit is a problem21:47
gary_posteri have not gotten around to it21:47
lifelesswhat sort of stuff are you referring to there?21:47
lifelessthings like stashing 'this is ok' directly into the permission cache?21:48
gary_posterlike, 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
lifelessah yes21:49
lifelessthey seem ugly to me21:49
gary_posterThis makes our automated security worse than nothing in some cases21:49
gary_posterBecause it lulls into a false sense of security21:49
gary_posterfor the webservice for instance21:49
lifelessI'd like to get us really using zope permissions as properly as possible.21:50
gary_posterbut also across domain knowledge, or even across forgetfullness21: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 way21:50
lifelessgary_poster: I agree completely21:50
gary_posterback to review ;-)21:51
gary_posterlifeless: 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
lifelessgary_poster: thanks!21:59
=== 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
lifelessI 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/3470923:41
lifelessthumper: mwhudson: I pick thee ^23:44
thumperlifeless: done23:44
thumperrc anyway23:44
thumperyou could submit with rc and r = me if you like23:44
* lifeless does that23:52
lifelessdamn the torpedoes!23:52

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