[00:55] lifeless: sure [01:14] thanks [01:34] lifeless: done [01:38] thanks [02:47] thumper: while you're at it, can you review this one for me? https://code.launchpad.net/~jtv/launchpad/recife-policy-mixin/+merge/40300 [02:48] (that was deliberately phrased for a flippant rebuttal, so feel free if it's not convenient) [02:48] jtv: if it was small I'd say yes, but I'm in the middle of something. Sorry. [02:49] no worries [02:49] * jtv scans for victims [02:49] bac! [02:49] Are you up for a non-small review? [02:49] jtv i can be. can it wait a little bit as i just started something? [02:50] bac: yes it can [02:51] ahhh the sweet smell of real urls in the morning [02:51] it smells like victory [02:58] * jtv finds this obvious crack in lifeless' sanity a good reason to ask: [02:59] you're suggesting a standalone class to implement translation policy. Were you thinking of a schema change? [02:59] jtv: I wasn't, but perhaps one is needed. [03:00] s/needed/possibly beneficial. [03:00] I see it making a lot of sense at this point. [03:00] we have -way- to many mixins [03:00] when you've got 1 mixin its a problem. 20+ mixings is arggggh zomg mode [03:01] Yes, we do have too many of them. [03:02] And in this case, there's actual state that's being replicated into multiple classes—basically a denormalized object design. [03:02] is there a good reason for that ? [03:05] Best I can come up with is "progressing insight." [03:05] In other words, "no but that may not have been so obvious at the time." [03:05] There's 2 identical fields in Product, ProjectGroup, and Distribution. [03:06] I'm trying to expose and unify the common patterns. [03:08] Extracting it into a separate database table could be a lot of work, of course. [03:08] Some joins will change shape. [03:09] The new objects' lifetimes have to be managed etc. [03:10] Too much work for now. I think I'll go with a standalone non-db class first. [03:24] lifeless: I'm looking at implementing this as an adapter. [03:25] lifeless: so you'd say "ITranslationPolicy(pillar).makeSomeDecision()" [03:25] but you can also adapt certain other objects to get the ITranslationPolicy that applies to them. [03:26] Does that make sense to you? [03:28] that seems a decent start [03:29] as for adapter vs pillar.translation_policy - shrug, its largely just spelling (except those pesky lifetimes that you mentioned) [03:36] Yes, and I don't just want to keep adding to the interfaces. [03:50] bac, lifeless: reworking the policy object so that it's not a mixin becomes too much work for this branch; I'll have leave that for a separate branch. [03:55] * jtv goes out for a bite === jtv is now known as jtv-eat === jtv-eat is now known as jtv [08:26] lifeless: Should https://code.launchpad.net/~stub/launchpad/update-storm/+merge/40264 go through without review? It consists entirely of unicode casts, updates to modern Storm syntax and delints at 2.4k lines of diff. [09:27] jtv: return the favor? [09:28] bac: honour compels me. [09:28] jtv: lucky for you i'm not as prolific. a measly 88 line diff. https://code.launchpad.net/~bac/launchpad/bug-671539/+merge/40309 [09:28] bac: prolific? I have more changes waiting that I carefully cut out of the diff to keep its size down. :-) [09:31] bac: I find the "if not person.is_team: enabled = True else: enabled = False" a bit hard to read. Have you considered this alternative? [09:31] owner_is_individual = (not person.is_team) [09:31] …(…, enabled=owner_is_invidual) [09:32] jtv: yeah, i'll make that change. [09:32] Thanks. [09:32] brain fart [10:21] any other comments, jtv? [10:21] bac: whoops, got sidetracked! Getting back to it now. [10:22] bac: Shouldn't the section heading in the story test be "---"-underlined instead of "==="-underlined? [10:22] (I do not like rest-style headings) [10:27] bac: approved. [10:36] jtv: i'll look at the formatting. i thought i had followed the style used in the file but perhaps i messed up [10:36] bac: or the previous author of the file got it wrong, or we just have a harmless inconsistency. === allenaptoo is now known as allenap === danilo_ is now known as danilos === matsubara-afk is now known as matsubara === Ursinha-afk is now known as Ursinha [12:50] Hey, who's up for a review that I actually tried to make fun? https://code.launchpad.net/~jtv/launchpad/independence-for-ihastranslationtemplates/+merge/40321 [12:51] matsubara maybe? [12:53] gmb then? you're always in for a laugh, no? [12:54] jtv, I'm not an official reviewer [12:54] ah! [12:54] well good morning, anyway. :) === jtv changed the topic of #launchpad-reviews to: On call: - || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [12:57] good morning :-) [13:00] Still holding that T-shirt. :) [13:03] henninge, do you want to review it then? [13:04] jtv: oh, yes I should ... [13:04] Since it's your day and all :) === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [13:04] Thanks. [13:04] Yes, I will have to find a replacement for next week. [13:04] We're not usually that picky about missing a review day. [13:04] I don't think I should be doing normal reviews *and* r-c reviews... [13:05] At least not on this side of the world, where usually there aren't any OCRs anyway. :) [13:05] henninge: why do you say normal reviews and r-c reviews? has there been a change that I'm unaware of? [13:06] jtv: I am release manager so I will be the one doing r-c reviews next Monday. [13:06] ah [13:06] I forgot that you were rc [13:06] rm [13:06] sorry, rm [13:06] rm -rf [13:07] In my world: Returned Missionary ;-) [13:07] (Seriously, I once saw Siemens advertise their OS called, if I remember the spelling correctly, RM/UNIX) [13:07] :-D [13:13] Those Germans... === mrevell is now known as mrevell-lunch === adeuring1 is now known as adeuring [14:11] henninge: Fancy taking on another branch? === allenap changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: - || queue: [jtv, allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [14:15] henninge: I've put myself in the queue, but feel free to eject me. [14:19] jtv, allenap: Sorry, lunch got in the way. I will relocate now and look at your branches after that. === mrevell-lunch is now known as mrevell === salgado is now known as salgado-lunch === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: jtv || queue: [allenap] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews === matsubara is now known as matsubara-lunch [15:51] jtv: r=me Thank you. === henninge changed the topic of #launchpad-reviews to: On call: henninge || Reviewing: allenap || queue: [jtv] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:03] henninge: Thanks! [16:03] allenap: I have not yet used ARRAY's [16:03] s/'// [16:06] henninge: Dang, I misread and thought that r=me was for me :) Thank you anyway! [16:06] allenap: oh, I am sorry. [16:06] thanks henninge [16:07] henninge: Hehe, don't be sorry, it was my fault. === matsubara-lunch is now known as matsubara === deryck is now known as deryck[lunch] [16:34] henninge: is [jtv] already reviewed? [16:34] abentley: yes I am === salgado-lunch is now known as salgado === abentley changed the topic of #launchpad-reviews to: On call: henninge, abentley || Reviewing: allenap, - || queue: [] || This channel is logged: http://irclogs.ubuntu.com/ || https://code.launchpad.net/launchpad/+activereviews [16:34] ups, chansubjupd error [16:35] ups=oops(de_DE) [16:35] henninge: We should have a web page we can use to set the channel topic :-) [17:04] abentley: Hi [17:04] abentley: Can I add a MP to your queue? [17:04] jelmer: sure. [17:05] abentley: https://code.launchpad.net/~jelmer/launchpad/672476/+merge/40349 === deryck[lunch] is now known as deryck [17:31] henninge: I have to go soon, so ask soon if you have any questions about my branch :) [17:32] allenap: sorry, I was about to write the review. I actually understand what's going on now ... ;) [17:32] allenap: I don't have questions, though. You can look at it in the morning. Nothing serious just some suggestions. [17:35] henninge: Awesome, thank you. === benji is now known as benji-lunch === leonardr is now known as leonardr-lunch === leonardr-lunch is now known as leonardr [18:20] jelmer: 59: s/iterabel/iterable [18:21] jelmer: Personally, I find using "with" with files more elegant than try/finally. [18:23] jelmer: Why do you test whether htpassword exists (and is a file) and delete it, instead of just overwriting it? [18:25] jelmer: is it secure to use token.person.name[:2] as a salt? [18:25] abentley: Thanks, fixed. [18:26] abentley: I usually don't bother with "with" out of fear of missing a "from __future__ import with_statment" somewhere [18:26] jelmer: we're all-2.6 now. You shouldn't import with_statement anymore. [18:27] abentley: Overwriting it would work too I guess. That, and the token.person.name[:2] are simply the same as they were in the generate_ppa_htaccess.py script. [18:28] jelmer: yeah, I realize this is mostly just reorganizing old code. [18:29] jelmer: But using token.person.name[:2] as a salt is security-by-obscurity, and now that we're open-source, it's no longer obscure. [18:32] abentley: it'd break a few existing tests though, so I wonder if I should leave that for a separate branch and file a high-priority bug about it for now. [18:33] abentley: What do you think? [18:33] salts can be known [18:34] james_w: Oh? I would have thought that would make it trivial to forge the password. [18:34] abentley, no. Assuming you are talking about a real salt, and not something wrongly called that, they are not keys [18:34] if they were then they wouldn't be separate [18:35] james_w: we're talking about salts as used by the crypt C function. [18:35] what they do is prevent exploiting all passwords at once. If there were no salts you could generate one rainbow table and then decrypt everyone's ciphertext at once [18:36] adding a salt means that you have to attack each ciphertext in turn [18:36] so they do no need to be secret, but they do benefit from being well-distributed [18:36] james_w: okay, thanks for that clarification. [18:37] * jelmer didn't know that either [18:37] (if num salts << num ciphertexts then you can decrypt a lot of ciphertexts with on rainbow table) [18:37] maybe a comment in the source? [18:37] james_w: and happy belated birthday. [18:37] thanks :-) [18:38] http://en.wikipedia.org/wiki/Salt_(cryptography) [18:39] it does say "For best security, the salt value is kept secret.", but if your system relies on the salt being secret, then the keys are probably too short anyway [18:41] james_w: it seems like in our context, all the passwords are equivalent, so only one cyphertext is needed. Therefore using distinct salts doesn't help? [18:42] abentley, where can I look to see the use of this in the source? [18:42] as I'm not sure what you mean by "all the passwords are equivalent" [18:42] james_w: https://code.launchpad.net/~jelmer/launchpad/672476/+merge/40349 [18:43] EACCESS [18:43] james_w: by "all the passwords are equivalent", I mean they all grant equivalent access AIUI [18:44] ah [18:44] so impersonation isn't really a problem [18:44] except for users who aren't supposed to have access at all [18:44] james_w: jelmer is refactoring this code: lib/lp/archivepublisher/scripts/generate_ppa_htaccess.py [18:46] abentley, yeah, I'd agree, in this case salts aren't particularly useful for their normal use like in unix passwd [18:47] it adds a little benefit in that you wouldn't have to revoke every access if one token was compromised, but you may want to do that anyway to be sure [18:48] in this case though the main requirement is that crypt requires a salt, and so I think that the value used is as good as any other [18:49] james_w: Right. Hard to imagine only one token being compromised in this scenario. [18:49] james_w: okay. === benji-lunch is now known as benji [19:13] jelmer: should _setupHtaccess consider the case where htaccess is different from what it would write? [19:13] abentley: No, it sufficient that there is *some* htaccess there. [19:14] abentley: generate_ppa_htaccess should take care of updating it [19:15] jelmer: ISTM that until generate_ppa_htaccess runs, the contents could be exposed. [19:16] jelmer: because the .htaccess might not impose any content restrictions. [19:16] abentley: We only write out one kind of htaccess file at the moment [19:17] abentley: I guess it might make sense take into account that we might want something different in that file in the future. [19:18] jelmer: so _setupHtaccess assumes that htaccess will not be different from what it would write. [19:21] abentley: Sorry, yes. It assumes the .htaccess file would be the same and that there is some .htpasswd [19:23] jelmer: If an earlier process had died causing a partial write, that would not be true, but I think that's rare enough not to care about. [19:26] jelmer: I guess I'm wondering whether skipping writing htaccess and htpasswd is worthwhile. [19:27] abentley: That's a good question, I've wondered about that as well. [19:27] jelmer: it seems like it doesn't save an appreciable amount of IO, and it does introduce a corner case. [19:27] abentley: It saves us a single database query and a certain amount of IO. [19:28] abentley: I don't know what the maximum number of subscriptions for a PPA is. [19:29] jelmer: At 243, I believe temp_filename is supposed to be on a new line. [19:31] jelmer: similarly at 351. [19:34] abentley: Fixed [19:35] jelmer: Your tests would be a bit simpler if you returned the contents of .htaccess and .htpasswd as strings or if your functions took a file parameter. [19:36] jelmer: You could also just use useTempDir in your tests. === lifeless_ is now known as lifeless [19:39] jelmer: You can use dedent to write your multi-line strings more neatly at 451. [19:43] abentley: I haven't used dedent before, what is it? [19:44] abentley: is this textwrap.dedent? [19:44] jelmer: Yes. It strips leading indents. [19:48] jelmer: Anyhow, those are just some ideas. r=me. [19:50] abentley: Thanks, that was quite educational. [19:51] jelmer: np. === matsubara is now known as matsubara-afk [20:12] abentley: I just realized that if we always write out the htaccess file we need to honor the blacklist that currently exists in generate-ppa-htaccess.py [20:13] jelmer: I see. It's your call. === salgado is now known as salgado-afk === Ursinha is now known as Ursinha-afk