[02:35] anyone can help me with lbox? [02:38] andrewsmedina, what do you need? [02:38] hazmat: I need create a new proposal [02:38] andrewsmedina, lbox propose -cr from within a branch should do it [02:38] andrewsmedina, that will create a proposal (if one doesn't exist) or update an existing one [02:38] hazmat: but "lbox propose -cr" are updated a closed issue =/ [02:39] andrewsmedina, i don't parse that [02:39] what closed issue? [02:39] hazmat: how lbox knows if a proposal exist [02:39] andrewsmedina, it looks at the branch in lp and look for an attached merge proposal [02:39] with an expected syntax [02:39] andrewsmedina, if you need to create an entirely new proposal, you should delete the existing merge proposal attached to the branch [02:40] but you'll lose existing comments [02:40] hazmat: I updated the goetveld to works with the last weekly version [02:40] typically you just propose it again, and lbox will update the existing proposal to let reviewers know theirs something new [02:40] andrewsmedina, yeah.. i don't play the go monkey dance version game [02:41] hazmat but lbox are appending it with another proposal [02:41] hazmat: what? [02:41] andrewsmedina, ;-) just a general comment on 3rd party go libs.. pls ignore [02:41] andrewsmedina, what does lbox appending it to another proposal mean? [02:42] andrewsmedina, what are you trying to do? [02:42] hazmat: https://codereview.appspot.com/5683064/ [02:42] andrewsmedina, just run lbox propose -cr in the branch again [02:42] hazmat: a week ago I sent a proposal [02:43] it will append to the existing merge proposal and notify reviewers that there are changes [02:43] hazmat and today and now I sent another proposal [02:43] * hazmat nods [02:43] oh.. its already there [02:43] 16m ago.. [02:44] hazmat: lbox unified both, but are different proposals [02:44] andrewsmedina, lbox uses the branch diff [02:44] if you want two different proposals, you'd have two different branches [02:44] but in this case, your responding to feedback from the original proposal [02:45] and you've made an update, so that's fine imo being attached to the existing proposal [02:45] hazmat: ty [02:45] np [02:51] hazmat: You dont sleep? [02:57] andrewsmedina, nah.. i find it distracting ;-) [03:01] hazmat: :) [03:42] is hazmat 24/7? [14:08] Hello everybody [14:09] niemeyer: hiya [14:11] niemeyer, g'morning [14:13] niemeyer: let me know when you're up for that chat [14:17] rogpeppe: Sure, let's go [14:19] niemeyer: there's something about tomb.Fatal(tomb.Stop) that seems odd to me [14:19] niemeyer: it's a common path, and we already have a way of expressing "no error" - just use nil. [14:20] niemeyer: so i'm thinking tomb.Stop(nil) [14:20] niemeyer: then tomb.Err() would return ErrStillRunning if the tomb hadn't died. [14:21] niemeyer: which means the user wouldn't have to mess with particular error values at all in the usual case [14:21] rogpeppe: I've designed that interface while using it.. the Fatal(nil) is odd, and may easily hide a bug [14:21] niemeyer: how's that? [14:21] think Fatal(err) when err is nil [14:21] that's fine [14:22] just like return nl [14:22] nil [14:23] another thing: i'm not keen on the "Fatal" name. it's not fatal to anything - it just signals the tomb to die in the future. Stop feels better to me. [14:24] rogpeppe: Sorry, we already went over that one.. [14:24] rogpeppe: It's fatal for the goroutine [14:24] niemeyer: only if there's only one goroutine [14:24] rogpeppe: Besides, the analogy there is sane.. dying, fatal, etc [14:24] niemeyer: it's explicitly documented that it may be called multiple times [14:24] rogpeppe: Yep [14:24] niemeyer: the only other instances of Fatal do not return [14:24] rogpeppe: Yep [14:25] niemeyer: but this one does [14:25] rogpeppe: Yep [14:25] rogpeppe: The analogy there is strong enough that I'm comfortable with that [14:25] rogpeppe: tomb, dying, fatal, etc [14:25] Die might be better [14:25] or even Kill [14:26] rogpeppe: Yeah, kill might work [14:28] rogpeppe: Yeah, I guess the ErrStillAlive might be a nice improvement [14:28] niemeyer: thanks. [14:28] niemeyer: i've got a branch that i could propose [14:28] rogpeppe: I'm still a bit sad about losing the error check in Fatal/Kill/Whatever, but you're right that this is no different than returning an err by mistake [14:29] rogpeppe: Sure, please do, thank you [14:29] niemeyer: one other thing [14:29] and I guess I'll have to update the blog post again [14:29] (sorry 'bout that) [14:29] np [14:29] niemeyer: i wonder about how much Stopf (or whatever) will be used. [14:30] niemeyer: i wonder about just providing Kill [14:30] sorry, Killf [14:30] rogpeppe: No, please preserve it.. [14:30] niemeyer: ok. i was going by the return analogy [14:30] rogpeppe: This isn't return.. this is Fatal/Fatalf [14:30] niemeyer: you can always use fmt.Errorf (there's no need for tomb to depend on fmt) [14:31] rogpeppe: You can, just like you could always do errors.New(fmt.Sprintf(...)) all the time [14:31] rogpeppe: That's not what we do, though [14:31] niemeyer: i'm thinking that almost all the time you'll be doing Kill(err) anyway [14:31] rogpeppe: Please.. [14:31] ok [14:32] niemeyer: oh, yes one other thing :-) [14:33] niemeyer: it would be nice if we could embed tomb.Tomb in the same way we can embed sync.Mutex [14:33] niemeyer: i.e. if the zero'd form was good to go [14:33] rogpeppe: Yeah, it would be nice indeed [14:33] niemeyer: it's doable, but it means Dying and Dead have to be functions rather than fields. [14:33] rogpeppe: Yeah, not an option [14:34] niemeyer: no? it works ok like that (and saves space too) [14:34] rogpeppe: It'll incur in a function call for absolutely no reason every single loop [14:34] niemeyer: no it wouldn't. [14:34] niemeyer: they'd be inlined [14:35] rogpeppe: Not really [14:35] niemeyer: sure they would. it's a single statement function. it's inlined by default. [14:35] rogpeppe: If it's a single statement you're not doing anything within the function [14:36] niemeyer: ah, good point. yeah, it needs a mutex too. [14:36] rogpeppe: and make(), and if, and ... [14:36] niemeyer: yeah [14:36] (i added Tomb.init function when i tried it) [14:37] i still think it might be worth it though. [14:37] rogpeppe: Maybe.. New bothers me in that case too [14:38] New? [14:38] rogpeppe: tomb.New [14:38] niemeyer: why? i just deleted that function. [14:38] rogpeppe: Which means it bothered you as well, right? :-) [14:38] niemeyer: ah, i see [14:39] niemeyer: i thought there was maybe something more subtle [14:39] rogpeppe: Hmm.. I guess we can always have a variable when speed actually matters [14:39] rogpeppe: +1 on trying that [14:39] niemeyer: ok, cool. [14:39] niemeyer: actually... [14:39] oh yeah, [14:40] you mean the client code can have a variable [14:40] +1 [14:41] niemeyer: when i created a watch with ExistsW() on a not yet existing path and the path later will be created i should expect an EVENT_CREATED, shouldn't i? [14:41] rogpeppe: Thanks for these suggestions... feel like good improvement [14:41] niemeyer: thanks for going with me on this. if we're going to be using this a lot, i think it's good that we try to get it right. [14:42] niemeyer: cool, as above [14:42] rogpeppe: Indeed [14:42] TheMue: Yep, sounds snae [14:42] sane [14:43] niemeyer: hmm, i'm trying here my agent watch test, e'thing fine until the point where i'm waiting for the event. funnily i get none. [14:44] niemeyer: btw, i've got two proposals open. when can i expect your review? [14:46] TheMue: This week still [14:46] rogpeppe: When you have a moment, would you mind to have a look at the review that is pending? [14:46] rogpeppe: I want to close the store work today still [14:46] niemeyer: will do [14:46] rogpeppe: It's a pretty small one [14:47] niemeyer: ok, will continue work on the agent. [14:47] TheMue: Thanks.. I'll just finish that store work before moving back onto reviews, or we'll never have that [14:47] niemeyer: yep, the store is important [14:47] rogpeppe: Cheers [14:48] TheMue, niemeyer: here's a little working example of how watchers might work: http://paste.ubuntu.com/871544/ [14:48] (using the changed tomb API, BTW) [14:57] Sorry.. connection dropped [14:57] (using the changed tomb API, BTW) [14:57] rogpeppe: FooWatcher.Err should be using tomb.Err [14:57] rogpeppe: Looks good, otherwise [15:01] niemeyer: what's wrong with using tomb.Wait? [15:02] rogpeppe: It waits.. :-) [15:03] hmm, yeah, probably don't want Err to wait, you're right. [15:04] niemeyer: the order of the defers in FooWatcher.run is crucial then... [15:06] rogpeppe: Why? [15:06] rogpeppe: I think it is reversed indeed, FWIW, but why do you think it is crucial? [15:08] niemeyer: otherwise a watcher might see that Change is closed and call err, but before tomb.Done is called, so it'll see the tomb still running. [15:08] s/call err/call Err/ [15:08] rogpeppe: Good point.. [15:08] rogpeppe: It is good practice to always have defer tomb.Done() as the first thing in the goroutine, since it's supposed to flag "nothing else will happen" [15:09] niemeyer: this is perhaps why it might be reasonable to make Err block... [15:10] rogpeppe: Sorry, I think I lost the reasoning why what we just said above is invalid [15:10] niemeyer: i can't see Err being useful to get an error during the watcher run [15:10] rogpeppe: So remove Err.. [15:10] rogpeppe: Err must not block. [15:10] niemeyer: why's that? [15:11] rogpeppe: Because it's a question about what was the error, rather than a question "please block for as long as necessary" [15:11] rogpeppe: That's the convention [15:11] niemeyer: but perhaps we don't know if there was or wasn't an error yet [15:11] rogpeppe: And ... ? [15:12] niemeyer: using Err before Done has been called is inherently racy, i think [15:12] niemeyer: i'm not sure we want to support that [15:12] rogpeppe: Indeed. Please remove Err. [15:12] niemeyer: ok. from tomb as well? [15:12] rogpeppe: Nope.. tomb is fine as ti is [15:13] niemeyer: tomb has the same issue [15:13] rogpeppe: Nope.. it's a fine question, with a fine error in case it is running. There's no race. [15:14] niemeyer: ah, i'd preserved some of the old semantic - if there'd been a non-nil error, it returned it. but i think ErrStillRunning is better. [15:14] rogpeppe: That's what I understood you were going to propose in the branch [15:14] rogpeppe: Otherwise Kill(nil) is a no-go [15:15] niemeyer: yeah, my thinking was fuzzy [15:16] niemeyer: but there's still an issue (maybe a non-issue actually) [15:16] niemeyer: that if the Err method is removed from FooWatcher, there's no way for the watcher to fail with an error that isn't discarded. [15:17] niemeyer: so maybe just returning ErrStillRunning is fine there too. [15:29] rogpeppe: Sure, but I suggest waiting until we have the use case [15:30] niemeyer: yeah, maybe a simple closed channel is enough. any errors could be logged. [15:35] rogpeppe: The error comes out of the Stop too [15:36] niemeyer: that only works if you Stop it, of course, which you won't if you see EOF on the Change channel. [15:36] rogpeppe: Why not? [15:36] rogpeppe: Stop should always be called for all of them [15:36] niemeyer: why would you? you know it's already stopped. [15:36] niemeyer: ah. [15:36] rogpeppe: Because the logic is cleaner that way [15:36] rogpeppe: Which channel was closed? Etc [15:37] hmm, not sure about that as reasoning. you'll always know which channel is closed, no? [15:37] rogpeppe: Either way, a bit premature for that discussion.. let's have the branch implementing a watch for review first [15:37] ok [15:51] niemeyer: https://codereview.appspot.com/5755055 [15:52] niemeyer: just on your review now, BTW [15:52] rogpeppe: Cool, I'm proposing the second one with a trivial runner for the Server just now [15:52] After that, just need to implement /charm support, and I guess it's done [15:56] niemeyer, sudden thought: JUJU_CLIENT_ID might be better named JUJU_CONTEXT_ID; opinion? [15:56] niemeyer, given that it's at least in theory the key used to look up a particular context in which to execute hook commands [16:00] fwereade__: How's it set? [16:00] fwereade__: Sorry, what's its value [16:00] niemeyer, heh, well, in python it's always set to "client" [16:01] fwereade__: I think this is supposed to differentiate which process is running [16:01] niemeyer, sorry, expand please [16:02] fwereade__: How to find the relevant jujud process, more specifically [16:02] fwereade__: Or, I don't see how it is relevant at all.. [16:02] niemeyer, hmm, not sure, JUJU_AGENT_SOCKET should handle that [16:03] fwereade__: Hmm.. indeed [16:03] fwereade__: Why is that used at all? I suggest we don't add that variable until we can figure the answer that tricky question! ;-) [16:04] niemeyer, I think it's useful so that the hosted commands can be executed directly with the aid of a looked-up hook context that holds relevant data [16:05] niemeyer, this may be excessive given that ATM there's only one possible active hook context [16:05] fwereade__: Sorry, I don't get it.. how is that variable useful at all if it's always set to "client"? [16:06] niemeyer, it's not currently useful, because the python HookExecutor knows that it only ever has one active context [16:07] niemeyer, but that doesn't IMO actually improve simplicity or sanity [16:07] rogpeppe: You got a review [16:07] niemeyer, apart from anything else it means that anyone can execute a hook command at any time a context is active [16:08] fwereade__: How's that a problem? [16:08] niemeyer, I dunno, it just seems like a possible way for things to get screwed up [16:08] niemeyer: thanks [16:09] fwereade__: FWIW, I think JUJU_CONTEXT_ID, and using it as you describe is fine.. I'm just suggesting we don't introduce the variable before it is actually being use [16:09] d [16:09] fwereade__: IOW, if you [16:09] 're just going to reproduce the Python logic and have client as a constant, drop it [16:09] fwereade__: If you have plans to improve it and have it as an _actual_ context specifier, than sure, let's see how it loosk [16:09] looks [16:09] I can't type [16:10] niemeyer, I have every intention of using it as a context specifier ;) [16:10] niemeyer, cheers [16:11] Lunch time here! [16:11] * niemeyer is hungry today.. [16:13] niemeyer: enjoy. you've got a review too. [16:59] oh hey hazmat [16:59] meetingology: is here [16:59] jcastro: Error: "is" is not a valid command. [16:59] hazmat: do you know how to use it? [17:02] jcastro, huhu? [17:02] meetingology help [17:02] hazmat: (help [] []) -- This command gives a useful description of what does. is only necessary if the command is in more than one plugin. [17:02] hazmat: for logging meetings and stuff if you want [17:02] meetingology help plugins [17:02] hazmat: Error: There is no command "plugins". [17:03] https://wiki.ubuntu.com/meetingology [17:04] jcastro, its been a while since i've used it but sounds good, we do most of our meetings off irc these days [17:04] in g+ [17:04] right, I was doing the irc bot stuff and just put him in here just in case. [17:05] jcastro, sounds good [17:05] thanks [17:05] anyway you've got your logging back and stuff [17:05] so you should be all set. [17:07] rogpeppe: Re-reviewed [17:07] rogpeppe: Just a few trivials and I'll submit [17:12] niemeyer: you caught my global search and replace! [17:17] niemeyer: all done. [17:21] niemeyer: not so sure about "csapi" as a command name. possible alternatives: i wonder if "charmserve" or "charmsrv" or even "charmd" [17:21] oops [17:21] niemeyer: not so sure about "csapi" as a command name. possible alternatives: charmserve, charmsrv, charmd, charmstored [17:26] rogpeppe: cs is from the charmstore [17:26] rogpeppe: But it's a bit more specific in that this will work with what we call as "cs:" only [17:26] niemeyer: i know. but cs can stand for all kinds of things. [17:26] rogpeppe: Not for us [17:27] rogpeppe: Note that this is relevant only for IS for now [17:27] i guess i tend to prefer names that aren't entirely acronyms [17:27] rogpeppe: charmd sounds good [17:27] rogpeppe: charmload and charmd.. ok [17:27] sounds good to me [17:28] rogpeppe: I'll change that [17:28] niemeyer: cool, thanks [17:28] niemeyer: other LGTM [17:28] otherwise [17:29] rogpeppe: Haven't pushed the changes yet, but responded to your review on https://codereview.appspot.com/5755047/ [17:29] rogpeppe: Please see if it makes sense to you or if you need anything else [17:30] niemeyer: if you're using ServeMux, then you'll want to check the URL anyway. i still think that http://charmserver.com/charm-info/fooey should give a 404 [17:35] rogpeppe: Yes, I agree with that [17:35] rogpeppe: Will fix it [17:35] rogpeppe: But will keep the mux, as I don't want to do the basic multiplexing by hand [17:35] niemeyer: that's fine. [18:30] rogpeppe: Really like how the tomb changes turned out, cheers [18:40] what is it with my network currently? [18:41] i think it might be to do with my network driver rather than the ISP connection. [18:42] (this morning i was seeing many missed packets and >500ms ping times to my local router, where another adjacent computer was getting consistently getting <5ms [18:42] ) [18:42] niemeyer: did you manage to sort out your network problems? [18:44] fwereade_: i think it's not worth proposing more than one branch at once, unless the changes are independent. it's confusing trying to work out if the changes shown in codereview are to do with the proposed branch or not. [18:47] off for the night. see yous tomorrow. [18:50] rog: Nope.. [18:50] rog: I've been using a cable meanwhile [18:54] niemeyer: i'm moving to cable, as soon as i get the tool that enables me to wire an RJ45 jack, on order now. [18:54] rog: Are you finding issues too? [18:54] rog: Or is it just package losing? [18:55] s/package/packet/ === marrusl_ is now known as marrusl