| * thumper loos for wallyworld_ | 01:39 | |
| thumper | s/loos/looks/ | 01:39 |
|---|---|---|
| * thumper sighs | 01:39 | |
| wallyworld_ | hello | 01:39 |
| thumper | wallyworld_: s'up? | 01:40 |
| wallyworld_ | finishing some work to make juju easier to use | 01:41 |
| wallyworld_ | using simplestreams metadata, public bucket url in keystone catalog etc | 01:41 |
| 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:42 |
| wallyworld_ | did you go away anywhre | 01:43 |
| wallyworld_ | apparently me, you and frank are on containers | 01:43 |
| thumper | wallyworld_: no, stayed at home and hacked on my personal project | 01:44 |
| thumper | made good progress | 01:44 |
| marcoceppi | Makyo: did you need help with anything in particular? | 02:32 |
| marcoceppi | err, kyhwana ^^ | 02:32 |
| 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:33 |
| 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:34 |
| marcoceppi | cheers o/ | 02:35 |
| === tasdomas_afk is now known as tasdomas | ||
| dimitern | g'morning | 07:05 |
| === liam_ is now known as Guest39447 | ||
| rogpeppe1 | dimitern: hiya | 08:37 |
| dimitern | rogpeppe1: heyhey | 08:37 |
| dimitern | rogpeppe1: still struggling with (srv)Pinger stuff | 08:37 |
| 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:38 |
| 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:46 |
| 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:47 |
| dimitern | rogpeppe1: it looks like it's just an empty struct rather than wrapping a pointer | 08:48 |
| 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:49 |
| 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:50 |
| 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:51 |
| 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 | 08:52 |
| 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:01 |
| 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:03 |
| 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:04 |
| 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:05 |
| dimitern | rogpeppe1: seems right | 09:06 |
| 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:07 |
| 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:08 |
| 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:09 |
| 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:10 |
| 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:11 |
| 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:12 |
| dimitern | rogpeppe1: hmm | 09:13 |
| rogpeppe1 | dimitern: but actually... | 09:13 |
| dimitern | rogpeppe1: when upgrading we need the pinger stopped but alive somehow? | 09:13 |
| 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:14 |
| 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:15 |
| 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:16 |
| dimitern | rogpeppe1: so what then - sP.Stop() calls P.Kill and have sP.Pause() ? | 09:17 |
| dimitern | rogpeppe1: calling P.Stop() | 09:17 |
| * rogpeppe1 inspects the logic inside presence | 09:18 | |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| 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:24 |
| dimitern | rogpeppe1: https://plus.google.com/hangouts/_/9849418108b0af37525e7611bc4b9e20837c241d?authuser=0&hl=en | 09:25 |
| kyhwana | dimitern: whats the difference? | 09:26 |
| dimitern | kyhwana: juju-core is actively developed, while py-juju is just maintained | 09:31 |
| kyhwana | ahhh | 09:32 |
| kyhwana | so I should try it in 13.04? | 09:33 |
| 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" | 09:46 |
| 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:03 |
| kyhwana | hmm | 10:04 |
| 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:19 |
| 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:20 |
| 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:21 |
| 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:22 |
| 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:23 |
| dimitern | rogpeppe1: how about the result of the call to SetAgentAlive? It returns params.PingerId and error | 10:27 |
| dimitern | rogpeppe1: do I need something like struct result {id params.PingerId, err error}? | 10:28 |
| rogpeppe1 | dimitern: no | 10:28 |
| 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:29 |
| 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:32 |
| rogpeppe1 | dimitern: yup | 10:33 |
| dimitern | rogpeppe1: i like it already :) | 10:33 |
| rogpeppe1 | dimitern: cool | 10:33 |
| dimitern | rogpeppe1: and Life? there's no error result, just params.Life -> m.st.call("Machine", m.id, "Life", nil, &life); return life? | 10:35 |
| 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:36 |
| 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:37 |
| 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:38 |
| 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:39 |
| 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:40 |
| dimitern | rogpeppe1: Life.String() is used in commands only I think and logs | 10:41 |
| 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:42 |
| 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:43 |
| 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:44 |
| 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:45 |
| 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:46 |
| dimitern | rogpeppe1: you never know :) | 10:47 |
| rogpeppe1 | dimitern: v true :-) | 10:47 |
| dimitern | rogpeppe1: so any API call can return: (1) an error, (2) struct, (3) both - and these are all the cases | 10:48 |
| rogpeppe1 | dimitern: yeah - read the rpc documentation! | 10:49 |
| rogpeppe1 | dimitern: it's all there | 10:49 |
| 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:50 |
| 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:51 |
| dimitern | rogpeppe1: sure | 10:52 |
| dimitern | rogpeppe1: how about doc comments - do I need to copy the state method's comment for both client and server methods? | 10:54 |
| 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:55 |
| 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 | 10:56 |
| 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:19 |
| dimitern | rogpeppe1: opMachine1SetPassword acts on machine 1 only, not both 0 and 1 | 11:20 |
| * rogpeppe1 goes to have a look | 11:20 | |
| dimitern | rogpeppe1: I think the machine-0 in allow is not needed | 11:21 |
| 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:22 |
| 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:23 |
| 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:27 |
| 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:28 |
| jam | dimitern, mgz, wallyworld_, danilos: greetings all | 11:29 |
| wallyworld_ | hello | 11:29 |
| dimitern | jam: hey, just staring mumble now | 11:29 |
| jam | danilos: are you there? We see you in mumble but you are deafened and muted | 11:32 |
| 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:58 |
| 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 | 11:59 |
| rogpeppe1 | dimitern: forever? | 12:00 |
| 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:01 |
| 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:02 |
| 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:03 |
| dimitern | rogpeppe1: and how about Life() - what should be in allow and deny? | 12:05 |
| 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:06 |
| 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:07 |
| 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:11 |
| danilos | jam, it's pretty good, thanks | 12:12 |
| === gary_poster is now known as gary_poster|away | ||
| jam | danilos: finishing up the bootstrap stuff? | 12:13 |
| danilos | jam, yeah | 12:13 |
| === gary_poster|away is now known as gary_poster | ||
| dimitern | rogpeppe1: so no srvMachine.Life(), only client-side | 12:20 |
| rogpeppe1 | dimitern: yup | 12:20 |
| rogpeppe1 | dimitern: and some tests to check that refresh refreshes the life, etc | 12:21 |
| 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:22 |
| 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 |
| === tasdomas is now known as tasdomas_afk | ||
| dimitern | rogpeppe1: and then restore the scenario | 12:23 |
| === tasdomas_afk is now known as tasdomas | ||
| 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:24 |
| 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:25 |
| 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:26 |
| dimitern | rogpeppe1: does this look ok to you? http://paste.ubuntu.com/5686802/ | 12:40 |
| dimitern | rogpeppe1: because i'm getting permission denied | 12:41 |
| 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:42 |
| rogpeppe1 | dimitern: what's the actual output from your test | 12:44 |
| rogpeppe1 | dimitern: (yes, i do think i see a potential problem) | 12:44 |
| 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:45 |
| 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:46 |
| 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:47 |
| 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:49 |
| dimitern | rogpeppe1: i'll poke it a bit more | 12:50 |
| rogpeppe1 | dimitern: i think it (and opMachine1SetPassword) could use some improvement actually | 12:50 |
| 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:51 |
| rogpeppe1 | dimitern: maybe call it m.root.loggedInAs(m.Tag()) | 12:52 |
| dimitern | rogpeppe1: there's no m anymore | 12:53 |
| dimitern | rogpeppe1: I have r *srvRoot only | 12:53 |
| 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:54 |
| dimitern | rogpeppe1: because I have also isOwnAgentOrManager - extracted the check from setpassword | 12:55 |
| 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:56 |
| * rogpeppe1 thinks | 12:57 | |
| dimitern | rogpeppe1: that's what I need, but the name might be better | 12:57 |
| dimitern | *could b* better | 12:58 |
| dimitern | rogpeppe1: hasPermission(tag, *job) ? | 12:59 |
| rogpeppe1 | dimitern: i'm trying to think of a good name too | 12:59 |
| 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:00 |
| dimitern | rogpeppe1: we can have multiple: canAccess(tag), canManage(tag, job) ? | 13:01 |
| 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:02 |
| 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:03 |
| 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:04 |
| dimitern | rogpeppe1: userCanAccess? :) | 13:05 |
| 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:06 |
| 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:07 |
| dimitern | rogpeppe1: userMatches(x) -> e.Tag() == x.Tag() | 13:08 |
| rogpeppe1 | dimitern: how about "isOwner" as a name | 13:08 |
| rogpeppe1 | dimitern: as in m.root.isOwner(m.m) | 13:09 |
| rogpeppe1 | hmm, not quite right too | 13:09 |
| dimitern | rogpeppe1: sgtm | 13:09 |
| dimitern | rogpeppe1: we're coming back to isOwnAgent :) | 13:10 |
| dimitern | "agent" here in a more general sense (s/o acting on behalf of) | 13:11 |
| 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:12 |
| dimitern | rogpeppe1: no, I mean have isOwnAgent in srvRoot | 13:13 |
| 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:14 |
| 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:15 |
| 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:16 |
| 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:17 |
| 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:18 |
| dimitern | rogpeppe1: so if !m.authOwner(m.m) && !m.authEnvironManager() { return errPerm } | 13:19 |
| rogpeppe1 | dimitern: sgtm | 13:19 |
| dimitern | rogpeppe1: cool | 13:19 |
| dimitern | rogpeppe1: what did you say can be improved about opMachine1SetPassword? | 13:31 |
| === wedgwood_away is now known as wedgwood | ||
| dimitern | rogpeppe2: kanban | 14:03 |
| === deryck is now known as deryck[afk] | ||
| 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:41 |
| 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:43 |
| 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:44 |
| 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:45 |
| rogpeppe2 | dimitern: ah, you probably need to call state.Sync | 14:47 |
| dimitern | rogpeppe2: oh, forgot about that :) | 14:47 |
| === BradCrittenden is now known as bac | ||
| dimitern | rogpeppe2: all done and all tests pass! https://codereview.appspot.com/9614043 | 15:11 |
| rogpeppe2 | dimitern: woo! | 15:12 |
| dimitern | rogpeppe2: scrutiny appreciated :) | 15:13 |
| rogpeppe2 | dimitern: looking now | 15:13 |
| rogpeppe2 | dimitern: reviewed | 15:37 |
| dimitern | rogpeppe2: tyvm | 15:37 |
| === tasdomas is now known as tasdomas_afk | ||
| 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:40 |
| rogpeppe2 | dimitern: ah | 15:41 |
| rogpeppe2 | dimitern: so you have to set the correct status | 15:41 |
| dimitern | rogpeppe2: what do you mean? | 15:42 |
| * rogpeppe2 thinks we do make life hard for ourselves sometimes. more code breeds more code. | 15:42 | |
| rogpeppe2 | dimitern: ah, i see, it's just pending you can't rewind | 15:43 |
| dimitern | rogpeppe2: yes | 15:43 |
| 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:44 |
| dimitern | TheRealMue: I feel the same about the 1 or 2 char vars throughout, but changing that is out of scope I think | 15:45 |
| * dimitern thinks now once I picked up speed I can ravage through the next agent's needed API calls easily (PA I think) :) | 15:47 | |
| TheRealMue | dimitern: But otherwise it never will be changed. ;) | 15:52 |
| dimitern | TheRealMue: not quite - can be changed independently as a whole | 15:55 |
| 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:56 |
| dimitern | rogpeppe2: it can be a format string, but that will duplicate the logic in state | 15:57 |
| === rogpeppe2 is now known as rogpeppe | ||
| 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:57 |
| 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 | 15:58 |
| === deryck[afk] is now known as deryck | ||
| === TheRealMue is now known as TheMue | ||
| dimitern | rogpeppe: ok, all done, will repropose soon and I'd appreciate one last look (running tests now) | 16:31 |
| rogpeppe | dimitern: ok | 16:31 |
| dimitern | rogpeppe: https://codereview.appspot.com/9614043 | 16:41 |
| rogpeppe | dimitern: reviewed | 16:52 |
| dimitern | rogpeppe: thanks | 16:53 |
| 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:56 |
| dimitern | rogpeppe: ok | 16:58 |
| dimitern | rogpeppe: will do that and land it then | 16:58 |
| dimitern | rogpeppe: ah, perhaps you meant add it to errorTransformTests ? | 17:00 |
| dimitern | rogpeppe: or that as well as the separate test case for EnsureDead? | 17:01 |
| mgz | dimitern: poke me if any of my review comments are unclear or just silly | 17:02 |
| dimitern | mgz: already replied :) | 17:03 |
| Makyo | Anyone up for a review of https://codereview.appspot.com/9566045/ - refactor deploy to statecmd? | 17:04 |
| mgz | dimitern: you can carry forward my lgtm | 17:06 |
| dimitern | mgz: cheers | 17:06 |
| mgz | Makyo: done, poke rog or dimitern as well though | 17:12 |
| Makyo | mgz, cool, thanks. Will poke around the args, too, see what can be reduced. | 17:13 |
| === gary_poster is now known as gary_poster|away | ||
| * rogpeppe has got to go. g'night all | 17:22 | |
| rogpeppe | dimitern: yeah, it should be in errorTransformTests | 17:22 |
| 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:23 |
| dimitern | ok, i'm off - mass effect is calling for me ;) | 17:27 |
| dimitern | g'night all | 17:28 |
| === gary_poster|away is now known as gary_poster | ||
| === BradCrittenden is now known as bac | ||
| thumper | morning mramm | 21:30 |
| mramm | morning | 21:30 |
| thumper | morning wallyworld_ | 21:45 |
| wallyworld_ | g'day | 21:45 |
| === wedgwood is now known as wedgwood_away | ||
| === wedgwood_away is now known as wedgwood | ||
| === wedgwood is now known as wedgwood_away | ||
| wallyworld_ | thumper: in your branch, i can't see where DescriptionTimeout is used to kill the process | 23:58 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!