| === flaviamissi_ is now known as flaviamissi | ||
| wrtp | fwereade: mornin' | 08:04 |
|---|---|---|
| fwereade | wrtp, heyhey | 08:04 |
| wrtp | fwereade: my ping last night BTW was to let you know that i'd found an old branch that actually made some of the juju commands work, in case you were about to work on something similar. | 08:13 |
| wrtp | fwereade: it's funny though, i dusted off the branch, split off a part of it that was logically separate and proposed that, then proposed the original again, and only *then* did i discover i'd proposed it before and even had a LGTM! | 08:14 |
| fwereade | wrtp, yeah, it was nagging at my mind, I *thought* you'd done something like that :) | 08:15 |
| wrtp | fwereade: yeah, exactly. i looked at cmd/bootstrap and thought "i thought this was working now", then i found the branch and thought i'd never got around to proposing it. | 08:16 |
| wrtp | fwereade: there was quite a bit of code that i'd totally forgotten about | 08:16 |
| fwereade | wrtp, heh, ouch :) | 08:17 |
| wrtp | fwereade: it's fine. nobody's written anything similar in parallel, and the tests are *almost* passing. | 08:18 |
| wrtp | fwereade: it's quite nice actually. | 08:18 |
| fwereade | wrtp, awesome :) | 08:20 |
| fwereade | wrtp, the Init change will hit one of us soon but it shouldn't be a bother | 08:21 |
| fwereade | wrtp, oo, cath has made breakfast, bbs :) | 08:21 |
| wrtp | fwereade: is there another Init change in the offing? | 08:21 |
| wrtp | fwereade: enjoy! | 08:21 |
| wrtp | hey Daviey | 08:21 |
| fwereade | wrtp, oh, yeah, Init went in already; sorry, confused :) | 08:30 |
| fwereade | wrtp, (mmm, maltese bread :)) | 08:30 |
| * wrtp has no idea what maltese bread might be like, but thinks he should have some breakfast because his mouth is watering. | 08:31 | |
| wrtp | (mmm, Sainsbury's muesli :-)) | 08:32 |
| wrtp | TheMue: hiya | 08:32 |
| TheMue | wrtp: moin, just came back from workout | 08:33 |
| TheMue | wrtp: … and a discussion with my daughter that a fresh setup of a computer keeping software and data is more than a five minute job | 08:34 |
| * TheMue laughs | 08:34 | |
| wrtp | TheMue: what, you mean you can't just do "juju add-unit ms-windows --constraint machine=daughters-computer ? :-) | 08:38 |
| wrtp | fwereade: what *is* maltese bread like, BTW? | 08:40 |
| TheMue | wrtp: Hehe, no. And even no "osx" (she's got an Apple). OK, installing the OS is done quickly, but a backup of her data as reinstallation of her software later. | 08:40 |
| TheMue | Btw, anyone already upgraded to 12.04? | 08:41 |
| fwereade | wrtp, crusty on the outside, awesomely soft and springy on the inside, nice taste that I don't really have the vocabulary to describe; works really *really* well with tomatoes, capers, maybe basil, little bit of oil | 08:42 |
| * fwereade is hungry again | 08:42 | |
| wrtp | TheMue: not I | 08:42 |
| fwereade | TheMue, yeah, haven't got around to it yet | 08:43 |
| wrtp | fwereade: white or brown? | 08:43 |
| fwereade | wrtp, white | 08:43 |
| TheMue | wrtp, fwereade: Will also wait a few days until upgrade. | 08:44 |
| TheMue | fwereade: Hey, I'm trying to loose some weight. Please don't write about delicous food here. ;) | 08:45 |
| * fwereade resists the urge | 08:46 | |
| * TheMue once again listens to Porcupine Tree and is happy about such a great music | 08:47 | |
| fwereade | wrtp, btw, you remember we had a discussion about cmd/jujuc/server and tomb the other day | 09:17 |
| wrtp | fwereade: i do | 09:17 |
| wrtp | fwereade: i suggested not using tomb in that case if i remember rightly | 09:18 |
| fwereade | wrtp, I must say I keep wanting to use tomb when I look at it -- for example, as it is I think it will be tricky to implement both Close and Wait nicely | 09:18 |
| wrtp | fwereade: does it need a Wait? | 09:19 |
| fwereade | wrtp, perhaps not yet; but without one, error discovery is deferred until you want to close the server anyway | 09:19 |
| fwereade | wrtp, and that feels like it could be a problem | 09:19 |
| wrtp | fwereade: i see | 09:20 |
| wrtp | fwereade: depends whether something might want to wait for an error and start a new server or something | 09:20 |
| wrtp | fwereade: but to be honest, when are you ever going to get an error here? accept is never going to fail in reality. | 09:21 |
| fwereade | wrtp, I don't know... but I'm not very comfortable just dropping errors on the floor like that: just because I can't imagine the error doesn't mean it won't happen ;p | 09:22 |
| wrtp | fwereade: can you paste the code again so i can remember it a bit better? | 09:23 |
| fwereade | wrtp, http://paste.ubuntu.com/947032/ | 09:23 |
| wrtp | fwereade: are you ever going to implement any other methods on Server? (other than Wait) | 09:25 |
| wrtp | fwereade: cos i have a suggeston | 09:25 |
| wrtp | iom | 09:25 |
| fwereade | wrtp, I don;t think so | 09:25 |
| wrtp | ion | 09:25 |
| fwereade | wrtp, go on | 09:26 |
| wrtp | fwereade: why not remove the server type, and replace with a function: RunServer(...) error | 09:26 |
| wrtp | ah, no | 09:26 |
| fwereade | wrtp, I *think* I need both wait and close | 09:27 |
| wrtp | well, why not | 09:27 |
| wrtp | fwereade: when are you going to want to shut down the server? | 09:27 |
| wrtp | fwereade: won't it always run forever? | 09:27 |
| fwereade | wrtp, as part of an orderly shutdown in preparation for a code upgrade, for example | 09:27 |
| wrtp | fwereade: if we're doing that, won't we be exiting the whole program anyway? | 09:28 |
| wrtp | fwereade: alternatively... | 09:29 |
| wrtp | fwereade: don't start the server on NewServer - just return the server object. then implement Server.Run which blocks when called. | 09:31 |
| fwereade | wrtp, ah, that sounds like it could work | 09:31 |
| wrtp | fwereade: similar to rpc.Server.ServeConn | 09:31 |
| fwereade | wrtp, I'll try that, ty :) | 09:32 |
| wrtp | fwereade: np | 09:32 |
| wrtp | fwereade, TheMue: https://codereview.appspot.com/6120052/ | 11:34 |
| TheMue | *click* | 11:34 |
| wrtp | you have reviewed it before, but it has changed a certain amount | 11:34 |
| fwereade | wrtp, I can't decide whether I like the bootstrap Init | 12:00 |
| wrtp | fwereade: you mean that it initialises the Conn rather than just the env name? | 12:01 |
| fwereade | wrtp, it kinda feels as though an error in juju.NewConn would be better reported to the user with os.Exit(1) rather than (2) | 12:01 |
| fwereade | wrtp, which is the observable distinction between Init and Run errors | 12:01 |
| wrtp | fwereade: it means we can uniformly test that all commands open their environment correctly | 12:01 |
| wrtp | fwereade: although perhaps there's another better way of doing that that i haven't thought of | 12:02 |
| fwereade | wrtp, I don't see how that follows -- the code is duplicated in the Inits of Bootstrap and Destroy, so where we happen to test it is a matter of choice | 12:03 |
| fwereade | wrtp, now I think of it, what we did with Log could quite easily work with other chunks of common command functionality | 12:04 |
| wrtp | fwereade: ah, now yer talkin | 12:04 |
| fwereade | wrtp, I think it'd work out really quite nicely :) | 12:05 |
| wrtp | fwereade: consider it done | 12:05 |
| fwereade | wrtp, cool | 12:05 |
| fwereade | wrtp, I'll leave a comment for form's sake, I haven't finished looking through it yet | 12:05 |
| wrtp | fwereade: are the final Log changes submitted yet? | 12:05 |
| fwereade | wrtp, afraid not, you'll have to look at the CL for the latest | 12:06 |
| wrtp | fwereade: k | 12:06 |
| fwereade | wrtp, hoping that this morning's modifications will pass muster :) | 12:06 |
| wrtp | fwereade: i'm not sure that that approach entirely solves the problem. it looks to me like we'll still need to test the environment behaviour of every command. | 12:24 |
| wrtp | fwereade: for instance, here's what bootstrap might look like: http://paste.ubuntu.com/947216/ | 12:24 |
| wrtp | fwereade: although, i suppose that if we test at least one working example, we can infer the fact that the command has the correct c.env.open call, and hence testing the env name after Init is sufficient. | 12:26 |
| fwereade | wrtp, I think that's a good boundary, yeah | 12:26 |
| wrtp | fwereade: yeah, i think i'll do that. | 12:27 |
| fwereade | wrtp, other comments delivered btw | 12:29 |
| wrtp | fwereade: tyvm | 12:30 |
| niemeyer | Hello! | 12:57 |
| wrtp | niemeyer: yo! | 12:59 |
| TheMue | niemeyer: moin | 13:05 |
| wrtp | fwereade: PTAL | 13:12 |
| wrtp | niemeyer: https://codereview.appspot.com/6120052 | 13:12 |
| fwereade | niemeyer, heyhey | 13:12 |
| niemeyer | fwereade: https://codereview.appspot.com/6115048/ reviewed | 13:14 |
| fwereade | niemeyer, tyvm | 13:14 |
| niemeyer | fwereade: My pleasure | 13:14 |
| fwereade | niemeyer, ah, I think I missed the import of what you said before | 13:15 |
| niemeyer | wrtp: Ohhh, tasty | 13:15 |
| niemeyer | fwereade: You mean re. public/private? | 13:15 |
| fwereade | niemeyer, Help should probably be public if for no other reason than directness of testing | 13:15 |
| niemeyer | fwereade: Yeah, it looks like a nice piece for the API | 13:15 |
| fwereade | niemeyer, there's a followup that covers that but it's WIP until the parent pipeline settles down | 13:16 |
| niemeyer | fwereade: That's ok | 13:16 |
| wrtp | niemeyer: you reviewed it before some time ago, and i forgot about it, found the old branch, dusted it off, split it, and only when i re-proposed did i find the old thread! | 13:16 |
| niemeyer | wrtp: Hah :-) | 13:16 |
| niemeyer | wrtp: Nice to see this going along the cool improvements by fwereade in the last few days | 13:17 |
| fwereade | niemeyer, I feel that "ERROR:" beats "error:" in practice merely because it's often followed up with "usage:", etc, and it makes it stand out much better | 13:17 |
| niemeyer | wrtp: Should we do a gigantic s/Environ/Env/ replacement? | 13:18 |
| wrtp | fwereade: the Init/Run distinction works well. i don't know quite why we didn't go with that originally. maybe because we didn't think of the trick of passing in FlagSet | 13:18 |
| niemeyer | and s/environs/envs/ etc | 13:18 |
| wrtp | niemeyer: i like Environ | 13:18 |
| wrtp | niemeyer: but i'd prefer "environ" as the package name | 13:18 |
| niemeyer | wrtp: I see we often pick one or the other arbitrarily | 13:18 |
| fwereade | wrtp, indeed, it would be interesting to go spelunking through its very early history in my copious free time ;) | 13:18 |
| wrtp | fwereade: yeah, there was a long discussion AFAIR | 13:19 |
| niemeyer | That's of course not too important, and if there's no agreement let's move on | 13:19 |
| wrtp | niemeyer: i realised that unnecessary plurals don't help anything. hence my "environs->environ" suggestion. | 13:20 |
| wrtp | niemeyer: but i'm much less keen on abbreviations than i used to be :-) | 13:20 |
| niemeyer | wrtp: I'd take "env" as a tradeoff ;-) | 13:21 |
| wrtp | niemeyer: i'll think about it | 13:22 |
| niemeyer | wrtp: FWIW, we had "environs" to free the "environ" name for variables, if I'm not mistaken | 13:23 |
| niemeyer | I may be wrong, though | 13:23 |
| wrtp | niemeyer: maybe. though we usually use "e" or "env" (a good reason not to rename the package, perhaps) | 13:23 |
| TheMue | niemeyer: Just for info, https://codereview.appspot.com/6111053/ has the changes after the review in and https://codereview.appspot.com/6118055/ is a new one I would like to see in trunk before moving the watchers. | 13:24 |
| niemeyer | wrtp: Or a good reason to have it as juju/envs | 13:25 |
| niemeyer | wrtp: same as strings, bytes, .. | 13:25 |
| niemeyer | TheMue: Cool, thanks | 13:25 |
| wrtp | niemeyer: i don't feel like i type it that often, or that it's an eyesore when i do. | 13:26 |
| niemeyer | wrtp: "that"? | 13:26 |
| wrtp | niemeyer: although i'm still thinking :-) | 13:26 |
| wrtp | niemeyer: "environ" | 13:26 |
| wrtp | niemeyer: "Environ" | 13:26 |
| niemeyer | wrtp: Ah, .. it's more that we can't agree on the proper way to abbreviate it | 13:26 |
| niemeyer | wrtp: Not that I dislike it per se | 13:27 |
| wrtp | niemeyer: true. | 13:27 |
| niemeyer | wrtp: Your branch has EnvName | 13:27 |
| wrtp | niemeyer: ah! | 13:27 |
| wrtp | niemeyer: now i see where you're coming from! | 13:27 |
| niemeyer | wrtp: It's not the only place.. it just reminded me of the issue | 13:27 |
| wrtp | niemeyer: hmm, you're right: http://paste.ubuntu.com/947322/ | 13:28 |
| niemeyer | wrtp: I'm pretty sure I had reviewed that branch before, but my comments are not there.. was there a different thread in place already? | 13:28 |
| wrtp | niemeyer: look at the first message in the thread | 13:28 |
| wrtp | niemeyer: i had to make a new CL because i changed the dependency | 13:28 |
| niemeyer | OMG | 13:29 |
| niemeyer | I can see that :-) | 13:29 |
| niemeyer | wrtp: I reminded of it because I still feel the same way about the tests | 13:29 |
| wrtp | niemeyer: all of them? i think there's enough commonality to justify tables, but perhaps only because i anticipate very similar tests for other subcommands. | 13:30 |
| niemeyer | wrtp: cmd_test.go | 13:30 |
| wrtp | niemeyer: there are two table-driven tests there. | 13:31 |
| niemeyer | wrtp: It feels over the top.. I'm tempted to reduce them to straightforward checks to see how it'd look | 13:31 |
| wrtp | niemeyer: the first one really will be identical for every command that takes a --environment flag | 13:31 |
| niemeyer | wrtp: Maybe I'm just missing how complex it is, but the amount of exceptions and reflection on these tests makes me feel like it's being unnaturally forced onto a table | 13:31 |
| wrtp | niemeyer: i don't see any exceptions in the EnvironmentInit tests, or reflection in the Commands tests. | 13:32 |
| wrtp | niemeyer: your argument is stronger for TestCommands, i think | 13:34 |
| niemeyer | if t.initErr != "" {; if t.runErr != "" {; com := testInit(c, t.cmd, t.args, t.initErr); etc | 13:34 |
| wrtp | niemeyer: i could easily factor out the command test thing into a function. | 13:34 |
| niemeyer | wrtp: This is also extremely simplistic: ops: envOps("peckham", dummy.OpBootstrap), | 13:35 |
| wrtp | niemeyer: say runCommand(c *C, com Command, args ...string) (initErr, runErr error, ops []dummy.Operation) | 13:35 |
| niemeyer | wrtp: This only works for cases where it's calling a simple method without any expectations about it | 13:35 |
| niemeyer | wrtp: It's mocking on its best.. tying implementation shape to the tests | 13:36 |
| wrtp | niemeyer: that's perhaps true | 13:36 |
| wrtp | niemeyer: i'll try refactoring it. i stick to my guns on TestEnvironmentInit though. | 13:36 |
| niemeyer | wrtp: This one is not so bad, but I'd change it to have a list of factories rather than a list of commands | 13:37 |
| niemeyer | wrtp: So that the magic copying can go away | 13:37 |
| wrtp | niemeyer: ok | 13:37 |
| niemeyer | wrtp: That's the only comment really.. everything else is great | 13:41 |
| wrtp | niemeyer: it doesn't get rid of reflection entirely BTW | 13:41 |
| wrtp | niemeyer: thanks | 13:41 |
| andrewsmedina | niemeyer, wrtp morning | 13:43 |
| wrtp | andrewsmedina: hiya | 13:43 |
| andrewsmedina | wrtp: are you ok? | 13:44 |
| wrtp | andrewsmedina: fine thanks. and you? | 13:44 |
| andrewsmedina | wrtp: when you have time, please take a look https://codereview.appspot.com/6099051/ | 13:45 |
| andrewsmedina | wrtp: im ok | 13:45 |
| wrtp | andrewsmedina: yes i will. thanks for the reminder. | 13:45 |
| andrewsmedina | wrtp: thanks | 13:48 |
| wrtp | andrewsmedina: one question: do you need to be root to run those tests? | 14:05 |
| wrtp | andrewsmedina: also i'm slightly concerned that running the tests might have undesirable side effects (but i'm not familiar with virsh - perhaps there are none) | 14:09 |
| andrewsmedina | wrtp: not. the user that will run the tests need access to libvirt | 14:10 |
| wrtp | andrewsmedina: and side effects? | 14:10 |
| andrewsmedina | wrtp: not | 14:10 |
| andrewsmedina | wrtp: i created and drestoy a dummy net | 14:11 |
| wrtp | andrewsmedina: i don't see anywhere in the tests that the net is destroyed | 14:12 |
| === nijaba is now known as nijaba_tab | ||
| === nijaba_tab is now known as nijaba | ||
| andrewsmedina | wrtp: in the other tests I'm using the default libvirt net | 14:14 |
| wrtp | andrewsmedina: sorry, i don't know anything about libvirt. what's the default libvirt net? | 14:15 |
| wrtp | andrewsmedina: is it something the user might be using anyway? | 14:15 |
| niemeyer | fwereade: https://codereview.appspot.com/6123049/ done | 14:16 |
| fwereade | niemeyer, tyvm | 14:17 |
| andrewsmedina | wrtp: I'll be more careful in the tests and create a flag for those who have not installed virsh | 14:17 |
| andrewsmedina | wrtp: he may be using the network default but I do not modify it in the tests | 14:18 |
| wrtp | andrewsmedina: isn't starting it a modification? | 14:18 |
| fwereade | niemeyer, nice suggestion, ty | 14:18 |
| niemeyer | fwereade: Glad you like it | 14:19 |
| andrewsmedina | wrtp: the start verify if the net is already started | 14:19 |
| niemeyer | fwereade: I pondered about the forcing upgrade as well, btw | 14:19 |
| wrtp | andrewsmedina: what if it's not already started? | 14:19 |
| niemeyer | fwereade: I didn't bother too much because it's mimicking what's in Py today | 14:20 |
| fwereade | niemeyer, oh, ok | 14:20 |
| niemeyer | fwereade: I'm happy to splat the state if we have --force, but I don't think the opposite is sensible | 14:20 |
| niemeyer | fwereade: IOW, if somebody does "upgrade --force", and then "upgrade", the second shouldn't drop the --force state | 14:21 |
| fwereade | niemeyer, ah, I thought it... kinda dumb to do so, but not intrinsically *bad* | 14:21 |
| TheMue | niemeyer, fwereade: I found it in the Py code, that a change isn't allowed. | 14:22 |
| fwereade | niemeyer, I don't really feel that it should be an error, even so | 14:22 |
| fwereade | niemeyer, it may silently refuse to downgrade from --force to not | 14:22 |
| niemeyer | fwereade: I think it's actually bad because doing an upgrade at all is generally forbidden unless the unit is running fine | 14:22 |
| niemeyer | fwereade: The first succeeds because of the --force, and the second succeeds because of the first | 14:22 |
| andrewsmedina | wrtp: a network should be active, inactive or does not exists | 14:23 |
| niemeyer | fwereade: So, in a way, the second would only happen at all because there's a --force in place.. | 14:23 |
| fwereade | niemeyer, hmm, ok | 14:23 |
| andrewsmedina | wrtp: if it's active, it's started | 14:23 |
| niemeyer | fwereade: That's why, in that situation, I'd rather see "hey, already doing it, hold on" rather than "sure!" | 14:23 |
| wrtp | andrewsmedina: by running this test, the current status is changed. i don't think tests should have side-effects. | 14:23 |
| andrewsmedina | wrtp: in your machine? | 14:24 |
| wrtp | andrewsmedina: that's right | 14:24 |
| niemeyer | TheMue: Are you with us | 14:24 |
| niemeyer | ? | 14:24 |
| niemeyer | TheMue: The summary is: | 14:24 |
| wrtp | andrewsmedina: it's ok if it has side-effects that are cleaned up afterwards | 14:24 |
| niemeyer | TheMue: upgrade + upgrade == fine | 14:24 |
| niemeyer | TheMue: upgrade + force-upgrade == fine | 14:24 |
| andrewsmedina | wrtp: yes | 14:25 |
| niemeyer | TheMue: force-upgrade + force-upgrade == fine | 14:25 |
| niemeyer | TheMue: force-upgrade + upgrade == NOT fine | 14:25 |
| andrewsmedina | wrtp: I will use a testdummy net in tests for start method ok? | 14:25 |
| wrtp | andrewsmedina: but anyone should be able to run go test launchpad.net/juju/go/... and be happy that it won't interfere with anything on their machine | 14:25 |
| niemeyer | TheMue: Well.. or maybe, fine as well | 14:25 |
| niemeyer | fwereade: We could actually make it fine, by saying that the --force continues in place in that situation | 14:25 |
| TheMue | niemeyer: Hmm, sadly don't know enough about the upgrade mechanism in total. | 14:26 |
| wrtp | andrewsmedina: you should make sure to delete the net afterwards too (perhaps by defining TearDownSuite) | 14:26 |
| fwereade | niemeyer, yeah, that seemed nicer to me | 14:26 |
| andrewsmedina | wrtp: you are "+1" for virsh flag or I shoud reuse the lxc flag? | 14:26 |
| andrewsmedina | wrtp: I'm doing it for other tests | 14:26 |
| wrtp | andrewsmedina: i don't understand | 14:26 |
| fwereade | niemeyer, you're not allowed to, er, downgrade your upgrade ;) | 14:26 |
| niemeyer | TheMue, fwereade: Ok, so what about this.. let's just move on with what's there given that it mimics what exists today.. we can improve it later | 14:26 |
| niemeyer | fwereade: We can then implement that nicer behavior on a follow up | 14:26 |
| TheMue | niemeyer: Sounds ok. | 14:27 |
| fwereade | niemeyer, TheMue: yep, I'm fine with that | 14:27 |
| niemeyer | TheMue: Cool | 14:27 |
| wrtp | andrewsmedina: what flag are you referring to? | 14:28 |
| * TheMue sadly forgot why he added (TODO) to the comment. | 14:28 | |
| TheMue | I'm getting old. | 14:28 |
| andrewsmedina | wrtp: a "--virsh" flag | 14:28 |
| andrewsmedina | ops | 14:28 |
| niemeyer | TheMue: So you just need to observe the last of fwereade's point to see how to get to some agreement | 14:28 |
| andrewsmedina | wrtp: a "-virsh" flag | 14:29 |
| wrtp | andrewsmedina: it depends whether you think you can implement the tests without affecting the user's environment | 14:29 |
| wrtp | andrewsmedina: if you don't, i think it would be a good idea to implement some tests that can be run without running virsh, even if they don't verify all the functionality | 14:30 |
| andrewsmedina | wrtp: how I mock the virsh commands? | 14:30 |
| wrtp | andrewsmedina: you could set $PATH | 14:31 |
| andrewsmedina | wrtp: I dont undertand | 14:31 |
| wrtp | andrewsmedina: have a look at state/ssh_test.go | 14:31 |
| wrtp | andrewsmedina: it uses that technique | 14:31 |
| andrewsmedina | wrtp: brb | 14:32 |
| wrtp | andrewsmedina: ok | 14:32 |
| TheMue | fwereade: For the watchers which run concurrently I would like to keep those changes in the background (see the other watchers). Direct write/read tests are already done in state_test.go. | 14:32 |
| fwereade | TheMue, I don't see how it's a benefit to run the test code concurrently when you don't have to | 14:37 |
| fwereade | TheMue, the change goes through ZK just the same either way, and if you skip the concurrency you can eliminate both waiting and the (admittedly unlikely) potential event coalescing | 14:39 |
| TheMue | fwereade: How would you build this as a table driven test? | 14:40 |
| andrewsmedina | wrtp: im back :D | 14:40 |
| fwereade | TheMue, I'm not sure it favours a table-driven style so much as it does a plain old procedural style | 14:41 |
| TheMue | fwereade: niemeyer and wrtp convinced me about the elegant way of doing it table-driven. And I've got to admit, I now like this way. It's clean. | 14:42 |
| fwereade | TheMue, heh, has it changed already? | 14:42 |
| fwereade | TheMue, still don't feel it's a good fit tbh, but perhaps it'll grow on me | 14:43 |
| TheMue | fwereade: What has changed? I had changed other watcher tests to be table-driven today. | 14:43 |
| TheMue | fwereade: All now work the same way, also the new PortsWatcher test. | 14:44 |
| fwereade | TheMue, sorry, I'm just unclear on a largely irrelevant point: whether or not the watcher tests were originally more procedural | 14:45 |
| TheMue | fwereade: I've started them in a more mixed way, asynchronous changes and a procedural testing of the obtained values. | 14:46 |
| TheMue | fwereade: And here it would habe been more simple to move the changes between the tests. | 14:47 |
| fwereade | TheMue, it's the asynchronicity that I feel is the problem anyway, not the table-drivenness | 14:47 |
| fwereade | TheMue, what behaviour does it exercise that a synchronous test wouldn't? | 14:48 |
| TheMue | fwereade: Yes, that's why I asked you if you've got a good idea on integrate the changes int the table too. | 14:48 |
| TheMue | fwereade: I've got no problem in convert it to be synchronous (but in a different branch, all watchers, after moving). But I would like to keep it table-driven. | 14:50 |
| fwereade | TheMue, `func() { unit.OpenPort("tcp", 80)}` ? | 14:50 |
| andrewsmedina | wrtp: I will create the mock for virsh like your ssh tests, ok? | 14:50 |
| wrtp | andrewsmedina: you've got a review | 14:50 |
| TheMue | fwereade: OK, so the table has to be an anonymous struct to contain test func and expected result, but yous, looks ok to me. | 14:51 |
| fwereade | TheMue, cool | 14:52 |
| wrtp | andrewsmedina: i think so. perhaps you might talk to niemeyer some time about this too. | 14:52 |
| andrewsmedina | wrtp: ok | 14:52 |
| andrewsmedina | wrtp: I will improve the tests and talk with you and niemeyer again | 14:52 |
| TheMue | niemeyer: How about you? I would do that change after this watcher and the PortsWatcher are in and then the branch with moving all watchers is in too. | 14:53 |
| niemeyer | TheMue: Which change? | 14:59 |
| niemeyer | TheMue: The refactoring fwereade suggested? | 15:00 |
| TheMue | niemeyer: I discussed with fwereade to take the concurrent changes out of the tests later. | 15:00 |
| TheMue | niemeyer: Yes. | 15:00 |
| niemeyer | TheMue: Yeah, as long as fwereade is happy, I'm happy | 15:01 |
| niemeyer | TheMue: and as long as you're happy too, of course | 15:01 |
| TheMue | niemeyer: Fine, and I will keep it as small branches to make you happy. | 15:01 |
| niemeyer | TheMue: WOohay small branches! | 15:01 |
| niemeyer | :) | 15:01 |
| TheMue | niemeyer, fwereade: Big party! | 15:01 |
| niemeyer | ! | 15:01 |
| niemeyer | i i i | 15:02 |
| niemeyer | ---------- | 15:02 |
| TheMue | *rofl* | 15:02 |
| fwereade | TheMue, I'm happy :) | 15:02 |
| niemeyer | | | | 15:02 |
| niemeyer | \:-) | 15:02 |
| TheMue | Oh, we get a cake. | 15:02 |
| TheMue | Btw, just had some selfmade chocolate chips by my wife with the coffee. Yummy. | 15:03 |
| fwereade | niemeyer, btw, ERROR/error: thoughts? ERROR stands out much better when the error line is not the only output | 15:09 |
| TheMue | niemeyer: So, submitted, next stop: PortsWatcher. ;) | 15:13 |
| niemeyer | TheMue: Was just looking at it, but in a meeting with zaid just now.. will continue after lunch | 15:14 |
| niemeyer | fwereade: just a sec and will be with you | 15:14 |
| TheMue | niemeyer: OK, np. | 15:22 |
| niemeyer | fwereade: I'd prefer "error: unrecognized flag --foo" than "ERROR: unrecognized flag --foo" | 15:30 |
| niemeyer | fwereade: My feeling about it is that it was just a problem detected.. no one is dying :-) | 15:31 |
| fwereade | niemeyer, fair enough, I imagine I'll grow used to it :) | 15:31 |
| niemeyer | fwereade: That said, you have a point.. | 15:31 |
| niemeyer | fwereade: Something we should do is to reorder the error vs. help | 15:31 |
| niemeyer | fwereade: In that one place we print the error & help, error should come last | 15:31 |
| niemeyer | fwereade: So it's next to the prompt | 15:31 |
| fwereade | niemeyer, that's no problem... and it will be much clearer in that case anyway | 15:32 |
| fwereade | niemeyer, ok, if I rearrange all that and make it lowercase, good to submit? | 15:32 |
| niemeyer | fwereade: In other cases, I suspect that's already happening | 15:32 |
| niemeyer | fwereade: I mean, error is already the last thing printed | 15:32 |
| niemeyer | fwereade: As long as that's true, I don't see a problem with it being lowercased as it'll be very clear | 15:33 |
| niemeyer | fwereade: Yep, +1 | 15:33 |
| niemeyer | TheMue: Btw, I just detected a silliness I've suggested that isn't necessary | 15:34 |
| niemeyer | TheMue: You know that change, ok := ... if !ok { s.tomb.Kill(nil); return }, that we're using everywhere? | 15:35 |
| TheMue | niemeyer: Yes? | 15:35 |
| niemeyer | TheMue: It's not necessary | 15:35 |
| niemeyer | TheMue: It's fine to just return in that case | 15:36 |
| niemeyer | TheMue: The deferred Done() will make it all work fine | 15:36 |
| TheMue | niemeyer: So check the ok but don't Kill()? OK, I'll change it during refactoring. | 15:36 |
| TheMue | niemeyer: Is it ok when putting it into the watcher movement change? | 15:37 |
| TheMue | niemeyer: So that change would only contain this code change and the moving. | 15:37 |
| niemeyer | TheMue: No, please not in the movement change | 15:39 |
| niemeyer | TheMue: You can put it in the PortsWatcher, though | 15:39 |
| niemeyer | TheMue: As it's just a bunch of one-liner removals, that's fine | 15:39 |
| TheMue | niemeyer: OK, no pro. | 15:39 |
| niemeyer | TheMue: If you do that now, please fix the comment of the watcher as well.. it's referring to the copy & pasted watcher | 15:40 |
| niemeyer | TheMue: I'll review it right after lunch and hopefully we can already get it in | 15:40 |
| TheMue | niemeyer: fwereade already reviewd it. | 15:40 |
| niemeyer | TheMue: Ah, superb | 15:40 |
| niemeyer | TheMue: Btw, do you understand why these lines can be removed? | 15:40 |
| TheMue | niemeyer: OK, will change it now and propose it immediately. | 15:40 |
| niemeyer | TheMue: Btw, do you understand why these lines can be removed? | 15:43 |
| wrtp | niemeyer: PTAL https://codereview.appspot.com/6120052 | 15:44 |
| TheMue | niemeyer: We don't pass an error to outside, but want to close the tomb and the channel. That's done by defers after the return. | 15:45 |
| niemeyer | TheMue: Right, Done() takes care of "killing" the tomb if it's not yet dead | 15:46 |
| niemeyer | TheMue: So as long as we're returning from this function, with nil err, just returning is fine | 15:46 |
| niemeyer | wrtp: Thanks, I'll have a look right after lunch + TheMue's | 15:47 |
| TheMue | niemeyer: Fine, code reduction at its best. | 15:47 |
| niemeyer | TheMue: +1 | 15:47 |
| * niemeyer => lunch | 16:03 | |
| wrtp | isn't it nice to know that gustavo implies lunch? i'll make sure to have him around as often as possible. | 16:16 |
| TheMue | wrtp: *rofl* took a few moments to get it. | 16:20 |
| wrtp | :) | 16:20 |
| wrtp | hmm, i thought i'd sussed this prerequisite stuff. but despite small diffs | 16:28 |
| wrtp | bzr diff --old lp:~rogpeppe/juju/go-more-commands | wc | 16:28 |
| wrtp | 68 260 2113 | 16:28 |
| wrtp | i guess this: | 16:28 |
| wrtp | https://codereview.appspot.com/6117064 | 16:28 |
| wrtp | s/guess/get | 16:28 |
| TheMue | wrtp: Strange. It claims different changes as yours. Hmm. | 16:30 |
| wrtp | TheMue: yeah. somewhere it's diffing against an unmerged branch | 16:31 |
| wrtp | or something | 16:31 |
| TheMue | wrtp: I still don't understand the prereq stuff. | 16:33 |
| wrtp | TheMue: i thought i did! | 16:33 |
| TheMue | wrtp: As long as the tools are working I'm happy. But in case of a problem I'm lost. | 16:34 |
| wrtp | TheMue: and this is really weird: http://paste.ubuntu.com/947637/ | 16:42 |
| wrtp | if i revert to a revision, isn't the revision-id going to be that revision? | 16:42 |
| * wrtp is trying to work out how to diff two revision-ids | 16:42 | |
| fwereade | TheMue, wrtp: is it not that lbox requires that changes on the target that have been merged into the proposed branch have *also* been merged into the prereq, even if the prereq has already been merged? | 16:43 |
| TheMue | wrtp: Huh | 16:43 |
| wrtp | fwereade: yes. i did that though, i think | 16:43 |
| fwereade | wrtp, ah ok -- it just looked a bit like it has for me when I've got that wrong | 16:43 |
| wrtp | fwereade: me too. that's why "i thought i understood it!" | 16:43 |
| fwereade | wrtp, haha, sorry :( | 16:44 |
| wrtp | oh, i have uncommitted changes. | 16:45 |
| wrtp | and it wants to remove cmd/juju/destroy.go | 16:45 |
| wrtp | so weird. i could've sworn it passed its test *and* i merged it. | 16:47 |
| fwereade | gn all, take care | 17:00 |
| wrtp | fwereade: gn, take care | 17:01 |
| wrtp | niemeyer, TheMue: https://codereview.appspot.com/6117064 | 17:01 |
| wrtp | all that hassle for about 6 lines of code :-) | 17:01 |
| wrtp | niemeyer, TheMue: ultra-small branch BTW! | 17:01 |
| TheMue | wrtp: *lol* | 17:02 |
| niemeyer | wrtp: LGTM | 17:04 |
| wrtp | niemeyer: still waiting for prereq BTW, but i'm sure you know that :-) it turns out that it didn't need to be a prereq, but i didn't want to delete the CL. | 17:05 |
| wrtp | i probably should've | 17:05 |
| niemeyer | wrtp: Yeah, I'll just review TheMue's first | 17:06 |
| wrtp | niemeyer: np | 17:06 |
| wrtp | niemeyer: do you know if http.Client.Do is guaranteed to close req.Body ? | 17:10 |
| niemeyer | TheMue: Done.. just a couple of trivials | 17:16 |
| TheMue | niemeyer: Fine, thx. | 17:16 |
| niemeyer | wrtp: I don't | 17:16 |
| wrtp | niemeyer: np. just wondering about s3 Put. but i don't think it matters actually. | 17:17 |
| niemeyer | wrtp: I'd check the implementation | 17:26 |
| TheMue | niemeyer: Do you wonna see the openPortsNode change? Otherwise I submit it. | 17:26 |
| niemeyer | TheMue: No, happy to have it submitted, thanks! | 17:26 |
| wrtp | niemeyer: yeah, i did, but it's not greatly obvious | 17:26 |
| wrtp | time to go | 17:28 |
| wrtp | niemeyer, TheMue: g'night. see ya tomorrow. | 17:28 |
| TheMue | wrtp: Yes, good night, I leave too in a few minutes. | 17:28 |
| TheMue | wrtp: CU | 17:28 |
| niemeyer | wrtp: Thanks, and have a good one too | 17:28 |
| wrtp | niemeyer, TheMue: to both of you too | 17:29 |
| niemeyer | wrtp: I think the answer is no, there's no guarantee | 17:36 |
| TheMue | niemeyer: So, I'm off. CU tomorrow. | 17:39 |
| niemeyer | TheMue: Cheers, have a good evening too | 17:39 |
| TheMue | niemeyer: You too, bye. | 17:39 |
| robbiew | niemeyer: we have 1:1 now, mind if we just have it in person next week? | 18:31 |
| * robbiew is cleaning up Precise blueprints all day today :/ | 18:31 | |
| niemeyer | robbiew: np | 18:32 |
| robbiew | thx...back to the grind | 18:33 |
| === bcsaller_ is now known as bcsaller | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!