[06:31] davecheney, are you seeing http://paste.ubuntu.com/1266985/ ? [06:32] nope, but i haven't updated recently [06:34] davecheney, fwereade_: mornin' [06:36] rogpeppe, heyhey [06:38] ... obtained map[string]interface {} = map[string]interface {}{"title":"My Title", "username":"admin001", "skill-level":9000} [06:38] ... expected map[string]interface {} = map[string]interface {}{"title":"My Title", "username":"admin001", "skill-level":9000} [06:38] o_0! [06:39] davecheney, huh [07:10] morning [07:10] morning [07:16] TheMue, heyhey [07:16] davecheney and fwereade_: hi [07:26] morning. [07:27] Aram, heyhey [07:35] hi amithkk [07:35] oops [07:35] hi Aram [09:41] fwereade_: i was thinking about your Context interface, and wondering if the MapChanger type was necessary. i was thinking something like this might work a tad more neatly: http://paste.ubuntu.com/1267165/ [09:46] fwereade_: probably this is just bikeshedding tho' [09:47] wrtp, I think I'd prefer to keep being able to just return a ConfigNode there [10:08] wrtp, Context.RelationId(); Context.RemoteUnitName() [10:09] wrtp, niemeyer has suggested adding error returns [10:09] wrtp, ISTM that, in practice, what this does is bloat the crap out of all the code that uses it [10:10] fwereade_: i was trying to see the interface from the context of a user of it. ISTM that it would be nice if it mapped fairly closely to the interface provided to the hooks. but i see yr point about just returning ConfigNode [10:10] wrtp, can you offer a perspective from which this change is a good thing? [10:11] fwereade_: as a consumer of the interface, it's not really clear why the MapChanger type is there. [10:11] wrtp, feels to me like it just means more code, with error paths that can't happen [10:12] fwereade_: oh, sorry, you mean adding error returns? [10:12] wrtp, yeah [10:12] fwereade_: error returns from what? [10:12] wrtp, Context.RelationId(); Context.RemoteUnitName() [10:13] fwereade_: so niemeyer would like you to return an error rather than relation-id == -1? [10:13] wrtp, yeah [10:14] fwereade_: i'm with you tbh. there's only one kind of error that can happen, and it's not really an error. [10:14] wrtp, and I was ok with this, it seemed reasonable, until it came to actually switching the commands over to use the interface [10:14] fwereade_: returning an error makes it look like all kinds of bad things could happen [10:14] wrtp, yeah exactly [10:15] fwereade_: there is another possibility, i suppose [10:15] wrtp, trying to *use* that interface has put me in a totally foul mood [10:15] fwereade_: which is to have an IsRelationContext (or something) method, and make Context.RelationId panic if called when IsRelationContext is false. === TheMue_ is now known as TheMue [10:16] fwereade_: that way you make the precondition clear, and there's no way you can inadvertently get it wrong. [10:16] wrtp, tbh I would like to take it further, and use HasRelation to guard Relation [10:17] wrtp, hell, HasRemoteUnit for RemoteUnitName [10:17] wrtp, that might lead to code that doesn't make me want to slit my eyes [10:17] wrtp, might actually even be more readable [10:18] fwereade_: that sounds reasonable to me [10:19] fwereade_: i don't know that it'll make niemeyer happy though. you can still run RelationId and forget it might be invalid. you'll just get a panic if you do that. [10:19] * fwereade_ goes off for a ciggie and a grumble to himself while he tries to figure out whether it's even worth bothering to change anything until niemeyer comes online [10:22] actually, going to get some food, bbiab [10:24] fwereade_: one other possibility: http://paste.ubuntu.com/1267207/ [12:00] morning niemeyer [12:01] niemeyer, I'm finding the error returns from RelationId and RemoteUnitName to be extremely unwieldy... I think the biggest problem is that the errors imply that more could go wrong than just ErrNoRelation etc [12:03] niemeyer, I'm wondering how you'd feel about pairs like HasRemoteUnit() bool and RemoteUnitName() string, and a requirement that clients look before they leap? [12:14] fwereade_: Good morning [12:14] Morning all [12:14] niemeyer, heyhey [12:14] fwereade_: It wouldn't really help much in the sense I was considering [12:15] fwereade_: The point was really to prevent using context.RelationId() as if the relation existed, when in fact it doesn't [12:15] fwereade_: We have the same logic in state itself, for example [12:15] fwereade_: E.g. unit.InstanceId() [12:17] niemeyer, I just don't see how it's anything but obfuscatory to have to go through identical error checking on RelationId and Relation [12:18] fwereade_: Do you have the paste at hand? [12:18] fwereade_: Actually, it was already a review wasn't it [12:18] * niemeyer looks [12:18] niemeyer, yeah [12:24] fwereade_: Alternatively, what about having RemoteRelation (analogous to RemoteUnitName) that returns (ContextRelation, error), possibly returning ErrNoRemote, and moving Id to within ContextRelation [12:27] niemeyer, not sure about the name; where does Remote come in? and actually I don't think we even need the ID once we've got the relation... [12:27] fwereade_: The name doesn't sound great indeed [12:28] niemeyer, quick access to the current relation would be a good thing though [12:29] niemeyer, but still, RemoteUnitName... in that case I really don't think we want any sorts of errors: I really don't see that checking for 2 different possible error kinds (one of which can't happen, but the interface says it might) is any better than `if ctx.RemoteUnitName != ""` [12:30] niemeyer, sorry, `if ctx.RemoteUnitName() != ""` [12:30] niemeyer, does the switch from struct to interface really mandate that we codify every zero value as a specific error? [12:30] fwereade_: Which error is it saying it can't happen but the interface says it might? [12:31] niemeyer, any error other than ErrNoRemote [12:31] fwereade_: Sorry, I think I'm still a bit slow today [12:31] niemeyer, if I want to get a remote unit name, I now have to worry about 3 code paths [12:31] fwereade_: Even if there's a single error, that's still a reason to report it.. (?) [12:31] niemeyer, doesn't exist; does exist; something else went wrong [12:32] niemeyer, the something else is not possible [12:32] fwereade_: What something else? [12:32] fwereade_: Sorry [12:32] niemeyer, anything [12:32] fwereade_: Erm.. great [12:32] fwereade_: Foo [12:32] niemeyer, unless we're saying that RemoteUnitName can *only* return ErrNoRemote? [12:33] fwereade_: I'm completely out of context [12:33] fwereade_: RemoteUnitName may be called when there's no remote unit name.. there are two ways to go about this: 1) We panic; 2) We error [12:33] fwereade_: There's no in-between [12:36] fwereade_: If you want to panic, and use IsRelationHook or similar, that sounds fine too [12:36] niemeyer, wooo! [12:36] niemeyer, cool, i thought I'd suggested that clearly at the beginning :0 [12:37] fwereade_: I don't recall a panic suggestion, but perhaps I just missed it [12:37] niemeyer, "require that clients look before they leap" [12:37] fwereade_: Cool, sorry, I didn't think that was it [12:37] fwereade_: +1 either way [12:37] niemeyer, that was I admit a sl obscure way to put it :) [12:38] niemeyer, ok, I'll try those [12:38] niemeyer, thanks [12:40] fwereade_: np, and sorry for being slow to understand [12:40] niemeyer, no worries :) am just pondering names... ActiveRelation? CurrentRelation? [12:41] fwereade_: HookRelation? [12:42] niemeyer, +1, thanks [13:21] fwereade_: I find the interface slightly non-conventional, in that rather than doing "if err != nil", we'll be doing the same thing with HasFoo in several places [13:21] fwereade_: We can go ahead and see, though, if you're happy with this [13:22] niemeyer, I'm not sure I have the best solution, but the error-juggling was getting painful... it will probably evolve a touch if we run with it for a bit and listen to what it's saying :) [13:23] fwereade_: Sure.. error handling is painful, but part of the deal [13:23] fwereade_: In some cases, it looks like it went too far [13:23] fwereade_: E.g. Relation(id int) needs an error return, I think [13:24] niemeyer, well, I'm not usually bothered by error handling :) [13:24] fwereade_: Otherwise you'll simply be doing if HasRelation(id); Relation(id) all the time [13:24] huh, maybe i wasn't attached [13:24] fwereade_: did you see my alternative suggestion above? [13:25] rogpeppe, I did... wasn't quite sure what you were suggesting, because it seemed to drop a lot of important bits from the interface [13:25] fwereade_: oh, i hadn't intended to drop anything. [13:25] fwereade_: i omitted ContextRelation because it was the same as before [13:26] niemeyer, well, not really... I'll be making use of *knowing* that a given relation exists, I think [13:26] fwereade_: How do you know it? [13:26] fwereade_: The only way is calling HasRelation, I believe [13:27] niemeyer, well, at the moment I know it because in all the commands that need relations Init will fail if the relation isn't valid [13:28] niemeyer, I'm not sure it's really worth checking that the relation is valid *every* time we're about to use it [13:28] niemeyer, against the same, frozen, context... [13:28] fwereade_: That's exactly the danger [13:28] fwereade_: You don't want to check that it is valid, but you have to anyway, or you have to memorize the code paths [13:28] fwereade_: so that it doesn't blow up [13:29] fwereade_: Writing down err != nil is cheap.. memorizing code paths is not [13:29] fwereade_: Either way, LGTM.. I'm happy to see the code and bother you later :) [13:29] niemeyer, we'll see how much it makes your eyes bleed when I propose then :) [13:30] fwereade_: suggestion in full: http://paste.ubuntu.com/1267395/ [13:30] fwereade_: My eyes won't bleed.. I'll simply laugh the first time we get a panic [13:30] niemeyer, heh :) [13:30] rogpeppe, yeah: how do I get an arbitrary relation out of that? [13:31] fwereade_: ctxt.RelationContext().Relation(id) [13:31] fwereade_: one final alternative: http://paste.ubuntu.com/1267396/ [13:31] fwereade_: then it would be: ctxt.(RelationContext).Relation(id) [13:31] rogpeppe, and in a non-relation hook? [13:31] rogpeppe: Are you really suggesting we have both ContextRelation and RelationContext? [13:31] fwereade_: ah, yeah [13:32] niemeyer: i thought it seemed reasonable, in the erm... context. [13:32] niemeyer, well, we already do, but I hope it will become nicer :/ [13:32] fwereade_: We do!? [13:32] niemeyer: they mean quite different things, so i think it's not unreasonable to have those names [13:32] niemeyer, we have a concrete RelationContext type and your suggested ContextRelation [13:33] rogpeppe: I think it's completely crackful [13:33] niemeyer: ContextRelation represents a particular relation; RelationContext represents a particular context. [13:33] niemeyer: but obviously mileage varies... [13:33] rogpeppe: This is insane, soryr [13:33] niemeyer, that's why I liked the idea of a jujuc.Relation interface -- IMO that is perfectly distinct from state.Relation [13:34] fwereade_: i like that too. [13:34] fwereade_: Why do we have both? [13:34] niemeyer, bear in mind that RelationContext will almost certyainly not keep that name, though [13:34] fwereade_: Ahhh, okay [13:34] niemeyer, well, we have a concrete type we're trying to hide behind an interface [13:34] fwereade_: Yes, and that concrete type doesn't live in the same package [13:34] niemeyer, not yet [13:34] fwereade_: Or won't (thinking end goal) [13:35] fwereade_: We can call it Mama for now [13:35] niemeyer, haha [13:35] fwereade_: :) [13:35] fwereade_: What I'm really interested on is where we're headed [13:36] fwereade_: The concrete type can have exactly the same name [13:36] fwereade_: Under the uniter or whoever will have the concrete implementation [13:37] niemeyer, I *think* I know, roughly, but the final shape is contingent on what I do with Relationer/RelationContext... and I don't have the foresight to know exactly how that will look until I'm free to hack them around a bit behind an interface, so I can experiment without spending 99% of my time fixing the build ;p [13:37] fwereade_: I thought RelationContext was quite clearly being defined by the ContextRelation interface, which will of course need a concrete implementation [13:39] niemeyer, sure, yes; so, you're right, it would make most sense to call it uniter.ContextRelation (unless it ends up in hook, which I think it will) [13:40] fwereade_: Yeah, in hook or wherever the concrete implementation ends up being.. it just feels like at least that portion of its role in the problem we're defined now.. [13:40] defining [13:40] niemeyer, but I believe it will come to include some of Relationer's responsibilities, and that the rest of Relationer will migrate into a new type [13:41] fwereade_: Understood, that stuff I'm less sure/aware about [13:41] niemeyer, in particular, I'm not sure whether that new type is going to be in uniter or hook [13:42] niemeyer, (well, *I* am as sure as I can be at this stage that it'll be in hook... but that's too many reviews in the future for that to hold much verifiable water) [13:42] fwereade_: Same here. Happy to see that stuff growing organically so we get a feeling of it while we go and don't spend too much time bikesheding on stuff that we can't really get right without further insight. [13:43] rogpeppe: This is ready on the pipeline, btw: https://code.launchpad.net/~rogpeppe/juju-core/105-cloudinit-initial-password/+merge/128222 [13:44] niemeyer: i realise. unfortunately it doesn't pass live tests, and i'm working on fixing that. [13:44] rogpeppe: Super, thanks [13:44] niemeyer, rogpeppe: btw, I misspoke earlier, I *do* need an Id() on ContextRelation, but that is not a big deal [13:45] fwereade_: Agreed, it makes sense anyway === niemeyer_ is now known as niemeyer [13:56] fwereade_: Reviewing the follow up now [13:57] fwereade_, rogpeppe, Aram: Btw, for synchronization, I intend to change state so it doesn't create the config nodes on demand, and instead handles them upfront at proper control points [13:57] I'll do that after I clean up the reviews [13:57] niemeyer, cool, thanks [13:57] niemeyer: that seems like a good thing [13:57] fwereade_: This is part of multi-config blah, btw [13:57] niemeyer: finally we change that. thank you. [13:57] Aram: np [13:57] niemeyer, <3 [13:58] fwereade_: https://codereview.appspot.com/6624062/diff/3001/worker/uniter/jujuc/config-get.go [13:58] fwereade_: Are these removals intended? [13:59] Hmm [13:59] niemeyer, yeah, I moved the logic behind Config() in HookContext, in the interest of implementing the interface "correctly" without duplicating the logic [13:59] niemeyer, I am aware it's not actually correct [13:59] niemeyer, but it's pre-existing and I'm resisting a lot of tempting rabbit holes in this pipeline [13:59] fwereade_: Cool [14:00] fwereade_: That's okay.. I just don't want us to mistakingly screw up Dave's efforts of having working charms [14:00] niemeyer, trust me, that thought was high up in my mind :) [14:00] fwereade_: Thanks [14:09] niemeyer: I've sent two new CL's this morning [14:09] niemeyer: https://codereview.appspot.com/6621062/ and https://codereview.appspot.com/6618064/ [14:10] fss: Thanks [14:10] niemeyer: after 6621062 get merged, I will start to send iamtest package (it uses RequestIdResp) [14:12] fwereade_: Review sent [14:13] niemeyer, tyvm [14:13] fwereade_: np [14:15] niemeyer, the public fields remain because the next step (after switching the commands to use the interface) will be to move the concrete HookContext into uniter... which will break the jujuc tests if they can no longer construct a HookContext neatly [14:15] niemeyer, they won't be there for long... [14:15] niemeyer, because the followup will be to write the command tests against a local implementation of the interface [14:16] fss: First looks nice.. just a couple of trivials [14:17] niemeyer, and re stupid doc comments... is it better IYO to leave them blank? or to duplicate the interface comments? [14:17] fwereade_: Okay, that's fine then, but let's show the path forward rather than documenting the transition as permanent then [14:17] fwereade_: Just leave blank IMO [14:17] fwereade_: They're documented in the interface [14:17] niemeyer, cool, I was going to do that and then I freaked out at the sight of a whole screen without comments ;p [14:18] fwereade_: Regarding _, can we please have a TODO comment inside the type explaining what we're doing, and tweaking the actual type docs so it doens [14:18] doesn't feel like the status quo is intended as permanent [14:19] niemeyer, absolutely so [14:19] fwereade_: Thanks a lot [14:23] fss: Second reviewed too.. good stuff as well with a trivial [14:24] niemeyer: cool :) I'm addressing your comments right now [14:32] niemeyer, rogpeppe: btw, is anyone else seeing http://paste.ubuntu.com/1267489/ on trunk? is there something I need to update? [14:35] niemeyer: thanks for you reviews [14:52] niemeyer: fwereade_: what do you want the new service relations watcher to return, []int? [14:53] Aram: +1 [14:53] because we don't have a function that takes an integer relation id and returns a relation ATM. [14:53] ok, so I'll add that function then. [14:54] fwereade_: I'm not sure.. I can't see the failure here [14:56] I'll step out for an early lunch.. biab for more fun [15:00] Aram, sgtm [15:00] Aram, I guess you're not seeing the failure I linked above? [15:00] no, I do not. [15:00] everything is fine here. [15:01] Aram, cool, thanks [15:20] US Holiday today so I will only be around sporadically [15:22] have fun [15:22] niemeyer: a trivial lbox fix for a panic i just had: https://codereview.appspot.com/6632043/ [15:27] niemeyer: PTAL at this - i had to make a few more changes to get all the tests to work correctly. (i changed the dummy provider to check EntityName too to catch the errors i was seeing in the live tests) https://codereview.appspot.com/6612054 [15:44] fwereade_: yes, i see that failure too [15:44] rogpeppe, ah, phew, I'm not insane :) [15:44] fwereade_: i'll have a look at fixing it, one mo [15:44] rogpeppe, you rock :) [15:45] fwereade_: i haven't fixed it yet :-) [15:45] rogpeppe, FWIW I think the deferred StopInstances just below that block is a bit too low down [15:45] rogpeppe, er just above [15:45] fwereade_: agreed [15:46] TheMue: ping [15:46] rogpeppe: pong [15:47] TheMue: i see a test failure in trunk that seems to be connected with port handling [15:47] TheMue: do you see the same failure? [15:47] rogpeppe: for the dummy provider? just submitted the fix [15:47] TheMue: ah, ok, cool [15:48] TheMue: although... i did just pull and i still see the failure, i think [15:48] rogpeppe: the test for the global firewall mode, which works fine for ec2 local and live, fails, my fault [15:48] TheMue: yeah, i still see the failure. [15:49] rogpeppe: eh, ok, wrong wording, i made a proposal, not submitted it [15:49] TheMue: ah! [15:49] TheMue: link? [15:49] TheMue: (i don't see many proposal emails sadly) [15:49] rogpeppe: https://codereview.appspot.com/6635043/ [15:50] rogpeppe: i wanted to extend the firewaller for the global mode and then i discovered the failure [15:59] rogpeppe: Looking [15:59] rogpeppe: Thanks for the lbox fix [15:59] niemeyer: yw [15:59] TheMue: (i don't see many proposal emails sadly) [15:59] I see too many [16:00] like 3 for each proposal or something [16:00] rogpeppe: would you take some of mine? [16:02] Aram: Yeah, sorry about that.. haven't yet found a way to have comments in both Launchpad and Rietveld without that overload [16:03] rogpeppe, pong, sorry, my sister just called to inform us that she is now engaged to be married [16:03] so, yay :) [16:07] Aram: that used to be true for me too [16:07] fwereade_: cool [16:07] fwereade_: i don't think i pinged you actually [16:08] * fwereade_ reads back and observes that he appears to be on crack [16:10] TheMue: you've got a review [16:17] rogpeppe: cloudinit done too [16:17] niemeyer: thanks [16:17] rogpeppe: np [16:25] rogpeppe: thx, appreciated it, will add it after dinner. [16:25] fwereade_: Did you figure what was up there? [16:25] niemeyer, apparently TheMue has proposed a fix [16:26] niemeyer, I'm holding off on merging anything for the moment in case it's trickier than it looks and demands a revert or something [16:26] fwereade_, TheMue: What was the bug? [16:26] fwereade_, TheMue: As far as I recall, the changes to the global port were all optional, and shouldn't have broken tests [16:27] niemeyer: the dummy provider test includes jujutest, but here the global mode failed because it didn't handle it [16:28] niemeyer: now the dummy provider supports the global mode too [16:28] TheMue: Okay, so I guess we should implement support on it/ [16:29] TheMue: This CL (https://codereview.appspot.com/6635043/) has changes to the firewaller too, in addition to the dummy provider [16:29] TheMue: Can we fix this bug without touching the firewaller? [16:29] niemeyer: i can split it, yes [16:29] TheMue: Sounds quite sensible, thanks [16:29] niemeyer: i detected it when extending the firewaller [16:30] TheMue: That's the way it always goes.. that's the point to stop a CL and do another one [16:30] niemeyer: yes, would have been better [16:31] TheMue: Then, can we talk about the theory behind the firewaller change? [16:31] niemeyer: will propose it after dinner, family calls [16:31] TheMue: Sounds sensible too, thanks [16:52] rogpeppe: You had a review that looked sensible on TheMue's branch.. would be interested in taking the patch on that one file and pushing something you're happier with, so we can fix trunk? [16:53] niemeyer: i can do, but i'm mid-flow currently, and i'd prefer not to lose context until i've fixed this test, if that's ok [16:53] rogpeppe: Absolutely, thanks [16:59] fwereade_: Ah, I can reproduce the failures now.. my branch lacked updates [17:03] niemeyer, jolly good :) [17:13] niemeyer: smallish CL. https://codereview.appspot.com/6639043 [17:14] rogpeppe: Handling trunk now [17:14] niemeyer: (currently verifying that live tests pass) [17:14] niemeyer: are you fixing trunk? that's great thanks. if not, i can move on to it now. [17:14] rogpeppe: I am [17:23] niemeyer: you asked what the firewaller CL has to do with the open task. this one nothing, but while thinking about the firewaller i dicovered that i can't simply close a port in global mode because another service may need it. so i added the open port counter. [17:24] TheMue: What happens if the firewaller is restarted? [17:25] niemeyer: have to check it again, but as all ports are re-opened then the counter gets new initialized. [17:27] TheMue: All ports are re-opened!? [17:27] niemeyer: as i said, have to recheck it. but please let me do the dummy CL before [17:28] TheMue: I'm already working on it.. doesn't solve the problem by itself [17:28] TheMue: We need more careful consideration of how this is going to work before implementing further ode [17:28] code [17:36] TheMue: Please don't worry about it for now, though.. it's late for you [17:37] niemeyer: yes, it is, but it's an interesting topic ;) [17:39] TheMue: So this may sound interesting: https://codereview.appspot.com/6615063 [17:39] rogpeppe: In case you want to check as well, given your prior review: https://codereview.appspot.com/6615063 [17:40] niemeyer: *click* [17:41] niemeyer: LGTM [17:43] niemeyer: unfortunately i seem to have broken trunk too. i must have live tested the wrong branch, dammit. [17:43] niemeyer: am just testing the (obvious) fix, then will propose [17:43] niemeyer: (it's only the live tests that break) [17:43] rogpeppe: Cool, thanks [17:44] niemeyer: LGTM, like the ports map idea. [17:52] niemeyer: 1 line fix: https://codereview.appspot.com/6635045 [17:53] niemeyer: can we get those two CLs merged? :-) [17:53] rogpeppe: Makes sense, LGTM [17:53] fss: Yeah, will look at them in a sec [17:54] niemeyer: sorry about that. i must work out some system for juggling my branches and tests better. [17:54] niemeyer: you're holding the lock on juju-core currently [17:54] rogpeppe: Trying to submit [17:54] niemeyer: ah, cool [17:54] niemeyer: that's not quick! [17:55] niemeyer: thanks [17:55] niemeyer: ... acquired 12 minutes, 12 seconds ago. [17:56] rogpeppe: Trust me, I want to submit as much as you do [17:57] niemeyer: i was commiserating, not complaining... [17:57] niemeyer: i've got to go, but i'll put my submit into a loop until it succeeds [17:58] rogpeppe: Don't worry, I can submit it once Launchpad works [17:58] niemeyer: that would be great, thanks. [17:58] rogpeppe: np [17:59] see y'all tomorrow [18:00] Launchpad simply doesn't work [18:15] Apparently I can't commit anything to LP [18:16] I wonder if it's just me or if it's a general issue [18:16] * niemeyer hops onto #lp [18:21] niemeyer, yeah, same here [18:53] Today is one of those days when the simple things seem unnaturally complex :) [18:54] * niemeyer prepares some chimarrão to relax a bit [19:21] Blowing away my local repository and starting over worked [19:55] fwereade_: ping [19:59] If someone is up for a simple one: https://codereview.appspot.com/6642046 [19:59] No semantic changes [22:26] * fwereade_ needs to restart to complete updates... but is going to subvert power relationships by shutting down instead. ha!