| niemeyer_ | Heya | 00:33 |
|---|---|---|
| === niemeyer_ is now known as niemeyer | ||
| niemeyer | andrewsmedina: ping | 00:34 |
| andrewsmedina | niemeyer: PING andrews (0.0.0.0): 56 data bytes | 00:40 |
| niemeyer | andrewsmedina: Heya | 00:40 |
| niemeyer | :) | 00:40 |
| andrewsmedina | niemeyer: 64 bytes from andrews: icmp_seq=0 ttl=48 time=613.936 ms | 00:41 |
| andrewsmedina | niemeyer: =) | 00:41 |
| niemeyer | andrewsmedina: Gosh, that's a terrible ping time ;) | 00:41 |
| davecheney | niemeyer: thanks for your comments on the dummy branch | 01:36 |
| davecheney | i have another idea which i'll propoes for you and rog. | 01:36 |
| niemeyer | davecheney: My pleasure | 01:36 |
| niemeyer | davecheney: Sounds great, thanks | 01:36 |
| davecheney | i'm thinking of making Operation an interface | 01:36 |
| davecheney | then op.Kind becomes a type switch | 01:37 |
| davecheney | will do a quick branch to see how large the change is | 01:37 |
| davecheney | to me this feels better than sticking a lot of mostly zero fields in Operation | 01:37 |
| niemeyer | davecheney: Sounds reasonable | 01:37 |
| niemeyer | davecheney: We can also mix both | 01:37 |
| niemeyer | davecheney: Having Kind() on the interface | 01:37 |
| davecheney | sure | 01:37 |
| niemeyer | But see how it looks like.. | 01:38 |
| davecheney | that might be a good way to introduce it | 01:38 |
| niemeyer | Experimenting should tell the best path | 01:38 |
| davecheney | i'll discuss it with Rog this evening | 01:38 |
| niemeyer | davecheney: type OperationKind int; func (k OperationKind) Kind() OperationKind | 01:39 |
| niemeyer | davecheney: ;) | 01:39 |
| davecheney | indeed, it makes it easy to introduce a placeholder type | 01:39 |
| niemeyer | <3 Go | 01:39 |
| davecheney | niemeyer: the reason I added the insts field to operation was so I could get the actual *instance | 01:40 |
| davecheney | in general, do you think this is a sane way of doing it | 01:40 |
| davecheney | it might require instance to define some sort of equality | 01:40 |
| niemeyer | davecheney: I think it's reasonable.. for that specific case, you might as well have opened the environment a second time | 01:40 |
| niemeyer | davecheney: and retrieved the instance from the env itself | 01:40 |
| niemeyer | davecheney: But I don't think that works for everything | 01:41 |
| niemeyer | davecheney: E.g. you might as well take the chance to check the machine id (!) | 01:41 |
| niemeyer | davecheney: It's a sane field to stick in the candidate StartInstanceOp struct type | 01:41 |
| davecheney | but not a good case for all operatoins | 01:41 |
| niemeyer | davecheney: and that kind of thing starts to get boring to check on the environment | 01:41 |
| niemeyer | davecheney: We'll have more of those over time | 01:42 |
| niemeyer | davecheney: E.g. constraints, etc | 01:42 |
| niemeyer | davecheney: Which are easier to take them out of the operation type than from querying the env externally | 01:42 |
| davecheney | niemeyer: https://codereview.appspot.com/6315043 | 01:42 |
| davecheney | mercifully shorter | 01:42 |
| davecheney | will propose this afternoon | 01:42 |
| niemeyer | davecheney: Hm? | 01:53 |
| niemeyer | davecheney: I've LGTMd it :) | 01:53 |
| niemeyer | davecheney: Looks good overall, anyway | 01:54 |
| niemeyer | I'll head to bed now | 01:54 |
| niemeyer | Night all | 01:54 |
| davecheney | niemeyer: night | 01:56 |
| rog | davecheney: hiya | 06:14 |
| rog | davecheney: the possibility of making Operation an interface was always there, but i didn't want to complicate things prematurely | 06:15 |
| rog | davecheney: you might also consider adding an "Error() error" method, because ISTR that some test code relies on knowing that the operation has been invoked even though it returns an error | 06:18 |
| davecheney | ahoy! | 06:22 |
| davecheney | rog: did you catch the conversation with gustavo ealier ? | 06:22 |
| rog | davecheney: yeah | 06:22 |
| davecheney | I had a quick crack at making Operation an interface and it isn't that horrible | 06:23 |
| davecheney | dozens of lines, not hundreds | 06:23 |
| rog | davecheney: i did a similar thing with file operations some time ago | 06:23 |
| davecheney | if you think it sounds good in principal, i'll polish it up and propose | 06:23 |
| davecheney | rog: so the PA can now start and stop machines | 06:24 |
| rog | davecheney: yeah, i think that sounds better than arbitrarily putting Instances in Operation, actually | 06:24 |
| davecheney | what is stopping us trying to bootstrap an environment ? | 06:24 |
| rog | davecheney: cool. have you tried using it for real? | 06:24 |
| davecheney | rog: yeah, then we can have concrete impls of operation that contain only the fields they need | 06:25 |
| rog | davecheney: just the code in cloudinit | 06:25 |
| rog | davecheney: yeah | 06:25 |
| davecheney | rog: i agree that having a bunch of fields which are only valid in some circumstances is a code smell | 06:25 |
| davecheney | and exactly what go interfaces avoid | 06:25 |
| rog | davecheney: yup | 06:25 |
| davecheney | sounds like a solid case to proceed | 06:26 |
| rog | davecheney: i just had a dream in which i was vigorously trying to defend Go interfaces against discriminated unions to an old work colleague :) | 06:26 |
| rog | davecheney: while wandering through a forest full of fungi... | 06:27 |
| rog | davecheney: strange | 06:27 |
| * davecheney is speechless | 06:27 | |
| davecheney | so, lets change the subject to cloud init | 06:27 |
| rog | lets | 06:27 |
| davecheney | tell me what is missing and if I can help | 06:28 |
| rog | davecheney: i think that it just needs a line of code in environs/ec2 | 06:28 |
| rog | davecheney: it might even have a TODO there | 06:28 |
| davecheney | curses, we're doomed, we'll never make it | 06:28 |
| davecheney | :) | 06:28 |
| rog | davecheney: yeah, grep for TODO in environs/ec2/*.go | 06:29 |
| rog | davecheney: only problem is i think the amazon tests might currently be broken. | 06:30 |
| rog | davecheney: i haven't been running them often enough (they take ages to run and only work 30% of the time anyway because of transitory errors). | 06:30 |
| rog | davecheney: i need to up the timeout and ssh to the bootstrap machine to see what's going on | 06:31 |
| rog | davecheney: in fact, i think i'll do that now. | 06:31 |
| davecheney | rog: hotsauce | 06:31 |
| rog | davecheney: congrats BTW. having the first agent done is a huge step forward! | 06:32 |
| rog | davecheney: ha! found the culprit! http://paste.ubuntu.com/1048666/ | 06:44 |
| rog | davecheney: i've no idea why it's trying to use the dummy package in the cloud | 06:45 |
| fwereade | rog, davecheney: mornings | 06:45 |
| rog | fwereade: hiya! | 06:47 |
| davecheney | rog: o_O! | 06:47 |
| davecheney | rog: how much work is it to resolve that facepalm ? | 06:52 |
| rog | davecheney: i hope not much. the stack traces don't seem to match the code, so i'm just sanity checking the binaries | 06:52 |
| rog | davecheney: i don't know how it is ending up in that code. | 06:53 |
| davecheney | could it be the old juju tree ? | 06:53 |
| rog | davecheney: it's possible. i'll try again, modifying that error message... | 06:54 |
| davecheney | rm -rf $GOPATH/pkg/launchpad.net/juju | 06:54 |
| rog | davecheney: the binaries don't come from there | 06:54 |
| rog | davecheney: it builds them on demand in a temporary directory | 06:54 |
| davecheney | gocheck ? | 06:54 |
| rog | davecheney: gocheck? | 06:59 |
| rog | davecheney: i've found the problem BTW | 07:00 |
| rog | davecheney: and i think you came up with the solution a few days ago | 07:00 |
| davecheney | fixitfixitfixit | 07:00 |
| rog | davecheney: the problem is that provisioning.go imports the dummy environ | 07:00 |
| davecheney | rog: ... yes, | 07:01 |
| davecheney | maybe it shouldn't, as long as I import it in the tests, it will work | 07:01 |
| rog | davecheney: that's right | 07:01 |
| * davecheney facepalm | 07:01 | |
| davecheney | i'll fix that shit right now | 07:01 |
| rog | davecheney: i'll do it here and check that it works too. | 07:02 |
| fwereade | rog, I have a trivial for you: https://codereview.appspot.com/6308089 | 07:02 |
| davecheney | rog: if you want to propse, please do, i have to run out in a second | 07:02 |
| rog | davecheney: ok, will do | 07:02 |
| rog | fwereade: LGTM. oops. | 07:03 |
| fwereade | rog, np, cheers :) | 07:03 |
| fwereade | rog, how would you feel about me trying to factor out some sort of common topology watcher? I want a service relation watcher, and at first glance I'm going to end up duplicating most of the machines/machine-units watchers | 07:07 |
| rog | fwereade: i'm not convinced. i can't see that it could save more than 4 lines per watcher. | 07:09 |
| rog | fwereade: (the code that parses the topology) | 07:09 |
| rog | fwereade: what other code do you think you could factor out? | 07:10 |
| fwereade | rog, I was wondering whether I could get rid of the Stop and a fair amount of loop, but it might indeed not turn out all that nice | 07:12 |
| rog | fwereade: i'm not that keen on adding another buffered value for not that much gain | 07:13 |
| fwereade | rog, yeah, fair enough, I'll give it 15 mins and then take a cynical self-look | 07:14 |
| rog | fwereade: sounds like a good plan | 07:14 |
| rog | fwereade: could you check that "go test launchpad.net/juju-core/juju/..." works for you, please? | 07:20 |
| rog | fwereade: it's hanging up for me in jujuc/server | 07:20 |
| fwereade | rog, did just a few minutes ago | 07:20 |
| rog | fwereade: hmm. | 07:20 |
| rog | fwereade: oh, bizarre. i've found the problem. | 07:22 |
| * fwereade concludes that, yeah, a topology watcher isn't worth the effort, and is sad about that | 07:23 | |
| * fwereade never expected to seriously miss c-style macros | 07:23 | |
| rog | fwereade: there may easily be some other way to refactor all that code. | 07:24 |
| rog | fwereade: i had some ideas right at the beginning which might work. | 07:24 |
| fwereade | rog, I'll probably get annoyed with it again later today and give the tyres another kick, but it seems like it'll get a bit convoluted unless I'm careful | 07:26 |
| rog | fwereade: yeah, that's the potential problem. | 07:27 |
| rog | fwereade: here's why that test was hanging BTW. i'd changed state.zkTimeout to 45*time.Minute. | 07:28 |
| rog | fwereade: but... that *should not* have caused the test to hang | 07:28 |
| rog | fwereade: when i changed the timeout back to 15*time.Second, the dial in question succeeded after 5 seconds. | 07:29 |
| fwereade | rog, weird | 07:34 |
| rog | fwereade: for watcher refactoring, i was toying with the idea of something like this: http://paste.ubuntu.com/1048710/ | 07:44 |
| rog | TheMue: good morning! | 07:45 |
| TheMue | rog: Morning rog. | 07:45 |
| rog | fwereade: i'm not entirely sure if it would be worth it though. | 07:46 |
| fwereade | rog, yeah, I was just toying again with a similar idea | 07:46 |
| fwereade | rog, I have a w.loop(w) in there, and that's the thing that feels a bit off really | 07:46 |
| TheMue | Ah, and morning fwereade too. | 07:46 |
| fwereade | TheMue, heyhey | 07:46 |
| rog | fwereade: it doesn't actually factor out that much code. | 07:46 |
| fwereade | rog, yeah | 07:47 |
| rog | fwereade: and it makes the code harder to follow, as callbacks do. | 07:47 |
| fwereade | rog, not so sure, there's a little more initial investment but I think once you have 3 of them it's arguable that it pays off | 07:47 |
| rog | fwereade: and closing the channel is awkward. | 07:47 |
| fwereade | rog, agreed :) | 07:47 |
| rog | fwereade: anyway, worth continuing to think on | 07:49 |
| fwereade | rog, ey up, I'm not sure about MachinesWatcher... is it ok to depend on the topology always containing at least one machine to guarantee the initial event? | 07:56 |
| rog | fwereade: hmm, does it? | 07:56 |
| * rog goes to look. | 07:56 | |
| fwereade | rog, looks like it does to me | 07:56 |
| rog | fwereade: yes, and the test agrees with you | 07:58 |
| rog | fwereade: that needs fixing. i should've seen it. | 07:58 |
| fwereade | rog, I think I reviewed that one as well :) | 07:58 |
| fwereade | rog, programming is hard ;) | 07:58 |
| fwereade | rog, btw, same applies to machine units watcher | 08:04 |
| rog | fwereade: ahem | 08:04 |
| fwereade | rog, or not? | 08:05 |
| rog | fwereade: definitely does | 08:05 |
| rog | fwereade: my fault. i blindly copied from MachinesWatcher. | 08:05 |
| rog | fwereade: without remembering the "first event" thing. | 08:06 |
| fwereade | rog, IMO once the code's reviewed the blame is distributed evenly ;) | 08:06 |
| rog | fwereade: and it's particularly important for machine units watcher too, as the machine agent actually acts specially on the first event. | 08:06 |
| rog | fwereade: good catch! | 08:06 |
| rog | fwereade: feel free to fix 'em both, if you like. | 08:07 |
| rog | :-) | 08:07 |
| fwereade | rog, sure | 08:07 |
| fwereade | rog, https://codereview.appspot.com/6295100 (again trivial IMO) | 08:23 |
| rog | fwereade: LGTM. i think i'd prefer "sentValue" as a name, but i see emittedValue is already used in watcher/watcher.go | 08:26 |
| rog | fwereade: "and -0 on ROW". do you really mean Roswell airport there? | 08:29 |
| fwereade | rog, I meant Rest Of World :p | 08:40 |
| rog | fwereade: ah! | 08:41 |
| fwereade | rog, I suspect I should amend it to -0 on ROW, excluding ROW, which also gets +1; but that would probably be even more confusing | 08:41 |
| rog | fwereade: :-) | 08:42 |
| fwereade | rog, yeah, I'm not too keen on emittedValue, but that was niemeyer's suggestion over "started" so I went with it | 08:42 |
| rog | fwereade: fine. | 08:42 |
| fwereade | rog, trivial enough to be a trivial? | 08:42 |
| rog | fwereade: i think so. | 08:42 |
| fwereade | rog, cool, cheers | 08:42 |
| rog | fwereade: i've just realised that we need some more working juju commands if we're to test the new PA | 08:43 |
| rog | fwereade: deploy! | 08:43 |
| fwereade | rog, I made a start on deploy a while ago but was blocked by lack of AddRelation | 08:44 |
| fwereade | rog, I guess i could switch back to that quickly | 08:44 |
| fwereade | rog, I'm a little reluctant to continue with my pipeline, it depends on the 2 I have out for review | 08:44 |
| fwereade | rog, I'd prefer to get at least one in before I propose another :) | 08:45 |
| rog | fwereade: couldn't we do a minimal deploy with no relations. | 08:45 |
| rog | ? | 08:45 |
| fwereade | rog, my feeling was that half-working features were a no-no | 08:45 |
| rog | fwereade: gotta start somewhere! | 08:45 |
| fwereade | rog, individual features *entirely* not working is fine though ;p | 08:46 |
| fwereade | rog, I think I even have most of it implemented somewhere | 08:46 |
| rog | fwereade: "relation" is a feature :-) | 08:46 |
| fwereade | rog, I'll get on it now :) | 08:46 |
| fwereade | rog, yeah, but deploy is expected to add relevant peer relations | 08:47 |
| fwereade | rog, I guess I could have just done panic("can't deploy services with peer relations yet") | 08:47 |
| rog | fwereade: i'm sure that if you did an initial deploy implementation with "TODO" for the relations stuff, it would be acceptable | 08:47 |
| fwereade | rog, I think I'm actually unblocked on it now | 08:48 |
| rog | fwereade: a trivial for you: https://codereview.appspot.com/6295101 | 09:07 |
| fwereade | rog, LGTM | 09:23 |
| fwereade | rog, do we have a way t get the default series from the environ yet? | 09:53 |
| rog | fwereade: nope. there's no default series parsed in environs/ec2/config.go | 09:54 |
| * fwereade grumbles | 09:56 | |
| fwereade | TODO comment I guess | 09:56 |
| fwereade | rog, half-implementing things really bothers me | 09:56 |
| fwereade | rog, I feel it's one of the biggest risks | 09:56 |
| rog | fwereade: it's not possible to implement everything at once | 09:56 |
| rog | fwereade: i started doing that originally, but there's just too much cruft and you don't know how it's gonna be used | 09:57 |
| fwereade | rog, yeah, but something that *looks* like it works, to a casual inspection, is IMO actually worse than something that doesn't exist at all | 09:57 |
| * rog isn't sure. | 09:58 | |
| rog | i think having something that works at all is useful in itself | 09:58 |
| Aram | moin. | 10:08 |
| fwereade | Aram, heyhey | 10:21 |
| Aram | so, we're nor going to brazil after all... | 10:22 |
| niemeyer | Gooood mornings | 10:27 |
| Aram | heyhey. | 10:30 |
| fwereade | heya niemeyer | 10:35 |
| niemeyer | fwereade, Aram, davecheney: Hey folks! | 10:36 |
| rog | niemeyer: hi | 10:40 |
| niemeyer | rog: Heya | 10:40 |
| * davecheney hates prereqs | 10:48 | |
| Aram | davecheney: I replied to your named return argument note. | 10:49 |
| davecheney | Aram: yup, no worries | 10:50 |
| davecheney | i'm glad we can agree to disagree | 10:51 |
| rog | niemeyer, fwereade, davecheney: here's an initial impression of the machine agent code (untested as yet). http://paste.ubuntu.com/1048896/ | 11:00 |
| rog | niemeyer, fwereade, davecheney: am i missing something? | 11:00 |
| niemeyer | rog: Well, I'd prefer to review it in codereview :) | 11:01 |
| davecheney | rog: line 45, line 60 | 11:01 |
| rog | niemeyer: i have no tests yet | 11:01 |
| rog | niemeyer: actually, i'll propose it as wip | 11:01 |
| fwereade | rog, I don't think it needs to do anything else, honestly | 11:01 |
| niemeyer | rog: Well, write the tests? | 11:01 |
| rog | niemeyer: that's something i want to talk about | 11:02 |
| fwereade | rog, I would suggest that maybe the non-principal filtering should be done at the watcher level... do we expect any other clients? | 11:02 |
| davecheney | rog: niemeyer busted by ass about making it recover if the watcher closes, so its only fair i pass on the love :) | 11:02 |
| rog | niemeyer: my current best idea is to add an exported function pointer to the container package that it will call to do the install. | 11:03 |
| rog | fwereade: no, but i think Machine.UnitsWatcher should mirror Machine.Units | 11:03 |
| fwereade | rog, PrincipalUnitsWatcher? | 11:03 |
| rog | fwereade: PrincipalUnitsForMachineWatcher :-) | 11:04 |
| niemeyer | rog: I'd prefer to have smaller bits going in | 11:04 |
| niemeyer | rog: Well tested | 11:04 |
| niemeyer | rog: and properly reviewed | 11:04 |
| rog | niemeyer: this isn't small? | 11:04 |
| niemeyer | rog: add the skeleton | 11:04 |
| niemeyer | rog: No, this is a paste with no tests | 11:04 |
| rog | niemeyer: it's 100 lines of code... i thought that was a reasonable size. | 11:05 |
| rog | niemeyer: in fact it's only 70 lines of new code. | 11:06 |
| rog | niemeyer: without Machiner.loop, i'm not sure there's any functionality to test | 11:06 |
| niemeyer | rog: If there's nothing to test, I guess we don't need the code..? | 11:07 |
| * rog is feeling a bit shit. | 11:08 | |
| rog | [12:04:14] <niemeyer> rog: I'd prefer to have smaller bits going in | 11:08 |
| niemeyer | rog: Sorry, that was a direct reaction to your statement that you had no tests and seemed unsure about what you were working on | 11:08 |
| niemeyer | rog: I prefer to break a task down further to a point you are sure about than to be reviewing untested code in a paste, that's all.. it wasn't a direct statement about your code | 11:09 |
| rog | niemeyer: i didn't want the code reviewed, just a quick glance for first impressions. | 11:11 |
| niemeyer | rog: Yes, deploy units and destroy units.. that seems right. I'd suggest having a look at some of the early on logic that Dave worked on before getting into those details. | 11:16 |
| davecheney | rog: did you try removing dummy from the PA to see if it fixed the problem ? | 11:16 |
| rog | davecheney: it did | 11:17 |
| rog | davecheney: i proposed the change | 11:17 |
| davecheney | hmmm, y u no email ? | 11:17 |
| davecheney | i'll find it online | 11:18 |
| rog | davecheney: https://codereview.appspot.com/6295101/ | 11:19 |
| davecheney | ta | 11:19 |
| rog | davecheney: i seem to have misspelled "remove" | 11:19 |
| davecheney | LGTM anyway. | 11:20 |
| davecheney | it's night time for me | 11:21 |
| rog | davecheney: see you tomorrow, i hope | 11:21 |
| davecheney | all: i'll be off and on line tomorrow because they are putting in a new floor downstairs, so I have to pack away the router and stuff | 11:22 |
| davecheney | so i'll be on reduced internet rations via phone | 11:22 |
| davecheney | i don't expect this will cause a problem | 11:22 |
| rog | fwereade: i've just realised a not-so-good thing about the watchers | 11:42 |
| rog | fwereade: if the sub-watcher watch channel is closed, the tomb is killed with "content change channel closed unexpectedly" which will actually mask the underlying error returned from the sub-watcher.Stop AFAICS | 11:43 |
| niemeyer | rog: http://etherpad.ubuntu.com/NCUR4J1kbL | 12:21 |
| fwereade | rog, won't the wtahcer.Stop return the underlying error? | 12:34 |
| fwereade | rog, oh, I see | 12:36 |
| fwereade | rog, hmm | 12:36 |
| niemeyer | fwereade: | 12:37 |
| niemeyer | / mustErr panics in case err is not set. | 12:37 |
| niemeyer | / This is used to make sure there's no weirdness in the implementation | 12:37 |
| niemeyer | / of the watcher. Basically, and underlying watcher should never be | 12:37 |
| niemeyer | / *cleanly* stopped if the outer watcher is still expecting it to be alive (duh!). | 12:37 |
| niemeyer | fwereade: Makes sense? | 12:37 |
| fwereade | niemeyer, it does, I'll need to thik a little more to convince myself it's a panic situation | 12:38 |
| fwereade | niemeyer, but offhand, yeah, it does seem like that would indicate a logic error | 12:38 |
| niemeyer | fwereade: Yeah, indeed.. rog mentioned it could return an error as well | 12:40 |
| fwereade | niemeyer, I haven't thought of any problem with panicing yet | 12:41 |
| fwereade | niemeyer, but I'm still a *little* reserved about it | 12:41 |
| fwereade | niemeyer, not enough to say we shouldn't though | 12:41 |
| niemeyer | fwereade: rog had some good points about closing the Changes channels too | 12:43 |
| niemeyer | fwereade: It's silly for us to be closing them.. it's just creating more work | 12:43 |
| niemeyer | Magic! | 12:44 |
| fwereade | niemeyer, I think I'll need a little more convincing there, not sure I totally like the idea of anything sitting watching an abandoned channel forever | 12:44 |
| niemeyer | mramm found a reasonable flight for saturday the 27th, I forwarded it to the agency, and they "discovered" it | 12:44 |
| fwereade | niemeyer, if we give every watch a Dying though I'd be very happy to select on Changes()/Dying() | 12:44 |
| fwereade | nihaha | 12:44 |
| fwereade | niemeyer, haha | 12:44 |
| fwereade | niemeyer, was that what you were you thinking of? | 12:45 |
| niemeyer | fwereade: Exactly | 12:45 |
| fwereade | niemeyer, cool, +1 then | 12:45 |
| niemeyer | fwereade: Although, you do have a point.. | 12:46 |
| niemeyer | fwereade: We could just as well do the same logic with closing the Changes | 12:46 |
| niemeyer | fwereade: if !ok { foo.tomb.Kill(mustErr(foo.watcher.Err())); return } | 12:46 |
| niemeyer | rog: ^ | 12:46 |
| fwereade | niemeyer, hmm, I think that's even better actually | 12:47 |
| hazmat | niemeyer, g'morning | 12:48 |
| niemeyer | hazmat: Heya | 12:48 |
| niemeyer | hazmat: What's up in hot Washington? | 12:48 |
| fwereade | heya hazmat | 12:48 |
| hazmat | niemeyer, there are a couple of charms that afaics should be in the charm store, but aren't.. and its not clear per se why | 12:48 |
| niemeyer | fwereade: Agreed.. no need to be casing twice every time | 12:49 |
| hazmat | pretty good in dc.. wet atm.. waiting to the dog walk | 12:49 |
| hazmat | niemeyer, i put together this page of the missing ones.. http://jujucharms.com/tools/store-missing | 12:49 |
| niemeyer | hazmat: mthaddon is our man in those cases | 12:49 |
| hazmat | some i can see why.. but quite a few its not clear | 12:49 |
| niemeyer | hazmat: He has to dig out of the log for the moment | 12:49 |
| hazmat | gotcha | 12:49 |
| niemeyer | hazmat: We need a proper interface for that, as these details are actually already in the database | 12:50 |
| rog | niemeyer: that LGTM | 12:50 |
| hazmat | niemeyer, well on a few it returns the reason with the response.. | 12:50 |
| hazmat | niemeyer, also i was curious how to use the stats interface, like what's the param | 12:50 |
| * hazmat pulls up charmstore src | 12:50 | |
| hazmat | it looks like its listening on /stats/counter/ and then the param name is the lp branch name? | 12:52 |
| Aram | I need some help with bzr | 13:02 |
| Aram | I used a worng branch by mistake | 13:02 |
| Aram | and I don't know what to do... I created a correct branch with the changes, but I don't know how to fix the original | 13:03 |
| fwereade | Aram, just abandon it I think :) | 13:03 |
| Aram | it's the maste branch though | 13:04 |
| Aram | master | 13:04 |
| Aram | can I delete it and recreate it? | 13:04 |
| Aram | and still keep my other branch? | 13:04 |
| fwereade | Aram, master branch of what? | 13:04 |
| Aram | juju | 13:04 |
| fwereade | Aram, if it's just some branch from which you have already merged the changes you should be fine | 13:04 |
| Aram | it's not | 13:04 |
| fwereade | Aram, ah, lp:juju-core/juju then? | 13:05 |
| fwereade | Aram, what's happened to it? :)_ | 13:05 |
| Aram | yes | 13:05 |
| Aram | I commited to my local copy of it. | 13:05 |
| Aram | some changes. | 13:05 |
| rog | Aram: i'm +1 with dfc on named return values FWIW. i think they should only be used for trivial functions - they're very easy to get wrong. | 13:06 |
| fwereade | Aram, hmm, I think there's a "bzr uncommit" | 13:06 |
| rog | Aram: (and that's also the policy in the go core i think) | 13:06 |
| fwereade | Aram, http://doc.bazaar.canonical.com/beta/en/user-guide/undoing_mistakes.html has helped me once or twice | 13:07 |
| fwereade | Aram, better I think is to get into the habit of branching for *everything* | 13:07 |
| fwereade | ;) | 13:07 |
| rog | Aram: for reference: http://codereview.appspot.com/2125042/diff/15006/src/pkg/archive/zip/reader.go#newcode85 | 13:11 |
| Aram | rog: I agree with rsc if s/trivial/small/. in that example named returns are bad because they force you to do 'return nil, err' in the middle of a huge function, but if the function is such that an unadorned return always suffices I see no problem with them. | 13:14 |
| Aram | fwereade: bzr uncommit worked for fixing that branch, thanks. | 13:16 |
| Aram | I still have another problem though. | 13:16 |
| fwereade | Aram, sweet | 13:16 |
| fwereade | Aram, go on | 13:16 |
| rog | Aram: i think it's worth using named return values if they actively add value. often that's not actually the case, and they force the reader to go back through the code to work out what's actually being returned. | 13:16 |
| Aram | what's the equivalent of hg update in bzr? | 13:16 |
| rog | Aram: bzr pull? | 13:16 |
| Aram | I though so, but then why: | 13:17 |
| Aram | bzr: ERROR: These branches have diverged. Use the missing command to see how. | 13:17 |
| rog | Aram: maybe you want to do a merge | 13:17 |
| Aram | what does that mean, there are no conflicts. | 13:17 |
| rog | Aram: bzr merge lp:juju-core/juju | 13:17 |
| Aram | bzr merge worked, but | 13:17 |
| Aram | zr merge lp:juju-core/juju | 13:18 |
| Aram | oops | 13:18 |
| Aram | white:state$ bzr diff | wc | 13:18 |
| Aram | 875 3448 23475 | 13:18 |
| Aram | do I have to commit after merge? | 13:18 |
| rog | Aram: yes | 13:18 |
| Aram | hmm | 13:18 |
| rog | Aram: i usually do: bzr commit -m 'merge trunk' | 13:18 |
| Aram | ok, thanks. | 13:18 |
| Aram | all fine now. | 13:19 |
| Aram | rog: yes, I agree that named return values should be used only if they add value, but I think they can add a lot of value if the programmer writes the code in a particular way. | 13:19 |
| rog | Aram: example: Machine.SetInstanceId. what value is the named return value adding there? | 13:21 |
| Aram | rog: none, I agree in that case. | 13:24 |
| rog | Aram: another example. here i would definitely prefer the second version: http://paste.ubuntu.com/1049099/ | 13:26 |
| rog | Aram: at a glance it's obvious that we're returning the correct combination of machine and error | 13:26 |
| rog | Aram: in the first version i have to look at the if err != nil logic to make sure that it's correctly returning a nil error. | 13:27 |
| Aram | rog: but if err != nil { return } is such a common idiom that by convention you know what it does, error out using the result from the previous function and code past it means it's all fine. it's like a for loop, you learn how it looks, you don't have to think about it. | 13:29 |
| rog | Aram: it's the bare return at the end i was talking about. | 13:29 |
| Aram | yes, it's past the if err != nil { return } so you know err is nil. | 13:30 |
| rog | Aram: exactly, you have to look back into the function to check that it's returning a nil error | 13:30 |
| rog | Aram: for me, the second version has less code, is more trivially readable, and the documentation looks nicer too. | 13:31 |
| Aram | but with things like return &Machine{st: s, id: id}, nil I always have to look to match up return parrameters with the return types to figure out what am I doing and if it's right, if it's a bare return I don | 13:31 |
| Aram | 't have to think about it. | 13:31 |
| Aram | and without named returns, error paths usually end up like 'return nil, err' or 'return "", err', and I always have to think about who is that nil or "". | 13:33 |
| Aram | and return &Machine{st: s, id: id}, nil makes me to fill up a mental stack frame processing &Machine{st: s, id: id} and then the return, whilst in my version the code is written in the same natural order the mind processes it (do the Machine, then return). | 13:35 |
| Aram | it's the same reason I don't like 'if ok := f(); ok {}', I like them separate. | 13:36 |
| Aram | ok := f(); if ok {} | 13:36 |
| rog | Aram: there are almost always exactly two return values... | 13:39 |
| rog | Aram: so matching return values with types isn't too onerous | 13:39 |
| rog | Aram: i think my mind can process &Machine{...} as a blob to go along with the nil | 13:40 |
| niemeyer_ | Aram: FWIW, I agree with rog in the original paste | 13:40 |
| niemeyer_ | Aram: This seems like overusing named results to me as well | 13:41 |
| hazmat | Aram, that's only safe to do when have a branch of trunk and not a checkout re uncommit | 13:41 |
| niemeyer_ | (that, specifically: http://paste.ubuntu.com/1049099/) | 13:41 |
| === niemeyer_ is now known as niemeyer | ||
| niemeyer | The second seems both shorter and reads more clearly | 13:42 |
| rog | Aram: in the "if ok := ..." case, i use the one line version when possible because it means i have to check less places to see if something relies on the result of ok. | 13:42 |
| rog | niemeyer: this is a one-line trivial change: https://codereview.appspot.com/6295101/ | 13:58 |
| niemeyer | Looking | 13:58 |
| niemeyer | rog: LGTM | 13:58 |
| rog | niemeyer: thanks | 13:58 |
| niemeyer | rog: With that kind of change you can just go ahead if you have someone else's review | 13:59 |
| rog | niemeyer: it was interesting that it broke the agent. | 13:59 |
| rog | niemeyer: it made me think that it would be nice if we had something that ssh'd in to the bootstrap machine and tailed the logs so we could automatically get feedback when a bootstrap fails. | 14:04 |
| rog | niemeyer: but i think that's too much pain for not enough gain | 14:04 |
| niemeyer | rog: I've been thinking about something along those lines too | 14:28 |
| niemeyer | rog: Once we get it on pair, we should look at doing some debugging improvements | 14:28 |
| niemeyer | rog: I think we should make better use of syslog, though | 14:29 |
| niemeyer | rog: To deliver *all* logs onto a central location | 14:29 |
| rog | niemeyer: agreed. but in this case it died with a panic in init. | 14:29 |
| niemeyer | rog: So we can say "juju log" and get a tail of it aall going | 14:29 |
| rog | niemeyer: before we'd have managed to set up the log, i think. | 14:29 |
| rog | niemeyer: yeah, it would be fantastic to have a centrally tailable log. | 14:30 |
| niemeyer | rog: I see.. that doesn't concern me as much.. I'm happy to have juju ssh for that | 14:30 |
| niemeyer | rog: We should probably do some early use of papertrail to get started | 14:32 |
| rog | niemeyer: papertrail? | 14:32 |
| niemeyer | rog: papertrailapp.com | 14:32 |
| rog | niemeyer: looks good | 14:33 |
| niemeyer | rog: If we make juju set up the machine syslog up, which is on the trivial side, we can use their service while we don't have our own syslog receiver | 14:33 |
| niemeyer | rog: We may actually never *have* to have a syslog receiver.. just deploy a syslog charm as a first step, and then do "juju set-env syslog=<address>" or whatever | 14:34 |
| niemeyer | rog: This should be a big aid for us | 14:34 |
| rog | niemeyer: a syslog subordinate charm? | 14:34 |
| niemeyer | rog: No, a syslog server charm | 14:35 |
| niemeyer | rog: We should probably have a basic syslog setup within juju itself, so we can do debugging across the board more easily | 14:35 |
| niemeyer | rog: It's really quite simple to have syslog sending logs from one machine onto the server | 14:35 |
| niemeyer | rog: The machine agent could just do that | 14:36 |
| niemeyer | I have flights for Lisbon, for the full week, thanks to mramm | 14:36 |
| Aram | great, so it's the original date? | 14:37 |
| * rog will go and try to book flights | 14:37 | |
| rog | niemeyer: when should i plan to leave? | 14:39 |
| rog | niemeyer: friday evening? or sat morning? | 14:40 |
| niemeyer | rog: Late Fri or anytime Sat both are great | 14:40 |
| niemeyer | Aram: Yeah | 14:40 |
| niemeyer | Aram: The change didn't quite worked out, and in the end there *was* a flight returning on the proper day that the agency I was talking to "didn't see" (!?) | 14:41 |
| Aram | heh | 14:41 |
| niemeyer | rog: Sent a couple of ideas on https://codereview.appspot.com/6312044/ | 14:43 |
| rog | niemeyer: seen. that's useful thanks. | 14:43 |
| niemeyer | rog: Glad to hear | 14:43 |
| * rog is booking flights now. | 14:45 | |
| Aram | so we don't use BTS or anything like that, we book our own flights? | 14:47 |
| * Aram is a little bit confused | 14:47 | |
| fwereade | mramm, are we meeting in 10, or do I have time zones confused? | 14:48 |
| niemeyer | Aram: We use BTS | 14:55 |
| Aram | niemeyer: ah, so I should contact BTS? | 14:55 |
| niemeyer | Aram: Yeah | 14:55 |
| Aram | niemeyer: ok, thanks, understood. | 14:56 |
| niemeyer | Aram: We as in you, me, and a few others.. depends on where the person is | 14:56 |
| niemeyer | Aram: We have a few different agencies | 14:56 |
| niemeyer | Aram: You can just get in touch with that email that I've CCd you in using your Canonical email address | 14:56 |
| niemeyer | Aram: Point out dates, location, and event in case they have to confirm | 14:57 |
| Aram | thanks | 14:57 |
| === bcsaller1 is now known as bcsaller | ||
| === Guest____ is now known as smaddock | ||
| hazmat | smaddock, bcsaller invitations out | 15:01 |
| smaddock | hazmat: thanks, jointing now | 15:02 |
| rog | niemeyer: i think i've addressed those issues now. https://codereview.appspot.com/6312044/ | 16:05 |
| niemeyer | rog: Super, I'll have a look as soon as I returned from lunch.. just got a call from Ale about it | 16:06 |
| rog | niemeyer: ok | 16:06 |
| hazmat | bcsaller, incidentally not sure if you saw the lp, but i assigned you the lp bug re lxc | 16:24 |
| bcsaller | hazmat: yes | 16:24 |
| fwereade | rog, TheMue, niemeyer: I seek advice re testing deploy with the charm store; don't want to hit real cs:, don't know how I should intercept requests (or replace url from outside package, or whatever). suggestions? | 17:05 |
| rog | fwereade: chat about it tomorrow morning? | 17:19 |
| rog | i'm off for the evening | 17:19 |
| fwereade | rog, sure, sounds good, thanks :) | 17:19 |
| rog | niemeyer: another small CL: https://codereview.appspot.com/6312051/ | 17:20 |
| fwereade | rog, happy evening :) | 17:20 |
| rog | fwereade: toi aussi | 17:20 |
| niemeyer | rog: Just done https://codereview.appspot.com/6312044/ | 17:21 |
| niemeyer | rog: Reviewed too | 17:25 |
| niemeyer | rog: Just wondering about whether it should follow our established conventions | 17:25 |
| niemeyer | rog: fooFixture/setup/teardown is generally FooSuite/SetUpSuite/TearDownSuite | 17:26 |
| fwereade | gn all, might make it on a bit later, or might not | 17:28 |
| niemeyer | fwereade: have a good one | 17:32 |
| jimbaker | hazmat, please see my comments re https://codereview.appspot.com/6308044/ | 22:10 |
| jimbaker | sadly the choice of using a json transport seems to be essential to preserving the format: 1 bugginess | 22:10 |
| jimbaker | when i restructured the code so the conditional logic was removed in favor being polymorphic, it might have made the comment in juju.lib.format.Format1 re JSON serialization too far away from what was going on | 22:13 |
| jimbaker | hazmat, but that's why there is dump/load methods in the Format classes, otherwise would just have used yaml.safe_load/safe_dump | 22:13 |
| fwereade | niemeyer, fwiw, I have a couple of fixes and a couple of counterarguments on https://codereview.appspot.com/6305107/ | 22:50 |
| fwereade | niemeyer, if you're at a loose end (har har) I'd appreciate your thoughts :) | 22:50 |
| niemeyer | fwereade: Checking :) | 23:31 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!