[00:00] niemeyer: just a quick double check https://codereview.appspot.com/6601056 [00:18] niemeyer: do you want review this goamz patch ? https://code.launchpad.net/~mirtchovski/goamz/ec2 [00:24] davecheney: LGTM on first, thanks [00:24] niemeyer: ta [00:24] davecheney: Do you need something from the second? [00:25] what is the code review policy for goamz [00:25] the key change for that branch is adding DescribeImages [00:26] as the user (f2f in #go-nuts) has created their own ami's [00:26] whereas we use the ubuntu cloud image finding service thing [00:27] niemeyer: does turning on debug in ec2 dump my secret key ? [00:27] ie, is it safe to post this whole debug log ? [00:28] davecheney: I don't know.. [00:28] davecheney: There's a lot pending in goamz.. someone needs to dedicate some time to it [00:29] davecheney: We also require people to sign the contribution agreement on external contributions to Canonical code bases [00:29] davecheney: Has he signed? [00:29] niemeyer: i doubt he's signed anything [00:29] niemeyer: https://bugs.launchpad.net/juju-core/+bug/1061941 [00:30] ^ debug in issue [00:32] davecheney: Okay, can you figure why it is broken? [00:33] i'm sure it's the length of the URL, or some component of that [00:33] bisecting the problem now [00:35] davecheney: That debug info doesn't say much.. the meat is usually inside sign.go [00:35] davecheney: We had it broken sometime ago due to a change in the way path was handled [00:36] davecheney: Ah, wait [00:36] davecheney: It could be about the encoding of the + there [00:37] niemeyer: right o [00:37] davecheney: Not how the signature has a %2B [00:37] davecheney: Note [00:37] yup, that will proably screw itup [00:37] damn spaces [00:38] niemeyer: there is a certain number of instances where it does work for [00:38] i'm figureing out that number, it's like 3 or less I think [00:39] davecheney: This may match the chances of getting a signature with a given shape === meetingology` is now known as meetingology [03:58] * niemeyer => bed [03:58] Night all [06:08] fwereade_: mornin' [06:09] davecheney: hiya [06:21] morning [06:29] morning gents [06:33] rogpeppe, heyhey [06:33] davecheney, heyhey [06:33] TheMue, heyhey [06:33] hi all [06:55] rogpeppe, I've been chatting to niemeyer about jujuc.HookContext [06:55] fwereade_: ok [06:55] rogpeppe, we are agreed that it should be an interface, and nothing more specific has been said [06:56] fwereade_: interesting. [06:56] rogpeppe, I have had a crazy idea, that the interface we *really* want may actually be a jujuc.Conn, with the same thinking behind it as juju.Conn [06:56] fwereade_: i think of it as quite concrete, but i'm interested where you end up [06:57] fwereade_: juju.Conn isn't an interface :-) [06:57] fwereade_: what different implementations of this interface do you envisage? [06:59] rogpeppe, the purpose is to separate the server and the commands cleanly from all the crap in RelationContext that is in the wrong place [06:59] rogpeppe, by making an interface and moving the existing implementation off into uniter we can start to collapse Relationer and RelationContext into something interesting [07:00] rogpeppe, the fact that it will also be possible to write neater contexts in the jujuc tests is both a bonus and a hassle [07:01] fwereade_: i'm not sure i see the relationship with juju.Conn. doesn't the context need to know the context within which the commands work (i.e. the current unit, relation, etc) ? [07:02] rogpeppe, well, yes, but I think we can expose all that clearly and usefully in terms of capabilities rather than things [07:02] rogpeppe, no need for a *state.Unit in the Conn(?) has an OpenPort method, etc etc [07:03] s/ in / if / [07:03] rogpeppe, and then I thought "hmm, juju.Conn presents a convenient high-level interface with state" [07:04] fwereade_: what about commands that operate relative to the context? [07:04] rogpeppe, "this situation has at least one interesting parallel" [07:04] fwereade_: e.g. unit-get, relation-get etc [07:04] rogpeppe, they basically all do... [07:04] fwereade_: yeah, so it won't look much like juju.Conn then, i guess [07:04] fwereade_: but you could certainly provide primitives that look like the ones the commands require [07:05] rogpeppe, yes, that is the commonality I am thinking of [07:05] fwereade_: but then, does that really save you much? [07:05] fwereade_: over just writing the commands, using the stuff inside the context [07:06] rogpeppe, well, it's basically a straight move of code inside the context [07:07] fwereade_: i guess i don't really see where the pressure here is. could you explain what the problem is with the current structure? [07:08] rogpeppe, primarily that relationy responsibilities are unhelpfully smeared across two modules, and I will be able to express things much more clearly [07:08] rogpeppe, by combining RelationContext and Relationer with some of HookContext [07:10] * rogpeppe is thinking [07:10] rogpeppe, a server module that contains nothing but the server and the commands, and implements them in terms of an interface, will allow me the freedom to butcher the existing types and construct a magnificent assemblage from their parts :) [07:13] fwereade_: perhaps you could put together a sketch of how you think the types might work? [07:15] fwereade_: i guess i don't really see how an interface will work. who will implement the interface? [07:15] rogpeppe, something in uniter whose true nature will only become apparent as i integrate relations [07:16] rogpeppe, for now, the existing HookContext and RelationContext will move into uniter and be manipulated by Relationer as before [07:16] rogpeppe, they'll just be tweaked to implement the interface [07:16] fwereade_: the difficulty i see is that you've got two kinds of context going on, and one is a superset of the other AFAICS [07:16] rogpeppe, expand please [07:17] fwereade_: well, aren't the commands that you can execute in a relation hook more-or-less a superset of the commands you can execute in a non-relation hook? [07:17] rogpeppe, no, they're exactly the same [07:17] rogpeppe, you just might need to specify things that aren't present in a non-relation context [07:19] fwereade_: ok, so... i still don't see the interface thing. it's only useful to have an interface if you've got multiple implementations of it, in my view. [07:19] fwereade_: and i still don't see how that applies here [07:20] fwereade_: couldn't you just move HookContext into relationer? (with possible renaming/restructuring along the way) [07:21] rogpeppe, no: import cycles [07:21] rogpeppe, that *is* the right place for it [07:21] fwereade_: what's the import cycle there? [07:21] rogpeppe, the uniter needs to import jujuc to do stuff with the context; jujuc needs to import uniter to know wtf it's been given [07:22] rogpeppe, it could have its own package [07:22] rogpeppe, and that package will still exhibit the original problem [07:22] fwereade_: if the context was in relationer, the uniter wouldn't need to import jujuc, no? [07:22] rogpeppe, there is no relationer package [07:22] rogpeppe, and I don;t think there should be [07:22] fwereade_: i think we're getting closer to the crux of the issue here. [07:23] rogpeppe, Relationer itself will almost certainly change beyond recognition [07:24] rogpeppe, it is at heart a part of the uniter, and if it deserves to be a helper type then its shape will be very different [07:25] fwereade_: so could you put the context in uniter? [07:25] fwereade_: without having an import cycle? [07:25] rogpeppe, yes, that is what I'm proposing; that is the situation with the import cycle [07:26] rogpeppe, an interface STM to be a great way of doing so with minimal disruption [07:26] fwereade_: why does the uniter need to import jujuc? [07:27] rogpeppe, so it can run a jujuc.Server [07:27] rogpeppe, and supply it with contexts [07:28] fwereade_: ah yes, of course, that seems right [07:29] * rogpeppe is still thinking [07:30] rogpeppe, it is a situation that deserves lots of it, I have been thinking about this for some time :) [07:33] rogpeppe, hmm, it crosses my mind that OpenPort/ClosePort are somewhat crackful, because they change global state directly [07:34] fwereade_: rather than wating until the hook has completed successfully? [07:34] rogpeppe, my fuzzy feeling that the Context interface should not directly expse state at all has just grown firmer [07:34] rogpeppe, yeah [07:34] rogpeppe, also, config-get isn't using a frozen config [07:35] fwereade_: it probably should, i guess [07:35] rogpeppe, (also we expose stuff like ConfigNodes which have *totally* inappropriate capabilities like Read and Write [07:35] ) [07:41] fwereade_: here's a perhaps crackful idea: could we obliterate *all* knowledge of units, charms etc from jujuc/server ? [07:41] rogpeppe, not crackful at all [07:41] fwereade_: and have it simply provide a command-running and callback service [07:41] rogpeppe, I've been skating around the issue due to the distracting potential crackfulness [07:41] rogpeppe, hmm [07:42] rogpeppe, I'd been thinking more in terms of it not referencing state directly in the interface at all [07:42] fwereade_: it seems to me that this is where the cyclic stuff is coming from, and that actually the *important* thing that jujuc/server is to run commands and allow commands to call back. [07:42] rogpeppe, niemeyer and I are in rough agreement that server+commands is a cohesive chunk [07:42] s:jujuc/server:jujuc/server does: [07:43] rogpeppe, I think he feels that way more strongly than me though [07:43] fwereade_: i think i'm more of the opinion that the commands are tied more closely to the uniter than the server mechanism [07:44] fwereade_: and i *think* that might help things straighten out [07:45] fwereade_: in fact it looks to me as if even now there's no essential reason for HookContext to live in jujuc/server [07:45] rogpeppe, I was convinced otherwise :) [07:46] fwereade_: NewServer doesn't rely on uniter [07:46] AFAICS [07:46] rogpeppe, I have been through this [07:46] rogpeppe, I think we are in agreement that "pull server out on its own, it'sthe most obviously independent bit" would be a reasonable thing to do [07:46] fwereade_: yeah [07:47] rogpeppe, however, I think that all that actually does is push a non-problematic part to one side [07:47] fwereade_: it seems to me that server+commands is *not* a cohesive unit - it's two things stuck together not-very-strongly-at-all. [07:47] rogpeppe, well, they are united in their need for a HookContext [07:48] fwereade_: the server doesn't need a HookContext necessarily. [07:48] rogpeppe, at least, it needs to "know" about them, even if it doesn't manipulate the type directly [07:49] fwereade_: what does it "know" about them? [07:49] rogpeppe, ContextId is one of the Request fields [07:50] rogpeppe, really [07:50] rogpeppe, the location of Server is a derail [07:50] rogpeppe, it does not contribute to solving the problem [07:50] fwereade_: i'm not so sure [07:51] rogpeppe, it is trivial to extract it now and it will remain trivial to do so tomorrow [07:51] fwereade_: the main cyclic problem was between jujuc/server and worker/uniter, no? [07:51] rogpeppe, the cyclic problem is HookContext, full stop [07:52] rogpeppe, and that is a symptom of the relation ickiness more than anything else [07:52] fwereade_: suppose you had a worker/uniter/commands package and defined HookContext in there? [07:53] fwereade_: might that help? [07:53] rogpeppe, then I *still* have the precise problem I have been trying to express all morning [07:53] fwereade_: sorry, please reiterate. it'll penetrate my think skull eventually :-) [07:53] rogpeppe, Relationer and RelationContext need to be in the same package so I can evolve them towards cohesive sanity without making a mess in other packages [07:54] rogpeppe, once I have them next to one another I think it will be almost trivial [07:54] rogpeppe, while they're distant any CL will be laughed out of the room for being monstrous, and quite rightly so :) [07:55] fwereade_: so... the problem is essentially that the commands need RelationContext and you don't want to put RelationContext in the same package as the commands. [07:55] fwereade_: (or vice versa) [07:56] fwereade_: because ISTM that an ultra-simple solution would just be to put the commands in uniter [08:00] fwereade_: it wouldn't make uniter too huge actually. the two packages combined (including the separable server stuff) amount to only ~2100 lines. [08:01] fwereade_: and perhaps this is the fundamental underlying semantic issue - the commands *are* tied closely to the uniter. [08:04] rogpeppe, IMO not so tightly as to justify dumping them all in the same package [08:05] rogpeppe, being able to express the interface without mentioning state has a benefit as well -- it keeps the jujuc package nice and hygienic, and unable to itself cause the write-on-error bugs we discussed above [08:05] fwereade_: it seems to me that they're really just functions that are called back into the uniter; they just happen to be expressed as commands. [08:07] rogpeppe, well, yes; but *everything* under uniter is similarly closely tied to uniter, and I really don't think dumping it all in one package is a good idea [08:08] fwereade_: hmm, i may be coming around to your point of view :- [08:08] ) [08:08] * fwereade_ feels cheered :) [08:10] fwereade_: you'll need to be careful how you define the interface though; it might not be easy to avoid cycles or awkward copying type conversions. [08:11] rogpeppe, I *think* have something moderately sane coming through [08:12] fwereade_: FWIW i think that worker/uniter/commands is a better name than jujuc/server [08:12] or worker/uniter/cmd [08:12] rogpeppe, the thing is you are right about the callbacky nature of the commands, and I think this will actually make that clearer [08:12] rogpeppe, as soon as we drop jujuc I will be agitating for worker/uniter/tools [08:12] fwereade_: tools == commands ? [08:13] rogpeppe, yeah, specifically borrowing terminology from elsewhere in the project [08:13] rogpeppe, anyway, while cmd/jujuc exists, worker/uniter/jujuc it will be [08:15] fwereade_: are you planning to move cmd/jujuc to worker/uniter/jujuc ? [08:15] rogpeppe, cmd/jujuc is the command, just about all the implementation is now in worker/uniter/jujuc; moved from cmd/jujuc/server [08:17] fwereade_: hmm. i'm not sure that "jujuc" is such a good name in that context. worker/uniter/server would be better. or worker/uniter/commands (or tools, though i'm not sure about borrowing that terminology) [08:17] anyway, that's just a naming issue [08:18] rogpeppe, for now it is the implememntation side of cmd/jujuc, and I think that remains the best name [08:19] rogpeppe, when we fold jujuc into jujud that point will become open [08:20] fwereade_: ok [08:26] fwereade_: one last murmur: i think you're going to a lot of effort to avoid importing 500 lines of code into the uniter package. a concrete type presented to the commands could potentially be just as clear (as in not making State available) as an interface, i think. [08:28] rogpeppe, it still feels like necessary scaffolding to allow me to make the change semi-peacefully [08:30] fwereade_: moving to using an interface feels like it'll be more disruptive, but i'm sure i don't understand the issues well enough. [08:47] fwereade_, rogpeppe: could you please take a short look at https://codereview.appspot.com/6589073/ ? i've got a strange error there when clicking on "View". [08:48] TheMue, I too see a chunk mismatch [08:48] TheMue: i've seen that before. you can look at the raw diff instead: https://codereview.appspot.com/6589073/patch/6001/4003 [08:48] TheMue, I don't know how to resolve it I'm afraid [08:48] TheMue: i'm not sure what causes it to happen, i'm afraid. [08:48] es, the raw is fine, i've tested. [08:48] s/es/yes/ [08:49] ok, thanks for your effort [10:23] fwereade_: i consistently get a uniter test failure in trunk: http://paste.ubuntu.com/1261625/ [10:23] rogpeppe, whoa [10:24] fwereade_: and i think jujuc was failing too, let me find the failure in the copious log output... [10:25] fwereade_: yeah, uniter/jujuc too: http://paste.ubuntu.com/1261631/ [10:26] rogpeppe, that one's because you haven't updated goyaml I think [10:26] fwereade_: ah, i didn't realise i needed to [10:27] fwereade_: yeah, that's the reason for that, thanks [10:28] fwereade_: that doesn't fix the other problem tho [10:29] rogpeppe, I'm investigating [10:29] fwereade_: does it pass for you? [10:30] rogpeppe, just got to running it, had to clean up a bit of mental state so it makes sense when I look back at it [10:30] rogpeppe, yeah, works for me, which is odd [10:31] rogpeppe, would you try logging the actual underlying error in unit.go:578 please? [10:33] fwereade_: will do [10:33] fwereade_: you mean uniter.go presumably? [10:34] rogpeppe, no, state/unit.go, surely? [10:34] fwereade_: oh, sorry, i wasn't looking at the log... [10:34] rogpeppe, np -- I think the copious logging is more of a blessing than a curse but I agree it can be a lot [10:35] fwereade_: it seems wrong that we're ignoring the error there anyway [10:36] * fwereade_ agrees [10:39] fwereade_: [LOG] 10.99256 JUJU set private address error: duplicate key insert for unique index of capped collection [10:39] fwereade_: ha. [10:41] fwereade_: maybe i need to update my mgo package [10:43] fwereade_: that fixed it. [10:43] fwereade_: phew. [10:47] fwereade_: if you have a spare moment: https://codereview.appspot.com/6623047 [10:47] rogpeppe: good hint, just upgraded it too [11:05] fwereade_, TheMue, davecheney: and another one: https://codereview.appspot.com/6612054/ [11:10] *click* [11:11] rogpeppe: In the last one you have two helpers in agent_test.go exported. is there a special reason? [11:11] TheMue: i just moved them from another file [11:12] TheMue: they're exported to indicate they're intended to be used globally, not that it makes any difference in a test file. [11:12] rogpeppe: yes, i've seen, but they could be private, don't they? [11:12] rogpeppe: ah, ic [11:12] TheMue: yes, and so could all our suite types, but they're not. i don't care too much. [11:14] rogpeppe: ;) [11:14] rogpeppe: btw, what's "arble"? [11:14] TheMue: a word :-) [11:14] TheMue: a nonsense word that was made up by a friend at uni AFAIR. [11:15] rogpeppe: hehe, a foobar alternative [11:15] TheMue: yeah [11:16] rogpeppe: you've got a lgtm [11:16] TheMue: thanks [11:16] rogpeppe: yw [11:16] TheMue: i win? woo! :-) [11:17] TheMue: what's my prize? [11:17] rogpeppe: yes, and the prize is to make the alpha bundle [11:17] TheMue: booby! [11:18] rogpeppe: hehe [11:19] * TheMue fights with headaches today, and that w/o alcohol. maybe that's the reason. [11:20] TheMue: i prescribe you a large 16 y.o. Talisker [11:20] rogpeppe: aaaaah, you know my medicine, good [11:22] rogpeppe: just asked my dealer for a 17 y.o. Balvenie DoubleWood, costs about 75 £ [11:22] rogpeppe: i hope it's not too expensive here, taxes are lower [12:04] fairly trivial CL: https://codereview.appspot.com/6622047/ [12:06] rogpeppe: is setting the pw to "" really removing it or setting it to ""? [12:07] TheMue: for AdminPassword, it's really removing it, as mentioned in the docs [12:07] lunch [12:07] ok thx [12:15] mramm: i thought the meeting was at 1.30 [12:15] rogpeppe: it is [12:15] I started the hangout too soon [12:15] mramm: ah, np [12:27] does anyone have the hangout link ? [12:28] found it [12:28] davecheney: yo! [12:28] Hi all [12:29] niemeyer, heyhey [12:30] fwereade_: Yo [12:30] fwereade_: hangout? [12:30] fwereade_: https://plus.google.com/hangouts/_/ad44942cb79ac76c808c48efaec6b9da87275d6c?authuser=0&hl=en [12:35] !meeting [12:36] didnt' expect that to work [12:52] "hello dear, My Name is Dorise i see your email at golang.org i will like us to have a good friendship" [12:52] WTF? [12:53] niemeyer: hands off, she's promised to me [12:53] CONTRIBUTORS for the win [12:53] :-) [12:53] no [12:53] for the spam [12:53] davecheney: Ah, makes sense [12:53] *ROFL* [12:59] davecheney: sorry mate, i got there first [13:00] rogpeppe: D comes before R [13:00] she emailed me first [13:02] :) [13:21] niemeyer: ping [13:21] rogpeppe: Hi [13:21] niemeyer: i'm just wondering about the best place to do the admin secret hash logic [13:22] niemeyer: here are a couple of possibilities: http://paste.ubuntu.com/1261889/ [13:24] rogpeppe: Option 2 seems nicer overall [13:25] niemeyer: ok, cool. i was trending towards that, but thought there were args both ways. [13:25] niemeyer: it's kinda odd perhaps that each environment will need to implement the same hashing logic, but that's perhaps ok. [13:26] niemeyer: actually we could implement PasswordHash function in environs [13:26] niemeyer: that might work nicely actually; then neither juju nor ec2 need be dependent on the details of the hashing. [13:27] rogpeppe: That's what I imagined too [13:27] niemeyer: cool [13:27] niemeyer: will do [13:27] rogpeppe: Cheers! [13:27] niemeyer: i'm piling up a few small CLs BTW if you have a moment some time. [13:28] niemeyer: https://codereview.appspot.com/6623047, https://codereview.appspot.com/6612054/, https://codereview.appspot.com/6622047/ [13:28] rogpeppe: I'll do reviews in a moment [13:28] niemeyer: np === TheMue_ is now known as TheMue [13:40] fwereade_: Do you want to have that conversation now? [13:40] niemeyer, http://paste.ubuntu.com/1261919/ [13:41] niemeyer, is a summary of my current thinking which hopefully draws on our shared context [13:41] niemeyer, can I just go for a ciggie while you read before we get settled in? :) [13:41] fwereade_: Certainly :) [13:41] niemeyer, cheers, bbs [13:42] I'll prepare some chimarrão at the same time [13:48] niemeyer_, back [13:49] fwereade_: I'm here too, let's fire it off [13:50] niemeyer_, starting a hangout then [13:50] fwereade_: Just did it [13:50] niemeyer_, joining === niemeyer_ is now known as niemeyer [14:09] TheMue: https://codereview.appspot.com/6589073/diff/6001/environs/jujutest/livetests.go [14:10] niemeyer: yes, i already asked roger and william how this is possible. the raw diff is ok. [14:10] TheMue: Yep.. have you tried to propose it again? [14:10] niemeyer: do you have any idea how to "repair" it? [14:10] niemeyer: not yet, willdo. one moment. [14:11] niemeyer: when you've finished with fwereade_, i've encountered another interesting wrinkle which needs a moment's thought. [14:11] rogpeppe: Just reviewing a branch and will be with you [14:12] niemeyer: strange, now lbox hangs here. *grmblx* [14:12] TheMue: Hangs? Or is it working? [14:12] niemeyer: oh, had a 502 [14:12] TheMue: That's a Launchpad internal error [14:13] niemeyer: i'm trying it again right now [14:14] niemeyer: ah, it's there now [14:17] niemeyer, hey, just one thing: naming of the HookContext interface? [14:18] niemeyer, I would actually be pretty happy with a jujuc.Context interface [14:18] fwereade_: +1 [14:18] rogpeppe, cheers [14:18] fwereade_: +1 [14:18] niemeyer, cheers :) [14:23] TheMue: Sent a couple of comments [14:23] TheMue: and LGTM assuming that tests pass on Amazon [14:23] TheMue: Live tests, that is [14:23] niemeyer: just seen the notification, thx [14:23] TheMue: I'd like to cover something about the next step [14:23] TheMue: Do you have a minute? [14:24] niemeyer: yes [14:24] TheMue: Okay, so [14:24] TheMue: I believe that one of the reasons why the firewaller works well today is because we have a contract that when a machine comes up, it's coming up with all its ports closed [14:25] TheMue: In other words, StartInstance always hands off machines with no ports open [14:25] TheMue: This is of course being broken by the global scheme [14:25] TheMue: Which is alright on itself, except for one detail [14:25] TheMue: When a machine dies, what happens with its ports? [14:27] TheMue: Are you still there? [14:27] niemeyer: Yes, thinking about it. [14:28] TheMue: The answer is nothing. [14:29] TheMue: And that's been fine so far [14:29] TheMue: Because the group is only used by that one machine [14:29] niemeyer: We react in the sense of the lifecycle [14:30] TheMue: Meaning? [14:30] niemeyer: if i get it right we close the ports of the unit. [14:31] niemeyer: but with a global group that's not good [14:31] niemeyer: it only should be closed if no unit needs that pot anymore [14:32] TheMue: When do we close the ports of the unit? [14:34] niemeyer: on moment, checking if i have seen it right. thought it is in flushMashine() [14:34] TheMue: Which machine? It was removed [14:35] TheMue: We may never have seen it [14:35] TheMue: I mean, the removal [14:35] How can we tell which ports on that global group are improperly opened [14:36] niemeyer: That's what i meant with a lifecycle change. This would be catched. But not a real hard death. [14:36] TheMue: We cannot trust on luck [14:37] niemeyer:yes, i know. it only has been a view on the status quo [14:37] TheMue: Anyway, I'll leave you thinking about that problem.. we need a way to close those ports [14:37] rogpeppe: SOrry, I have a call with flacoste right now.. I'll be with you after that [14:37] niemeyer: np [14:38] niemeyer: ok, so i know my next task ;) [14:42] niemeyer, when you're off your call, https://codereview.appspot.com/6620054 isn't quite what we originally discussed but should still be pretty trivial [15:15] fwereade_: Sounds good, I'll just be with rogpeppe for a while and will then jump on that [15:15] niemeyer: there's a problem with using a salted password [15:15] niemeyer, cool, thanks [15:15] niemeyer: which is that we don't know the salt when we reconnect [15:15] rogpeppe: Where is the salt put [15:16] niemeyer: we need to store the salt in the environment [15:16] niemeyer: we can return it in the state.Info [15:16] niemeyer: alternatively [15:16] niemeyer: we could not use a salted password at all and just say "use a long random password" [15:17] rogpeppe: Just use a well known salt for now [15:17] niemeyer: what's the point? [15:17] niemeyer: well, i guess we can add salt later :-) [15:18] niemeyer: the changes are fairly small actually [15:18] rogpeppe: The point is the same of having a salt [15:18] rogpeppe: You can't attack a juju environment with a pre-made dictionary unless that dictionary was built specifically to attack a juju environment [15:18] niemeyer: there's no point in having a constant salt - it's equivalent to having no salt at all [15:19] niemeyer: i guess so [15:19] rogpeppe: No, it's not equivalent [15:19] niemeyer: yeah, i see your point [15:19] rogpeppe: If you use a salt, you force people to start building that dictionary today [15:19] niemeyer: ok, constant salt it is [15:20] rogpeppe: If you use an algorithm that takes 1 second to compute the hash on a modern computer, even a short password will take many years to break [15:20] niemeyer: the password hash function i've just used takes about 0.2 seconds per password FWIW. [15:20] rogpeppe: Increase the count [15:21] niemeyer: ok, will do. it'll increase the test time though :-) [15:21] rogpeppe: Not if you make that a parameter [15:21] niemeyer: i'm not sure it should be a parameter. it could be a global variable changed by the test code though. [15:22] niemeyer: part of the point of putting it in a function is to hide the parameters, i think. [15:22] rogpeppe: Or just keep it 0.2 [15:22] niemeyer: i think 0.2 should be fine tbh [15:23] rogpeppe: I think so as well.. we're overvaluing that problem.. we have a lot to do before that kind of thing becomes a real issue [15:23] niemeyer: and we can add a random salt quite easily at a later stage, and people can change their passwords. [15:23] niemeyer: yup [15:57] rogpeppe: All of yours reviewed [15:57] niemeyer: brilliant, thanks [15:57] fwereade_: I'll do yours first thing after lunch [15:57] niemeyer: i've just made the change you suggested to https://codereview.appspot.com/6623047/ [15:57] * niemeyer => food! [15:57] rogpeppe: Cheers [15:57]