=== TheMue_ is now known as TheMue === asavu_ is now known as asavu === asavu_ is now known as asavu === asavu_ is now known as asavu === asavu_ is now known as asavu [09:44] wrtp, are you free for a vague rambling discussion about hook contexts? [09:45] * wrtp enjoys vague rambling discussions. [09:45] fwereade_: certainly [09:45] mornin' BTW [09:46] wrtp, ok: I think I've figured out how Context should look, but I'm not sure what path I should take to get there [09:46] wrtp, mornin' :) [09:47] fwereade_: what general direction are you aiming in? [09:47] wrtp, I'm pretty sure that we only need a single `type Context struct`, with methods that map 1:1 to hook commands, which is cool [09:47] fwereade_: ah, the Context that was called Env, right? [09:47] wrtp, yeah [09:48] fwereade_: so this is from the agent's perspective? [09:48] wrtp, (I retain a feeling that it's not insane to have an Env interface and a Context interface, just for clarity of testing, but that's by the by_ [09:48] wrtp, yeah [09:49] wrtp, it seems that the way to do that is to have fields for local unit, unit relation, relation members, and remote unit [09:49] i'm not sure i understand [09:49] wrtp, and simply to error sensibly when someone asks for a capability we cannot provide [09:50] oh, i see, i think. [09:50] you think it's possible to make all hook commands context insensitive. [09:50] wrtp, so RelationSet, for example, just won't work if we don't have a .relation [09:51] wrtp, it sounds like that's on the roadmap [09:51] wrtp, and thinking about it from that perspective leads to IMO a nicer implementation right now [09:52] in which case, yeah, i could see that working. each field determines the behaviour of one or more callback methods. [09:52] wrtp, exactly [09:53] wrtp, ...hm ok I think I now know what I need to do in the exec branch [09:53] fwereade_: in fact, the callback methods need not be defined on the Context. [09:53] wrtp, my original plan was not to have them on the context [09:53] wrtp, I think it works out nicer if they are though [09:53] fwereade_: you can still use your approach of "remote command execution" if you want to. [09:54] wrtp, yep, that's almost entirely orthogonal [09:54] fwereade_: yeah [09:54] fwereade_: but i think it would look nicer if they were there, from an implementation perspective. they wouldn't need to be exported. [09:54] wrtp, the only difference is that JUJU_CONTEXT_ID maps back to a context which already has some of those fields filled in [09:55] wrtp, and we need to be able to optionally override which state we're working with on the command line [09:56] fwereade_: so the agent proactively fills in those fields rather than wait for the callback before reacting? [09:56] i'm not sure i understand your last remark [09:57] wrtp, sorry, it's somewhat speculative [09:57] what's "state" here? [09:57] wrtp, for now it's not necessary: it's just so we have a path to `relation-set $SOME_RELATION` foo-bar [09:57] wrtp, state in that case is $SOME_RELATION [09:58] wrtp, foo=bar^^ [09:59] hmm, still at sea [09:59] wrtp, it's far enough in the future that I can't predict exactly how it will look: for now, pre-filled contexts are exactly what we need [09:59] wrtp, sorry, I'll try to build it up a bit more logically [09:59] fwereade_: i'd like to understand your speculation... [10:01] wrtp, proximate goal: given a known, er "platonic" context for a given hook, we can construct a Context that knows how to get to the correct state.Unit/Service/UnitRelation in order to extract or manipulate it [10:02] wrtp, we can then implement Commands that just have the appropriate Context set on creation, and which can manipulate the context directly, concerning themselves only with input/output handling [10:04] fwereade_: the Commands are agent-side? [10:04] wrtp, ultimate goal: make the Commands capable of handling additional args that allow them to construct their own Contexts [10:04] wrtp, yes [10:05] hrmm [10:06] wrtp, that's not something I'm offering a *plan* for at the moment [10:06] i don't see why they'd need to do that. [10:06] wrtp, it's just a door I don't want to close [10:06] fwereade_: can't you ensure that there's always a context for a command? [10:07] wrtp, so that relation-set $SOME_RELATION can work out-of-band [10:07] fwereade_: even out-of-band commands may have a context, no? [10:08] fwereade_: (they've got to know how to talk to the agent, for a start) [10:08] wrtp, they do, but that context is essentially "what unit/relation are we working with here?" [10:08] wrtp, yep, so JUJU_AGENT_SOCKET may end up having to be global, IMO that's not an exceptionally big deal [10:09] wrtp, but I don't think we can know what relation relation-set wants to affect before it's called [10:09] fwereade_: i think that is a big deal when we allow multiple unit agents in the same container [10:10] wrtp, huh, yeah, true [10:10] bah [10:10] fwereade_: so given that, i think we can always assume JUJU_CONTEXT_ID, and hence some context. [10:11] niemeyer: hiya [10:11] wrtp, I think they're different problems [10:12] Heya! [10:12] Mornings [10:12] wrtp, out-of-band execution means we *can't* depend on JUJU_CONTEXT_ID [10:12] morning niemeyer [10:12] fwereade_: i don't think it does [10:12] fwereade_: environment variables get exported [10:12] fwereade_: out-of-band doesn't mean no-relation-to. [10:14] wrtp, get exported to where? my understanding is that we eventually intend `relation-set foo bar=baz` to work even when called in response to some action taken by the service itself in an entirely unpredictable environment [10:14] fwereade_: once a hook context has "expired" (when the hook has finished executing), we could make the old context id to a generic "out of band" context. [10:14] s/make the/map the/ [10:14] fwereade_: i don't think that can possibly work [10:15] wrtp, I don't see how that's any different to just not having a context id [10:15] fwereade_: maybe it's not. JUJU_AGENT_SOCKET is the important thing. but... [10:16] wrtp, there's the how-do-we-talk-to-the-right-agent problem but that's probably soluble even if it doesn't turn out especially elegant [10:16] fwereade_: perhaps we might want to make callbacks within the context of a given hook (even if the hook has completed), still have some hook-specific behaviour [10:17] fwereade_: wrtp, out-of-band execution means we *can't* depend on JUJU_CONTEXT_ID [10:17] fwereade_: Kind of.. JUJU_CONTEXT_ID is what says it's out-of-band in the first place, right? [10:18] niemeyer, lack thereof, yes [10:18] niemeyer: i think there are two kinds of out-of-band here [10:18] niemeyer: 1) called after the hook has returned [10:18] niemeyer: 2) called from an independent environment [10:19] niemeyer: i'm thinking about 1), and i think we can rule out 2) [10:19] wrtp, in (1), the old hook context is potentially *wrong*, so I think that's the case we should rule out [10:19] in 1), you've always got a JUJU_CONTEXT_ID [10:19] wrtp, ...which maps to a context with a potentially out-of-date member set [10:19] fwereade_: at least we know when the context is wrong, and can do something appropriate [10:20] wrtp, how's thatany different to just not having a context and knowing we have to "do something appropriate"? ;) [10:20] wrtp, ie somehow obtain the correct context to work with [10:21] fwereade_: because i imagine that the behaviour might differ depending on the original hook context. [10:21] wrtp, why? [10:21] wrtp: I don't understand what you mean by that [10:21] wrtp: 1 or 2 [10:22] fwereade_: Agree regarding the out-of-date member set [10:22] fwereade_: But, it's a bit of a tricky situation.. [10:22] ok... here's a tangential question: which hook callbacks are not ok to call in which hooks? [10:23] fwereade_: Why is a command spawned in the background by a hook different from a command spawned in the background by something else? [10:23] wrtp, *at the moment* you can't do relation-set,-get,-list outside a relation hook [10:23] fwereade_: ok, but we'd change that, right? [10:24] fwereade_: 'cos it's silly if we allow them out-of-band but not in certain hooks [10:24] wrtp, and attempting a relation-set in a departed hook will error out (which I think we should also change -- no reason not to be able to set, even if nobody's going to read it) [10:24] niemeyer: that's an easy answer - because the command spawned in the background by something else can't know what unit it's executing in [10:24] wrtp, yep, which is why this is a bitof a digression ;) [10:24] wrtp, but why should it care? [10:24] wrtp, ok, it should care because AGENT_SOCKET [10:25] fwereade_: exactly [10:25] fwereade_: and relation-get returns something different depending on what unit it's in [10:25] wrtp: A command spawned in the background isn't necessarily executing in any unit.. [10:25] niemeyer: no? [10:25] wrtp, indeed [10:25] wrtp: Sorry, bad terminology of my part [10:26] wrtp: What I mean is that it's not necessarily executing in any relation.. you can spawn a server, and have it executing relation-set reactively, for instance [10:26] niemeyer: yeah, that seems fine. [10:27] It shouldn't blow up just because it was started within a hook rather than by the init scripts [10:27] In that sense, it's out-of-band as well, and it should work [10:27] niemeyer: i think we're leading towards the idea that all the callbacks should work independently of whether they're called inside a hook or not. [10:27] niemeyer: and all callbacks should be valid all the time. [10:28] wrtp, exactly so [10:28] except... [10:28] wrtp, I hadn't picked up on this until I spoke to niemeyer ?yesterday? [10:28] that what about races [10:28] ? [10:28] i.e. within a hook, you know that relation-get is consistent [10:29] but outside a hook, it might be constantly changing. [10:29] ha [10:30] maybe there could be some way of *asking* for a context id [10:30] wrtp: Indeed. We can allow the creation of contexts at some point, for providing the same guarantees a hook has, but that's a distant future. Commands don't even work out of hooks yet. [10:30] JUJU_CONTEXT_ID=$(get-context-id) [10:30] wrtp, yeah, was just pondering same [10:31] niemeyer: yeah, this all goes back to some current design decisions which were being influenced by this speculation... [10:31] wrtp, we'd need some way to retire/discard them as well, but details details [10:31] Might be intersting to marry context id and the socket [10:31] fwereade_: ^ [10:32] niemeyer, hm, yeah [10:32] fwereade_: While we still have time :) [10:32] niemeyer: i was thinking the same [10:32] niemeyer, I don't think that door is closed [10:33] fwereade_: JUJU_AGENT_SOCKET=[@] [10:33] niemeyer, neither context id nor agent socket are actually intrinsic to the context, I think [10:33] niemeyer, context id is needed to look up a context, but I think the only reason it's stored in the context is for convenience [10:34] niemeyer, (...when constructing the env vars for a hook) [10:34] fwereade_: Right [10:34] JUJU_CONTEXT=:path ? [10:34] niemeyer, similarly for the agent socket: they'll both need to be controlled by something other than the context itself [10:34] fwereade_: It's also provably not necessary right now, which means we can make it optional [10:35] fwereade_: Actually, now that I think of it, it's only optional because we don't have anything else but context-rich execution [10:35] niemeyer, yeah, I don;t want this to affect the implementation right now, but I don;t want to make assumptions that will make it *harder* to implement this when the time comes [10:35] fwereade_: So once we introduce out-of-band commands, context-rich execution needs the context info [10:36] niemeyer: yes [10:36] fwereade_: I suggest taking the context stuff out [10:36] fwereade_: I mean, the context id, more specifically [10:36] fwereade_: Unless you're already making use of it in a way that moves us closer to these ideas we're talking about [10:37] niemeyer, so Exec takes a context id and an agent socket in addition to the context itself, which doesn't itself know about them? [10:37] fwereade_: Hmm.. context id and the agent socket are part of the context right now.. [10:37] niemeyer, feels like a bit of a speculative interface complication to me... not sure it makes the putative changes any easier [10:38] don't we need a context id to stop inappropriate execution of hook commands after the hook has finished? [10:38] fwereade_: I was just suggesting dropping context id from the context, until we have a reason to hold it [10:38] niemeyer, OTOH having them on the context potentially encourages people to use them inappropriately [10:38] wrtp, we need it but it doesn't have to live on the context [10:38] fwereade_: What's "context" in this sentence.. it feels like we're talking about different things? [10:40] niemeyer, the type which (1) ultimately supplies env vars to Exec and (2) exposes useful methods to the hookcommand implementations [10:40] niemeyer, the actual execution environment the hook runs in needs a context id ATM, no argument there [10:40] fwereade_: Those feel like different things.. [10:41] niemeyer, well, I thought so too, hence separation between Env and Context [10:41] niemeyer, but I'm starting to think otherwise [10:41] fwereade_: Regarding your second sentence above, ok, so let's keep ContextId in Context (perhaps calling it Id, though?) [10:41] niemeyer, yeah, that's how my current latest sketch looks [10:42] fwereade_: Cool [10:43] fwereade_: hmm, i have to say i was quite convinced by our earlier discussion [10:43] fwereade_: Exec needs a type that it can use to execute a command.. this feels isolated from the aspect of hook commands themselves, at least as far as I can foresee at the moment [10:43] fwereade_: i think Context works well combined with Env [10:43] type Context struct { [10:43] Id int [10:43] Vars map[string]string [10:43] Relations map[string]string [10:43] Unit *Unit [10:43] } [10:43] or whatever [10:44] but... yeah [10:44] niemeyer, but it's true that as you suggested everything in Env can be derived from stuff we have in the context [10:44] the Env is independent, because we might have a context without running a hook [10:44] niemeyer, and there's not much justification for a separate type/interface [10:44] fwereade_: Right [10:44] niemeyer, except in that testing becomes a bit easier [10:44] fwereade_: I see two things, but they're not the same two things that were in the original branch [10:45] fwereade_: that's an excellent point though [10:46] niemeyer, heh, I can't precisely remember the original branch now... I thought I had a Context that exposed Env()... and the Commands() thing is no longer really relevant to the conversation [10:46] fwereade_: It's quite possible as well that you had a perfect grand vision, and I just assumed things incorrectly from looking into part of it. [10:47] fwereade_: Context was lost in that branch. It may make sense for another concept, but it doesn't belong into that branch, or in that file even [10:47] niemeyer, definitely not perfect, our discussion the other day modified it a certain amount, but I think the broad strokes are similar enough [10:48] fwereade_: What we need is Exec and ExecVars (your Env, just avoiding the name conflict) [10:48] niemeyer, ah yes, it was just Env which I was semi-expecting the eventual Context type to implement, but wasn't yet sure about: hence just an easy-to-independently-test interface [10:49] niemeyer, I'm actually not so sure about that: I think we just need Context to have `Vars() []string` and leave it at that [10:50] fwereade_: I can easily buy into that too [10:50] niemeyer, whether we ultimately pass a Context or an []string to Exec depends on what makes sense in the final context [10:51] fwereade_: I don't think it makes sense to pass a context to Exec.. [10:53] niemeyer, well, it needs Vars and a way to get to the actual hook, which IMO it's sensible to calculate from Context.CharmDir [10:53] fwereade_: i think that both Context and Env can be concrete types. [10:53] niemeyer, we could ofc just have context take (vars []string, hook string) where hook is an absolute path claculated elsewhere [10:55] fwereade_: Agreed. This whole design may actually change a bit once we see the rest of what's necessary [10:55] fwereade_: It's strange to have CharmDir in Context, for example. [10:56] niemeyer, agreed, now you mention it :) [10:56] fwereade_: Many of those properties aren't properties of a context.. they are properties of a unit [10:57] fwereade_: So it feels a bit like we're trying to reverse engineer how the bottom looks like by looking at the tip [10:57] fwereade_: WHich IMO is why we're a bit lost in that back and forth debate [10:57] niemeyer, true... but I'm not sure there's such a thing as a context without a unit, even when out-of-band [10:57] fwereade_: There's no context without a unit.. but a unit has multiple contexts [10:57] niemeyer, true [10:58] hmm, i wonder what happened there [10:58] fwereade_: i missed everything after "niemeyer, we could ofc just have context take (vars []string, hook string) where hook is an absolute path claculated elsewhere" [10:58] (if there was anything) [10:59] fwereade_: Agreed. This whole design may actually change a bit once we see the rest of what's necessary [10:59] fwereade_: It's strange to have CharmDir in Context, for example. [10:59] niemeyer, agreed, now you mention it :) [10:59] fwereade_: Many of those properties aren't properties of a context.. they are properties of a unit [10:59] <-- wrtp has quit (Read error: Operation timed out) [10:59] fwereade_: So it feels a bit like we're trying to reverse engineer how the bottom looks like by looking at the tip [10:59] fwereade_: WHich IMO is why we're a bit lost in that back and forth debate [10:59] niemeyer, true... but I'm not sure there's such a thing as a context without a unit, even when out-of-band [10:59] fwereade_: There's no context without a unit.. but a unit has multiple contexts [10:59] niemeyer, true [10:59] fwereade_: To solve the current debate and make some progress, I suggest we move on with you suggestion. Let's make Exec trivial.. Exec(vars []string, path) as you suggested. [10:59] fwereade_: thanks [11:00] niemeyer, sounds good to me [11:00] fwereade_: I suspect the correct thing to do will be a lot more obvious once we reach these fundamental layers [11:00] niemeyer, yeah [11:00] vars map[string]string, presumably [11:00] fwereade_: I also suggest sleeping on the idea of how a context relate to a unit, and what's the proper way to organize the relationship between those concepts [11:01] wrtp: That'd work, yeah [11:03] wrtp, not sure it matters much either way, not unreasonable to pass vars around in os.Environ style [11:03] wrtp, once they're fixed anyway [11:03] fwereade_: i think os.Environ might be a map now... [11:03] wrtp, ofc they're easier to manipulate as map[string]string [11:03] fwereade_: no, it's not [11:04] fwereade_: there was *something* like that :-) [11:05] niemeyer, wrtp: I'm just going to try to flesh out my current sketch a little and see if any new enlightenment dawns [11:06] niemeyer, wrtp: well *actually* it seems I'm going to eat lunch [11:06] lol [11:06] ttyl :) [11:06] fwereade_: luncheon enlightenment? [11:06] haha [11:06] by light alone [11:06] fwereade_: you read that yet? [11:08] fwereade_: Lunch.. hmm.. still several hours to go here, how unfortunate :) [11:08] fwereade_: Enjoy :) [11:09] niemeyer: []string is better actually. that's the type of the Env field in exec.Cmd. [11:13] wrtp: Right, but it may be easily assembled within Exec.. your suggestion is likely a better interface to work with from within the package [11:14] niemeyer: it's possible. but i suspect that in practice all the calls will look like: Exec([]string{"FOO="+foo, "BAR="+bar}, ...) [11:14] niemeyer: that is, i wonder how much we'll actually "work with" the environment variables. [11:15] wrtp: That looks like a map to me [11:15] niemeyer: no more than os.Environ or exec.Cmd.Env :-) [11:15] wrtp: Never built one of those by hand. [11:16] niemeyer: i'm happy to minimise impedance mismatch where it doesn't make much difference. [11:17] niemeyer: but i don't mind too much (i did suggest a map originally, after all) [11:17] wrtp: :-) [11:18] wrtp: i would uses an own type based on map[string]string here, having methds to pass it as []string [11:19] wrtp: so the usage is simple and passing the content to Exec() too [11:19] niemeyer: one possible place where it's actually easier to work with a slice is if your env vars are mostly constant, with one changing. then you can do Exec(append(constantVars, "ONE_VAR="+...)) [11:20] wrtp: m["ONE_VAR"] = ... [11:21] niemeyer: that doesn't work if you don't want the map to be changed for everyone [11:21] niemeyer: you'd have to copy the map first [11:21] wrtp: That's not how exec works.. [11:21] wrtp: could be done with my type too. even more comfortable in a method myEnv.WithNewVariable(k, v string) []string [11:21] wrtp: w/o changing it, using the append internally [11:22] i think all this is overthinking it. i think in practice the usage will be ultra simple. [11:22] Let's please move on. Whatever fwereade_ picks is fine. [11:22] agreed [11:22] wrtp: Yes, and you've been redefining how to work with a map above.. [11:22] niemeyer: ? [11:22] wrtp: ! [11:23] [11:21] wrtp: That's not how exec works.. [11:23] depends whether the map is used in a concurrent context [11:24] anyway, let's leave it. [11:24] wrtp: Both structures are mutable.. a process environment isn't mutable. [11:26] niemeyer: i didn't say it was. i was referring to m["ONE_VAR"] which will change m for everyone unless you create it every time. it's not a big issue. [11:26] wrtp: So will append.. [11:26] fwereade_: Please use a map.. let's see how it looks like. [11:27] niemeyer: not if the slice has cap == len. but perhaps that's too fragile. [11:27] wrtp: Indeed it is. append mutates the slice. [11:27] niemeyer: only cap!=len [11:27] wrtp: take a look at http://paste.ubuntu.com/883111/ [11:28] wrtp: Indeed it is fragile. [11:28] wrtp: untested quick hack [11:28] wrtp: Gosh.. [11:28] wrtp: These arguments really consume our time for no good reason. [11:28] yeah [11:28] let's not [11:29] sorry, it's my fault! [11:30] * TheMue sighs [12:13] lunchtime [12:14] niemeyer, I think that Exec does actually need a Context... we'll need to flush settings writes once the hook's complete [12:25] fwereade_: can't that be done by the caller of Exec? [12:27] fwereade_: Agree with wrtp's questoin [12:28] fwereade_: Also, who calls Exec [12:28] wrtp, well, it could; but `Exec(vars map[string]string, path, dir string)` is actually so trivial a wrapper around os/exec that I'm not sure it's meaningful any more [12:28] niemeyer, fwereade_: is it ok if i leave the log prints in the live test until we hook up logging to the tests? [12:28] fwereade_: I don't have the full picture, so don't really have any strong recommendations at this point.. I'd just try to work out a way to keep the concepts nicely isolated [12:28] fwereade_: we had too long a discussion about this earlier. go with what you feel like :-) [12:29] wrtp, I question the value of logging those tests at all, really, but I don't feel all that strongly about it [12:29] niemeyer, wrtp: cool, cheers [12:29] fwereade_: the logging only appears in verbose mode. sometimes it'll hang forever and it's useful to know where., [12:29] wrtp: Hooking up logging to the tests is done by log.Target = c in SetUpTest [12:29] wrtp: We already do that in some suites [12:30] wrtp, ok, cool, that makes sense then [12:30] wrtp: I'm happy with the branch to move forward as it is, though, and have that in a separate one [12:31] niemeyer: i can do that, but i'd need to add log.Printf calls too. i'd prefer to do it in a separate branch [12:31] yeah [12:35] fwereade_: Just as a nice realization, everything in that branch *is* a thing wrapper on top of Exec.. [12:35] s/thing/thin [12:37] niemeyer, agreed, but imo it's sane for "hook.Exec" to know about the particular idiosyncracies of hook execution [12:37] niemeyer, it'd be a different matter if it were "util.Exec" or something [12:37] fwereade_: Agreed [12:38] fwereade_: But there are none there, yet.. [12:39] fwereade_: I see your point, though.. it may well make sense to have a context there [12:40] fwereade_, niemeyer: submitted! thanks. 144 commits it took :-) [12:40] wrtp, that's gross [12:40] * fwereade_ tries to keep a straight face [12:40] wrtp: 2 of those are mine! ;-) [12:41] niemeyer: 2 of your commits say "committer: Roger Peppe" ? [12:42] wrtp: Nah, nevermind [12:42] anyway, i'm very happy to have got there in the end! [12:43] aww, I was hoping for at least a groan of pain from someone, it's rare to get such a perfect opportunity for a really ugly pun [12:47] * wrtp lets the pun sail way over his head. [12:49] * wrtp lunches [12:58] fwereade_: So, turns out I lied to you.. proposing again with lbox won't mark the branch as Needs Review [12:59] fwereade_: I'm fixing that now, but just wanted to mention in case you reproposed some branch before that I'm not seeing! === marrusl_ is now known as marrusl [13:23] niemeyer, ah-ha, I suspect I have a branch or two languishing then, let me check [13:24] niemeyer, nope, go-add-cmd-context is still NR (but then I don't think I ever WIPed it); I'll bear it in mind though [13:24] niemeyer, thanks [13:32] good morning [13:33] andrewsmedina: Heya [13:33] fwereade_: np [13:34] fwereade_: Already fixing it, so hopefully won't bite in the future [13:34] niemeyer, sweet, tyvm [13:34] andrewsmedina, good morning [13:34] niemeyer: I saw the reviews made for you and rog [13:34] fwereade_: I hope to fix pre-req right after [13:35] niemeyer, <3 [13:35] fwereade_: *that* will be a real win :) [13:35] niemeyer, maybe even <4 [13:35] andrewsmedina: Super.. sorry for some confusion there [13:35] fwereade_: LOL [13:35] fwereade_: We should totally start using <4 as a convention :) [13:36] niemeyer, I have a vague feeling I stole it from kingdom of loathing [13:36] niemeyer: I realy need parse the lxc error output? [13:37] andrewsmedina: Not really parsing.. the basic idea is to include it in the error.. but it'd be nice to strip out the usage message in case it's there [13:37] andrewsmedina: so [13:37] output, err := ... [13:38] niemeyer: I'm already doing it [13:38] if i := bytes.Index(output, []byte("\nUsage: ")); i > 0 { [13:38] output = output[:i] [13:38] } [13:38] andrewsmedina: That's all really [13:39] niemeyer: I will do it now [13:43] andrewsmedina: Thanks a lot [13:43] I promise I won't ask why setStatus is its own method in a merge_proposal object, while everything else is just changing fields [13:56] fwereade_, wrtp: I'll also invert the meaning of -same-log, and make it the default behavior, as suggested by wrtp before. [13:56] wrtp: You were right.. showing the logs in all cases is boring, and people don't update the log even if it's in their face. [13:57] niemeyer, I update the log (er, sometimes) [13:57] niemeyer: ECONTEXT [13:57] niemeyer, but I can bear to type a few more chars when I need to ;) [13:58] wrtp: Fixing lbox [13:58] * wrtp can't remember what command -same-log is a flag to... [13:59] ah, i remember now [13:59] fwereade_: It will continue to be shown on submit too, so that's always a good hooking point to get the log right [13:59] log==description [13:59] niemeyer, excellent [14:00] i've said this before, but i think it's a pity that so much functionality is shoehorned into "lbox propose" [14:01] niemeyer: i know the conception of lbox is as a general interface to launchpad, but will it actually end up as any more than propose+submit? [14:02] wrtp: Yes [14:03] niemeyer: BTW it would also be great if we could somehow reduce the volume of emails that are sent as a result of lbox. [14:03] each change generates about 3 emails. [14:04] wrtp: Each change generates two emails, one from codereview, one from Launchpad. I don't know of any way to avoid the email from Launchpad without also losing the history in the merge proposal itself. [14:04] niemeyer: there's also the two emails that are sent when you actually send the comments on the codereview page... [14:05] wrtp: Exactly.. each change generates two emails. [14:06] niemeyer: it means i can't respond to some comments with some changed code without generating 4 emails. [14:06] wrtp: I apologize for the trouble. I don't know how to solve the issue. [14:06] niemeyer: yeah, i know. maybe i should just filter out all emails that simply say "please take a look" [14:07] niemeyer: because they're often noise, and the real ones are those sent direct from codereview [14:07] wrtp: They're not noise for me, but whatever works for you is certainly fine for me. [14:08] wrtp: Many times people don't answer the feedback, and simply do an "lbox propose". You'd miss those. [14:08] niemeyer: if there was a way of saying "please upload these changes *and* publish my codereview comments", that would be nice... [14:08] wrtp: That's already what it does.. [14:08] niemeyer: yeah, i know. [14:09] niemeyer: really? [14:09] wrtp: I believe.. [14:09] wrtp: Yep.. try it out and let me know :) [14:09] niemeyer: so lbox propose publishes pending codereview comments, you think? [14:10] hmm, that would be good, if slightly unexpected. [14:11]