[01:39]  * thumper loos for wallyworld_
[01:39] <thumper> s/loos/looks/
[01:39]  * thumper sighs
[01:39] <wallyworld_> hello
[01:40] <thumper> wallyworld_: s'up?
[01:41] <wallyworld_> finishing some work to make juju easier to use
[01:41] <wallyworld_> using simplestreams metadata, public bucket url in keystone catalog etc
[01:42] <wallyworld_> i have something like 6 branchs in review
[01:42] <thumper> heh
[01:42] <wallyworld_> what you doing?
[01:42] <wallyworld_> you were off last  week?
[01:42] <thumper> I'm just about to put up the three branches I have to implement plugins fully
[01:42] <thumper> yes, last week was a holiday for me
[01:42] <wallyworld_> yay
[01:42] <thumper> about to spend the afternoon breaking down work items into bite sized chunks
[01:43] <wallyworld_> did you go away anywhre
[01:43] <wallyworld_> apparently me, you and frank are on containers
[01:44] <thumper> wallyworld_: no, stayed at home and hacked on my personal project
[01:44] <thumper> made good progress
[02:32] <marcoceppi> Makyo: did you need help with anything in particular?
[02:32] <marcoceppi> err, kyhwana ^^
[02:33] <kyhwana> marcoceppi: not yet ;)
[02:33] <kyhwana> still work time *power cycles some more boxeS*
[02:33] <kyhwana> XFS doesn't like it when you're building a kernel on it and you power cycle the box, even with RAID BBUs :(
[02:34] <marcoceppi> Well, it's sleep time here, wanted to make sure you weren't stuck on something before I powered down
[02:34] <kyhwana> oh thanks, will see how I get on
[02:34] <marcoceppi> kyhwana: feel free to ping in #juju if you have a problem. A lot more charmers tend to lurk in there
[02:35] <marcoceppi> cheers o/
[07:05] <dimitern> g'morning
[08:37] <rogpeppe1> dimitern: hiya
[08:37] <dimitern> rogpeppe1: heyhey
[08:37] <dimitern> rogpeppe1: still struggling with (srv)Pinger stuff
[08:38] <dimitern> rogpeppe1: but i hope it'll end soon
[08:38] <rogpeppe1> dimitern: ok. feel free to ask if you think there's something i could help with.
[08:38] <dimitern> rogpeppe1: cheers
[08:46] <dimitern> rogpeppe1: so to be able to use the pinger client-side would I need to define func (r *srvRoot) Pinger(id string) (srvPinger, error) ?
[08:46] <rogpeppe1> dimitern: yes
[08:46] <dimitern> rogpeppe1: not just have func (m *srvMachine) SetAgentAlive() (params.PingerId, error)
[08:46] <rogpeppe1> dimitern: well, probably you'd return *srvPinger
[08:47] <rogpeppe1> dimitern: yeah, you'd need that too
[08:47] <dimitern> rogpeppe1: but EntityWatcher() returns just srvEntityWatcher
[08:47] <rogpeppe1> dimitern: ah, probably because it's only wrapping a pointer.
[08:47] <dimitern> rogpeppe1: and errUnknownWatcher
[08:48] <dimitern> rogpeppe1: it looks like it's just an empty struct rather than wrapping a pointer
[08:49] <dimitern> rogpeppe1: s/empty/uninitialized/
[08:49] <dimitern> rogpeppe1: oh.. you know what i mean :)
[08:49] <rogpeppe1> dimitern: it's only returning a zero struct when it returns an error
[08:50] <dimitern> rogpeppe1: should I follow this and define an error as well or just return a pointer?
[08:50] <rogpeppe1> dimitern: otherwise it wraps the *srvWatcher
[08:50] <rogpeppe1> dimitern: those are weird alternatives :-)
[08:51] <rogpeppe1> dimitern: type srvPinger {*srvWatcher} would seem to be the right approach
[08:51] <dimitern> rogpeppe1: I prefer a pointer actually
[08:51] <rogpeppe1> dimitern: there's no point :-)
[08:51] <dimitern> rogpeppe1: yeah, I did that already
[08:52] <rogpeppe1> dimitern: it just means one more allocation for no particular reason
[08:52] <dimitern> rogpeppe1: so have errUnknownPinger and use the same pattern
[08:52] <rogpeppe1> dimitern: yeah
[08:52] <dimitern> rogpeppe1: ok
[09:01] <dimitern> rogpeppe1: so which methods do I need to implement on srvPinger? there are Start, Stop and Kill defined, but for srvEntityWatcher I can see only Next() defined and there are more in state.EntityWatcher
[09:03] <rogpeppe1> dimitern: i think you probably want Stop and Kill
[09:03] <dimitern> rogpeppe1: but how to start it?
[09:03] <rogpeppe1> dimitern: SetAgentAlive starts it
[09:03] <dimitern> rogpeppe1: ah, right!
[09:04] <rogpeppe1> dimitern: BTW, i've just noticed something
[09:04] <dimitern> rogpeppe1: yeah?
[09:04] <rogpeppe1> dimitern: you probably the resource to Kill the pinger rather than Stopping it
[09:05] <rogpeppe1> dimitern: which means you won't be able to just embed the srvResource
[09:05] <dimitern> rogpeppe1: why?
[09:05] <rogpeppe1> dimitern: because when a connection goes away, i *think* we want the agent to be reported as dead immediately
[09:06] <dimitern> rogpeppe1: seems right
[09:07] <dimitern> rogpeppe1: but this means I need to both embed srvResource and define Kill
[09:07] <dimitern> rogpeppe1: ah, I see your poing - implement Stop() as Kil()
[09:07] <rogpeppe1> dimitern: you'll need srvResource as a field, and yes, you'll need to define Stop and Kill on srvPinger
[09:07] <rogpeppe1> dimitern: yeah
[09:08] <dimitern> rogpeppe1: I need srvResource because?
[09:08] <rogpeppe1> dimitern: i think the naming distinction between Stop and Kill in Pinger is a bit too subtle
[09:08] <dimitern> rogpeppe1: s/need/need to embed/
[09:09] <dimitern> rogpeppe1: actually wrt to the logic above, do I need Stop at all?
[09:09] <rogpeppe1> dimitern: not strictly speaking
[09:09] <rogpeppe1> dimitern: if we don't currently use it
[09:09] <rogpeppe1> dimitern: i think that we should probably have Pinger.Stop and Pinger.StopButLeaveRunning :-)
[09:10] <rogpeppe1> dimitern: or something like that
[09:10] <dimitern> rogpeppe1: why the second one?
[09:10] <rogpeppe1> dimitern: that's what Stop does currently
[09:11] <dimitern> rogpeppe1: I can't see Stop being used - perhaps i'm not looking in the right place
[09:11] <rogpeppe1> dimitern: currently Stop is the only method being used
[09:11] <rogpeppe1> dimitern: which is perhaps not quite right.
[09:11] <dimitern> rogpeppe1: I meant presence.Pinger.Stop
[09:12] <rogpeppe1> dimitern: that's what i meant too
[09:12] <rogpeppe1> dimitern: basically, we should really only be using pinger.Stop when we know we're upgrading
[09:12] <dimitern> rogpeppe1: where is it used?
[09:12] <rogpeppe1> dimitern: in worker/machiner/machiner.go:/watcher.Stop
[09:13] <dimitern> rogpeppe1: hmm
[09:13] <rogpeppe1> dimitern: but actually...
[09:13] <dimitern> rogpeppe1: when upgrading we need the pinger stopped but alive somehow?
[09:14] <rogpeppe1> dimitern: well yes - we don't want to see that the agent has died when in fact it's just restarting
[09:14] <dimitern> rogpeppe1: it *is* dead until restarted, isn't it?
[09:14] <rogpeppe1> dimitern: just temporarily inactive :-)
[09:15] <dimitern> rogpeppe1: i don't see how this matters - it might not come up again
[09:15] <dimitern> rogpeppe1: and when it does it'll start a new pinger anyway
[09:15] <rogpeppe1> dimitern: yeah, in which case the normal timeout will apply
[09:15] <rogpeppe1> dimitern: it will start a new pinger which will take over from the previous one, and noone will know
[09:16] <dimitern> rogpeppe1: that's very subtle logic
[09:16] <rogpeppe1> dimitern: that was part of the design
[09:16] <rogpeppe1> dimitern: well, not the "subtle" bit :-)
[09:17] <dimitern> rogpeppe1: so what then - sP.Stop() calls P.Kill and have sP.Pause() ?
[09:17] <dimitern> rogpeppe1: calling P.Stop()
[09:18]  * rogpeppe1 inspects the logic inside presence
[09:20] <dimitern> rogpeppe1: or just don't complicate things and have sP.Kill() -> P.Kill() and sP.Stop() -> P.Stop()
[09:20] <dimitern> rogpeppe1: ?
[09:20] <rogpeppe1> dimitern: yes, but a bit more too
[09:21] <dimitern> rogpeppe1: please explain
[09:21] <rogpeppe1> dimitern: i think you want: type pinger struct {p *presence.Pinger}; and pinger.Stop() -> pinger.p.Kill
[09:22] <dimitern> rogpeppe1: by "pinger" you mean srvPinger or it's a separate type?
[09:22] <kyhwana> hrm, going through the juju getting started at https://juju.ubuntu.com/docs/getting-started.html in 12.04, trying to use lxc, in "In ~/.juju/environments.yaml, add a section for type "local":" I get error: environment "sample" has an unknown provider type "local" when I run juju bootstrap
[09:22] <rogpeppe1> dimitern: it's a separate type
[09:22] <dimitern> kyhwana: Go juju (juju-core) does not yet support the local provider
[09:23] <dimitern> rogpeppe1: ok, I'm confused now.. hangout?
[09:23] <rogpeppe1> dimitern: sure
[09:23] <kyhwana> dimitern: what. Why does the getting-started then have instructions for LXC/local?
[09:23] <dimitern> rogpeppe1: I'll start one this time
[09:24] <dimitern> kyhwana: it's about Python juju mostly, if you want to use the local provider you should stick with Py juju for now
[09:25] <dimitern> rogpeppe1: https://plus.google.com/hangouts/_/9849418108b0af37525e7611bc4b9e20837c241d?authuser=0&hl=en
[09:26] <kyhwana> dimitern: whats the difference?
[09:31] <dimitern> kyhwana: juju-core is actively developed, while py-juju is just maintained
[09:32] <kyhwana> ahhh
[09:33] <kyhwana> so I should try it in 13.04?
[09:46] <kyhwana> right, works in 13.04
[09:46] <kyhwana> I know it says "12.04" is in beta, but maybe the getting started page should say "requires 13.04"
[10:03] <dimitern> kyhwana: so in 13.04 juju-core is released for the first time, but it does not implement all the features of py-juju; it implements more things as well and generally is the way forward for juju. In 13.10 there is a lot more stuff planned (including the local provider among other things)
[10:04] <kyhwana> hmm
[10:19] <dimitern> rogpeppe1: all done on the server side i think, now I have a question about SetAgentAlive -> *Pinger, error
[10:19] <dimitern> rogpeppe1: (client-side)
[10:19] <rogpeppe1> dimitern: ok
[10:19] <dimitern> rogpeppe1: I need both to call srvMachine.SetAgentAlive and start a pinger client-side, right?
[10:20] <rogpeppe1> dimitern: you just need to call SetAgentAlive
[10:20] <rogpeppe1> dimitern: and store the pinger id in the *Pinger that you return from tha
[10:20] <rogpeppe1> t
[10:20] <dimitern> rogpeppe1: but how about the  loop stuff and Pinger at client-side?
[10:21] <rogpeppe1> dimitern: it's all done server side
[10:21] <rogpeppe1> dimitern: no loop necessary
[10:21] <dimitern> rogpeppe1: so why EntityWatcher defines a loop at client-side then?
[10:21] <rogpeppe1> dimitern: because it needs to turn the API calls into channel sends
[10:22] <rogpeppe1> dimitern: there's no need for that with a pinger
[10:22] <dimitern> rogpeppe1: ah, right; but I still need a Pinger type at client side
[10:22] <rogpeppe1> dimitern: yup
[10:22] <rogpeppe1> dimitern: type Pinger {id string}
[10:22] <dimitern> rogpeppe1: which does what?
[10:22] <rogpeppe1> dimitern: it'll just define a Stop method
[10:23] <rogpeppe1> dimitern: actually, type Pinger {st *State; id string}
[10:23] <dimitern> rogpeppe1: I see
[10:23] <rogpeppe1> dimitern: then func (p *Pinger) Stop() error {return p.st.call("Pinger", p.id, "Stop", nil)}
[10:23] <rogpeppe1> or something like that
[10:23] <dimitern> rogpeppe1: cool :)
[10:27] <dimitern> rogpeppe1: how about the result of the call to SetAgentAlive? It returns params.PingerId and error
[10:28] <dimitern> rogpeppe1: do I need something like struct result {id params.PingerId, err error}?
[10:28] <rogpeppe1> dimitern: no
[10:29] <rogpeppe1> dimitern: just pass a PingerId to the result paramter of call
[10:29] <dimitern> rogpeppe1: and the error is returned by m.st.Call ?
[10:29] <rogpeppe1> dimitern: yup
[10:32] <dimitern> rogpeppe1: and for methods w/o arguments and an error result, like EnsureDead? -> return m.st.call("Machine", m.id, "EnsureDead", nil, nil)
[10:33] <rogpeppe1> dimitern: yup
[10:33] <dimitern> rogpeppe1: i like  it already :)
[10:33] <rogpeppe1> dimitern: cool
[10:35] <dimitern> rogpeppe1: and Life? there's no error result, just params.Life -> m.st.call("Machine", m.id, "Life", nil, &life); return life?
[10:36] <rogpeppe1> dimitern: every API call *must* return a struct
[10:36] <rogpeppe1> dimitern: so you'll need a struct with a single Life member
[10:36] <dimitern> rogpeppe1: ah, ok
[10:37] <dimitern> rogpeppe1: does it have to be a struct with named fields? will type Life struct { string } work?
[10:37] <rogpeppe1> dimitern: it should be with named fields, yes
[10:38] <rogpeppe1> dimitern: that's kinda the point
[10:38] <dimitern> rogpeppe1: so Life { Life string } then
[10:38] <rogpeppe1> dimitern: it means that we can add fields later without breaking the API
[10:38] <rogpeppe1> dimitern: yeah, that sounds reasonable.
[10:38] <rogpeppe1> dimitern: although...
[10:38] <rogpeppe1> dimitern: i'm wondering if we need to define "type Life string" in params
[10:39] <rogpeppe1> dimitern: so that ServiceInfo can use it
[10:39] <rogpeppe1> dimitern: and that means we need to think of another name...
[10:39] <dimitern> rogpeppe1: I did that initially, but now I have: http://paste.ubuntu.com/5686558/
[10:40] <rogpeppe1> dimitern: so no state.Life type?
[10:40] <dimitern> rogpeppe1: well state.Life is an int8
[10:40] <rogpeppe1> dimitern: yes, but lots of things in the code use state.Life, don't they?
[10:40] <rogpeppe1> dimitern: (or maybe they don't actually...)
[10:40] <dimitern> rogpeppe1: all of them most probably
[10:41] <dimitern> rogpeppe1: Life.String() is used in commands only I think and logs
[10:42] <dimitern> rogpeppe1: so then I should have params.LifeResults with Life state.Life? can I do that in params?
[10:42] <rogpeppe1> dimitern: hmm, i wouldn't mind just having life as an untypes string
[10:42] <rogpeppe1> untyped
[10:42] <rogpeppe1> dimitern: but others may well disagree
[10:42] <rogpeppe1> dimitern: no
[10:43] <dimitern> rogpeppe1: yeah, thought so
[10:43] <rogpeppe1> dimitern: you'd need to define params.Life type
[10:43] <rogpeppe1> dimitern: e.g. (in params) type Life string
[10:43] <dimitern> rogpeppe1: so why not have type Life int8 in params and use that, so there's no fishy string conversions?
[10:44] <rogpeppe1> dimitern: i'm not sure i want those constant values to be hardcoded in the javascript
[10:44] <dimitern> rogpeppe1: but how about the agents?
[10:44] <rogpeppe1> dimitern: i think we should choose a single representation across the whole API
[10:45] <dimitern> rogpeppe1: these values has to be defined somewhere if you need to use them - strings or ints
[10:45] <rogpeppe1> dimitern: sure.
[10:45] <rogpeppe1> dimitern: i'd do (in params) type Life string
[10:45] <dimitern> rogpeppe1: but I agree numbers in js are not real numbers, so strings is better
[10:45] <rogpeppe1> dimitern: and then 'type LifeResult struct { Life Life }'
[10:46] <dimitern> rogpeppe1: ok (and let the bikeshedding begin during the review :)
[10:46] <rogpeppe1> dimitern: s/Result/Results/
[10:46] <rogpeppe1> dimitern: i think that's relatively uncontroversial actually
[10:47] <dimitern> rogpeppe1: you never know :)
[10:47] <rogpeppe1> dimitern: v true :-)
[10:48] <dimitern> rogpeppe1: so any API call can return: (1) an error, (2) struct, (3) both - and these are all the cases
[10:49] <rogpeppe1> dimitern: yeah - read the rpc documentation!
[10:49] <rogpeppe1> dimitern: it's all there
[10:50] <dimitern> rogpeppe1: sorry for bugging you so much, but it's easier for me to understand things as I go (in context), then the docs will be more useful (as a reminder)
[10:51] <rogpeppe1> dimitern: np. it is worth reading (and trying to understand) the docs at least once though, to start with
[10:51] <dimitern> rogpeppe1: I agree, my bad
[10:51] <rogpeppe1> dimitern: and if you find them incomprehensible, i want to know so i can improve them.
[10:52] <dimitern> rogpeppe1: sure
[10:54] <dimitern> rogpeppe1: how about doc comments - do I need to copy the state method's comment for both client and server methods?
[10:55] <rogpeppe1> dimitern: i'd copy existing convention. definitely copy doc comments for the client methods, but not necessarily for the server methods
[10:55] <dimitern> rogpeppe1: or the client one is more important and the server one can be shorter (e.g. EnsureDead is several lines long)
[10:55] <rogpeppe1> dimitern: but... we might want that to change actually
[10:56] <dimitern> rogpeppe1: right
[10:56] <rogpeppe1> dimitern: if we start auto-generating docs for the API
[10:56] <rogpeppe1> dimitern: but for the time being, i'd go with that
[11:19] <dimitern> rogpeppe1: why in the permission tests allow specifies both machine-0 and machine-1 when (e.g. SetPassword) only affects machine 1?
[11:19] <rogpeppe1> dimitern: i'm not sure i understand the question
[11:19] <dimitern> rogpeppe1: should i follow this pattern?
[11:20] <dimitern> rogpeppe1: opMachine1SetPassword acts on machine 1 only, not both 0 and 1
[11:20]  * rogpeppe1 goes to have a look
[11:21] <dimitern> rogpeppe1: I think the machine-0 in allow is not needed
[11:22] <rogpeppe1> dimitern: ah, it's correct but there should be a comment
[11:22] <dimitern> rogpeppe1: explain please?
[11:22] <rogpeppe1> dimitern: machine-0 is allowed to set the password because it's an environment manager
[11:23] <dimitern> rogpeppe1: but for all other cases only the machine we're acting upon has to be specified in allow, right?
[11:23] <rogpeppe1> dimitern: yes.
[11:23] <dimitern> rogpeppe1: I'll add a comment about opMachine1SetPassword in the table then
[11:23] <rogpeppe1> dimitern: sounds good
[11:27] <dimitern> rogpeppe1: so the func() that's being returned resets whatever the operation did?
[11:27] <rogpeppe1> dimitern: yes
[11:27] <rogpeppe1> dimitern: hmm
[11:27] <rogpeppe1> dimitern: that may mean you need to provide a Kill operation
[11:27] <dimitern> rogpeppe1: fortunately for EnsureDead it's a no-op, because it won't affect the machine which is alive
[11:28] <dimitern> rogpeppe1: oh, for SSA() yeah..
[11:28] <rogpeppe1> dimitern: SAA?
[11:28] <dimitern> rogpeppe1: or just call pinger.Stop?
[11:28] <dimitern> rogpeppe1: SetAgentAlive
[11:28] <rogpeppe1> dimitern: actually, that should be fine
[11:28] <dimitern> rogpeppe1: ok then
[11:28] <rogpeppe1> dimitern: it doesn't matter if we're still notionally alive
[11:29] <jam> dimitern, mgz, wallyworld_, danilos: greetings all
[11:29] <wallyworld_> hello
[11:29] <dimitern> jam: hey, just staring mumble now
[11:32] <jam> danilos: are you there? We see you in mumble but you are deafened and muted
[11:58] <dimitern> rogpeppe1: so wrt permissions should all these machine methods be accessible only by the machine's own agent: SetStatus, Life, EnsureDead, SetAgentAlive?
[11:58] <dimitern> rogpeppe1: not sure about Life
[11:58] <rogpeppe1> dimitern: not Life, but the others, yes
[11:59] <rogpeppe1> dimitern: one other thing
[11:59] <rogpeppe1> dimitern: the environ manager needs to be able to call SetStatus
[11:59] <dimitern> rogpeppe1: why?
[11:59] <rogpeppe1> dimitern: what happens when the provisioner can't provision a machine?
[11:59] <dimitern> rogpeppe1: it tries again
[12:00] <rogpeppe1> dimitern: forever?
[12:01] <dimitern> rogpeppe1: let me check
[12:01] <rogpeppe1> dimitern: we need to think about what happens if we can't bring up a machine. i'm thinking that the environ manager probably needs to be able to call EnsureDead in that case actually.
[12:02] <dimitern> rogpeppe1: actually if the PA fails to start a machine it sets it to an error state and will skip it next time
[12:02] <rogpeppe1> dimitern: that's what i thought
[12:03] <rogpeppe1> dimitern: so it calls SetStatus, right?
[12:03] <dimitern> rogpeppe1: ah, I see your point
[12:03] <dimitern> rogpeppe1: yeah, m0 needs to SetStatus
[12:05] <dimitern> rogpeppe1: and how about Life() - what should be in allow and deny?
[12:06] <rogpeppe1> dimitern: Life shouldn't be in the perms checks
[12:06] <rogpeppe1> dimitern: sorry, i just remembered :-)
[12:06] <rogpeppe1> dimitern: it's just a field in params.Machine
[12:06] <rogpeppe1> dimitern: not a separate API call at all
[12:06] <dimitern> rogpeppe1: hmm..
[12:06] <dimitern> rogpeppe1: like InstanceId ?
[12:07] <rogpeppe1> dimitern: exactly like that
[12:07] <dimitern> rogpeppe1: ok, I was wondering the same actually
[12:07] <rogpeppe1> dimitern: sorry for not catching that earlier
[12:07] <dimitern> rogpeppe1: np
[12:07] <rogpeppe1> dimitern: we did mention it yesterday though
[12:11] <danilos> jam, dimitern, mgz, wallyworld_: hey, sorry, got too focused on coding :/
[12:11] <jam> danilos: how's it going?
[12:11] <danilos> not that that's necessarily a bad thing, just forgot about the meeting
[12:12] <danilos> jam, it's pretty good, thanks
[12:13] <jam> danilos: finishing up the bootstrap stuff?
[12:13] <danilos> jam, yeah
[12:20] <dimitern> rogpeppe1: so no srvMachine.Life(), only client-side
[12:20] <rogpeppe1> dimitern: yup
[12:21] <rogpeppe1> dimitern: and some tests to check that refresh refreshes the life, etc
[12:22] <dimitern> rogpeppe1: it'll be difficult to change the lifecycle with the current scenario
[12:22] <rogpeppe1> dimitern: how do you mean?
[12:22] <dimitern> rogpeppe1: all machines have assigned units, so cannot be removed or set to dying
[12:23] <rogpeppe1> dimitern: isn't that the same as it is now?
[12:23] <rogpeppe1> dimitern: you must remove the units, then the machine
[12:23] <dimitern> rogpeppe1: ah, ok
[12:23] <dimitern> rogpeppe1: and then restore the scenario
[12:24] <rogpeppe1> dimitern: oh, i see, when testing
[12:24] <rogpeppe1> dimitern: don't bother
[12:24] <dimitern> rogpeppe1: wait - are we talking about perm checks or individual api calls tests?
[12:24] <rogpeppe1> dimitern: the perms checks
[12:24] <rogpeppe1> dimitern: just try to change the lifecycle and check you get the right kind of error
[12:25] <dimitern> rogpeppe1: yeah - i could add a hanging machine with no units that can change life
[12:25] <rogpeppe1> dimitern: don't even bother to do that
[12:25] <dimitern> rogpeppe1: sgtm
[12:25] <rogpeppe1> dimitern: just try to change the life of one of the machines
[12:25] <rogpeppe1> dimitern: you should get a "machine has units" error
[12:25] <rogpeppe1> dimitern: if you don't, there's a problem
[12:25] <dimitern> rogpeppe1: that's exactly what i did :)
[12:26] <rogpeppe1> dimitern: but for the individual API test, you should create a machine and test that it actually works
[12:26] <dimitern> rogpeppe1: sure
[12:40] <dimitern> rogpeppe1: does this look ok to you? http://paste.ubuntu.com/5686802/
[12:41] <dimitern> rogpeppe1: because i'm getting permission denied
[12:42] <rogpeppe1> dimitern: i think i'd implement isOwnAgent on srvRoot, not srvMachine - it's quite generic
[12:42] <rogpeppe1> dimitern: (not that that effects the correctness of the code)
[12:42] <dimitern> rogpeppe1: i though about that
[12:42] <dimitern> rogpeppe1: but otherwise?
[12:44] <rogpeppe1> dimitern: what's the actual output from your test
[12:44] <rogpeppe1> dimitern: (yes, i do think i see a potential problem)
[12:45] <dimitern> rogpeppe1: http://paste.ubuntu.com/5686827/ (disregard the messed up Life value - fixed that)
[12:45] <rogpeppe1> dimitern: look at opClientResolved
[12:45] <rogpeppe1> dimitern: you've got a spelling error in your error message for a start
[12:46] <rogpeppe1> dimitern: and if you get a permission-denied error, you should return it
[12:46] <rogpeppe1> dimitern: on line 29, you can just do c.Assert(err, IsNil)
[12:46] <dimitern> rogpeppe1: so I should expect api.CodeUnauthorized at first (Machine("1")) ?
[12:47] <rogpeppe1> dimitern: hmm, no you can't
[12:47] <rogpeppe1> dimitern: hmm, interesting
[12:47] <dimitern> rogpeppe1: I was wondering why about that - why check + return empty func instead of assert
[12:49] <rogpeppe1> dimitern: because the actual permission-denied error will happen when trying to get the machine, not on the later call
[12:49] <dimitern> rogpeppe1: ah..
[12:50] <dimitern> rogpeppe1: i'll poke it a bit more
[12:50] <rogpeppe1> dimitern: i think it (and opMachine1SetPassword) could use some improvement actually
[12:51] <rogpeppe1> dimitern: they're not actually testing the right thing.
[12:51] <dimitern> rogpeppe1: but first - if I move isOwnAgent into srvRoot, how can I get the m.m.Tag() there?
[12:52] <rogpeppe1> dimitern: maybe call it m.root.loggedInAs(m.Tag())
[12:53] <dimitern> rogpeppe1: there's no m anymore
[12:53] <dimitern> rogpeppe1: I have r *srvRoot only
[12:54] <rogpeppe1> dimitern: i mean in EnsureDead, you'd call m.root.loggedInAs(m.Tag()) rather than m.isOwnAgent()
[12:54] <dimitern> rogpeppe1:  i see.. would've liked to shorten this somehow
[12:55] <dimitern> rogpeppe1: because I have also isOwnAgentOrManager - extracted the check from setpassword
[12:56] <dimitern> rogpeppe1: maybe I can make it loggedInAs(tag, job) - and job can be nil
[12:56] <rogpeppe1> dimitern: hmm, no i don't think that's quite right.
[12:57]  * rogpeppe1 thinks
[12:57] <dimitern> rogpeppe1: that's what I need, but the name might be better
[12:58] <dimitern> *could b* better
[12:59] <dimitern> rogpeppe1: hasPermission(tag, *job) ?
[12:59] <rogpeppe1> dimitern: i'm trying to think of a good name too
[13:00] <rogpeppe1> dimitern: this is going to be used a lot, so it's worth coming up with something that works well
[13:00] <dimitern> rogpeppe1: canAccess? are you ok with the *job arg?
[13:00] <rogpeppe1> dimitern: i'm not sure. i think i'd prefer to keep the two checks separate.
[13:01] <dimitern> rogpeppe1: we can have multiple: canAccess(tag), canManage(tag, job) ?
[13:02] <dimitern> or just canManage and hard code JobManageEnviron check inside
[13:02] <rogpeppe1> dimitern: i don't find the meanings of those verbs very obvious
[13:02] <dimitern> rogpeppe1: why?
[13:02] <rogpeppe1> dimitern: what does canAccess mean? who can access what?
[13:03] <dimitern> rogpeppe1: we're asking "can <user> access <tag>"?
[13:03] <rogpeppe1> dimitern: the answer to that depends on the operation, no?
[13:03] <dimitern> rogpeppe1: not really - it's ancilliary to the operation
[13:04] <rogpeppe1> dimitern: the verb seems to imply that it's universal, to me at any rate.
[13:04] <dimitern> rogpeppe1: I mean the operation itself has to define what is done
[13:04] <dimitern> rogpeppe1: that's why I started with isX
[13:05] <dimitern> rogpeppe1: userCanAccess? :)
[13:06] <dimitern> rogpeppe1: it has to be not to long as well
[13:06] <rogpeppe1> dimitern: personally i think readability is probably more important than length here.
[13:07] <dimitern> rogpeppe1: ok, let's dice it step by step - 1) get logged in user's tag, 2) check it matches the passed entity's tag
[13:08] <dimitern> rogpeppe1: userMatches(x) -> e.Tag() == x.Tag()
[13:08] <rogpeppe1> dimitern: how about "isOwner" as a name
[13:09] <rogpeppe1> dimitern: as in m.root.isOwner(m.m)
[13:09] <rogpeppe1> hmm, not quite right too
[13:09] <dimitern> rogpeppe1: sgtm
[13:10] <dimitern> rogpeppe1: we're coming back to isOwnAgent :)
[13:11] <dimitern> "agent" here in a more general sense (s/o acting on behalf of)
[13:12] <rogpeppe1> dimitern: yeah, i didn't really have a problem with isOwnAgent except the name and the fact we'd need to duplicate similar code for other entities.
[13:12] <rogpeppe1> dimitern: although, come to think of it, probably only srvUnit
[13:13] <dimitern> rogpeppe1: no, I mean have isOwnAgent in srvRoot
[13:14] <dimitern> rogpeppe1: actually you're right - we don't need other perm checks like that (what other agents are there that "own" entities?)
[13:14] <dimitern> units and machines only
[13:14] <rogpeppe1> dimitern: yeah
[13:15] <rogpeppe1> dimitern: how about this? http://paste.ubuntu.com/5686906/
[13:15] <rogpeppe1> dimitern: then we can use the general Auth suffix to apply to various kinds of authorization primitive
[13:16] <rogpeppe1> dimitern: where ownerAuth is your isOwnAgent by another name
[13:16] <dimitern> rogpeppe1: i like it - we can have envrionManagerAuth on srvRoot actually
[13:16] <rogpeppe1> dimitern: ah yes, good point
[13:16] <dimitern> rogpeppe1: sgtm, will make it so
[13:16] <rogpeppe1> dimitern: cool, thanks for bearing with me
[13:17] <rogpeppe1> dimitern: hmm, maybe "auth" would be better as a prefix rather than a suffix
[13:17] <rogpeppe1> dimitern: i can't quite decide
[13:18] <dimitern> rogpeppe1: i thought the same
[13:18] <rogpeppe1> dimitern: in which case, let's do that
[13:18] <dimitern> rogpeppe1: let's make it a prefix and also - how about putting authOwner on srvRoot getting x with .Tag()?
[13:19] <dimitern> rogpeppe1: so if !m.authOwner(m.m) && !m.authEnvironManager() { return errPerm }
[13:19] <rogpeppe1> dimitern: sgtm
[13:19] <dimitern> rogpeppe1: cool
[13:31] <dimitern> rogpeppe1: what did you say can be improved about opMachine1SetPassword?
[14:03] <dimitern> rogpeppe2: kanban
[14:41] <dimitern> rogpeppe2: I cannot test SetAgentAlive properly without having AgentAlive() as well
[14:41] <dimitern> rogpeppe2: and using the state machine's AgentAlive won't work because the pingers are different
[14:43] <dimitern> rogpeppe2: but I can just call it and see it returns no error and calling stop on the returned pinger is also successful
[14:44] <rogpeppe2> dimitern: for the permissions check or the actual test?
[14:44] <dimitern> rogpeppe2: actual test
[14:44] <dimitern> rogpeppe2: permissions test fine
[14:44] <rogpeppe2> dimitern: why can't you use AgentAlive on the state Machine ?
[14:45] <dimitern> rogpeppe2: it returns false after api.SetAgentAlive
[14:45] <rogpeppe2> dimitern: really? that seems a bit weird.
[14:45] <dimitern> rogpeppe2: hmm.. maybe something's wrong with my pinger code
[14:47] <rogpeppe2> dimitern: ah, you probably need to call state.Sync
[14:47] <dimitern> rogpeppe2: oh, forgot about that :)
[15:11] <dimitern> rogpeppe2: all done and all tests pass!  https://codereview.appspot.com/9614043
[15:12] <rogpeppe2> dimitern: woo!
[15:13] <dimitern> rogpeppe2: scrutiny appreciated :)
[15:13] <rogpeppe2> dimitern: looking now
[15:37] <rogpeppe2> dimitern: reviewed
[15:37] <dimitern> rogpeppe2: tyvm
[15:40] <dimitern> rogpeppe2: the issue with setstatus is you cannot change it back to pending once it's advanced
[15:40] <dimitern> rogpeppe2: that's why I added default status and changed the scenario
[15:41] <rogpeppe2> dimitern: ah
[15:41] <rogpeppe2> dimitern: so you have to set the correct status
[15:42] <dimitern> rogpeppe2: what do you mean?
[15:42]  * rogpeppe2 thinks we do make life hard for ourselves sometimes. more code breeds more code.
[15:43] <rogpeppe2> dimitern: ah, i see, it's just pending you can't rewind
[15:43] <dimitern> rogpeppe2: yes
[15:44] <dimitern> rogpeppe2: and anyways, i think the scenario should have all machines in started status
[15:44] <TheRealMue> dimitern: another review
[15:44] <rogpeppe2> dimitern: you may well be right.
[15:44] <dimitern> TheRealMue: cheers
[15:45] <dimitern> TheRealMue: I feel the same about the 1 or 2 char vars throughout, but changing that is out of scope I think
[15:47]  * dimitern thinks now once I picked up speed I can ravage through the next agent's needed API calls easily (PA I think) :)
[15:52] <TheRealMue> dimitern: But otherwise it never will be changed. ;)
[15:55] <dimitern> TheRealMue: not quite - can be changed independently as a whole
[15:56] <dimitern> rogpeppe2: HasAssignedUnitsError is a tricky one - what should the CodeHasAssignedUnits be? it's not a const
[15:56] <rogpeppe2> dimitern: it should be a const like the others
[15:57] <dimitern> rogpeppe2: it can be a format string, but that will duplicate the logic in state
[15:57] <rogpeppe> dimitern: i'm not sure i see the problem.
[15:57] <rogpeppe> dimitern: why can't it just be a const code like the other codes?
[15:57] <dimitern> rogpeppe: func (e *HasAssignedUnitsError) Error() string { return fmt.Sprintf("machine %s has unit %q assigned", e.MachineId, e.UnitNames[0]) }
[15:58] <rogpeppe> dimitern: the message is separate from the code
[15:58] <dimitern> rogpeppe: ah, ok
[15:58] <rogpeppe> dimitern: take a look at the serverError implementation
[16:31] <dimitern> rogpeppe: ok, all done, will repropose soon and I'd appreciate one last look (running tests now)
[16:31] <rogpeppe> dimitern: ok
[16:41] <dimitern> rogpeppe: https://codereview.appspot.com/9614043
[16:52] <rogpeppe> dimitern: reviewed
[16:53] <dimitern> rogpeppe: thanks
[16:56] <dimitern> rogpeppe: I don't get what's wrong with the error code check in opMachine1EnsureDead - do you mean to move the error check into a test similar to TestMachineEnsureDead instead?
[16:56] <rogpeppe> dimitern: yes. the op checks are only for sanity - they should not be used to test specific functionality
[16:58] <dimitern> rogpeppe: ok
[16:58] <dimitern> rogpeppe: will do that and land it then
[17:00] <dimitern> rogpeppe: ah, perhaps you meant add it to errorTransformTests ?
[17:01] <dimitern> rogpeppe: or that as well as the separate test case for EnsureDead?
[17:02] <mgz> dimitern: poke me if any of my review comments are unclear or just silly
[17:03] <dimitern> mgz: already replied :)
[17:04] <Makyo> Anyone up for a review of https://codereview.appspot.com/9566045/ - refactor deploy to statecmd?
[17:06] <mgz> dimitern: you can carry forward my lgtm
[17:06] <dimitern> mgz: cheers
[17:12] <mgz> Makyo: done, poke rog or dimitern as well though
[17:13] <Makyo> mgz, cool, thanks.  Will poke around the args, too, see what can be reduced.
[17:22]  * rogpeppe has got to go. g'night all
[17:22] <rogpeppe> dimitern: yeah, it should be in errorTransformTests
[17:23] <rogpeppe> dimitern: which probably means you don't need a separate test for the error return
[17:23] <dimitern> rogpeppe: well I did both
[17:23] <dimitern> rogpeppe: they're different anyway
[17:23] <dimitern> rogpeppe: one tests the flow and the other changes to serverError
[17:27] <dimitern> ok, i'm off - mass effect is calling for me ;)
[17:28] <dimitern> g'night all
[21:30] <thumper> morning mramm
[21:30] <mramm> morning
[21:45] <thumper> morning wallyworld_
[21:45] <wallyworld_> g'day
[23:58] <wallyworld_> thumper: in your branch, i can't see where DescriptionTimeout is used to kill the process