| axw | wallyworld_: do you know why bootstrap forces --upload-tools for the local provider? | 02:11 |
|---|---|---|
| wallyworld_ | axw: it doesn't distinguish, so if it doesn't need to for the local provider, that needs to be added in | 02:12 |
| wallyworld_ | it was an oversight on my part i think | 02:12 |
| wallyworld_ | axw: although local provider has always been funky with tools | 02:13 |
| axw | wallyworld_: what I mean is, in cmd/juju/bootstrap.go, it goes: if provider == 'local': uploadTools = true | 02:13 |
| axw | I just don't understand why. | 02:13 |
| wallyworld_ | ah ok. that was deliberate then, by tim | 02:13 |
| axw | yeah | 02:13 |
| axw | don't know why? :) | 02:13 |
| axw | convenience? | 02:14 |
| wallyworld_ | i think it's because the jujud needs to be put in the local provider's storage | 02:14 |
| wallyworld_ | so it can find it | 02:14 |
| wallyworld_ | cause local provider does not want to have to use streams.canonical.com etc | 02:14 |
| wallyworld_ | so the local provider has been treated as an edge case | 02:14 |
| wallyworld_ | but it is known to be a dirty great hack | 02:15 |
| axw | why not use streams.canonical.com like the others? | 02:15 |
| axw | it can be overridden with --upload-tools | 02:15 |
| wallyworld_ | when it was written, there was no streams,canonical.com. there was an s3 bucket and you needed credentials for that | 02:15 |
| wallyworld_ | so it was done out of necessity | 02:16 |
| axw | ok | 02:16 |
| wallyworld_ | we can probs revisit that now | 02:16 |
| wallyworld_ | s/probs/should :-) | 02:17 |
| axw | hm, I removed it and get errors about not finding 1.17 at s3. that could be due to some other changes I have going on tho | 02:19 |
| wallyworld_ | i think sync tools used to look at s3 | 02:20 |
| wallyworld_ | and that's what upload-tools uses | 02:20 |
| wallyworld_ | if are running the dev version, there are no 1.17 tools | 02:20 |
| wallyworld_ | s/dev/from source | 02:20 |
| axw | no, but it should upload the local ones as a last resort | 02:21 |
| wallyworld_ | so that's the other issue - running from source also requires a upload tools | 02:21 |
| wallyworld_ | it should have i think yes | 02:21 |
| axw | I'm making providers responsible for selecting bootstrap tools | 02:21 |
| axw | must've broken it | 02:21 |
| wallyworld_ | :-) | 02:21 |
| axw | wallyworld_: wasn't my code- the local provider explicitly sets agent-version to version.Current if it doesn't have agent-version already | 02:40 |
| wallyworld_ | ok | 02:40 |
| axw | which stops the auto-upload stuff | 02:41 |
| wallyworld_ | if there a agent-version check somewhere? | 02:41 |
| axw | yeah, in the current code as well. It won't try to auto-upload if the user has specified agent-version | 02:41 |
| axw | we could perhaps be a bit smarter, and check if agent-version is the same as what's on disk | 02:42 |
| wallyworld_ | i wonder why that code is there | 02:42 |
| axw | but ... I'm not convinced the local provider needs all this special stuff | 02:42 |
| axw | so you don't accidentally get a version you didn't ask for, I guess | 02:42 |
| wallyworld_ | i know there was a lot of hair pulling at the time, but am not sure of the details | 02:42 |
| wallyworld_ | would be worth a chat with tim when he's back to get the back story on it all | 02:43 |
| axw | yup definitely | 02:43 |
| jam | sinzui: it is "tsv" as tab-separated-value so it wasn't supposed to be changed | 07:04 |
| fwereade | axw, heyhey | 07:09 |
| fwereade | I'm around if you are | 07:09 |
| axw | fwereade: morning | 07:09 |
| axw | I am | 07:09 |
| axw | brb, just getting a drink | 07:10 |
| fwereade | axw, actually I'm around in 5 mins after a quick ciggie | 07:10 |
| fwereade | axw, you're too quick for my morning brain ;) | 07:10 |
| axw | nps | 07:10 |
| axw | fwereade: I can't hear you if you're talking | 07:31 |
| * fwereade is off to get up and run a couple of errands, bbiab | 07:49 | |
| axw | wallyworld_: would you be able to review this for me next week? https://codereview.appspot.com/14433058/ | 07:49 |
| axw | (please) :) | 07:49 |
| rogpeppe1 | mornin' all | 08:06 |
| wallyworld_ | axw: sure, will do | 08:59 |
| axw | hey rogpeppe1, thanks for the review. let me know if you have any thoughts about my response (particularly around the ConfigureBase name) | 09:12 |
| rogpeppe1 | axw: looking | 09:12 |
| rogpeppe1 | axw: the underlying problem, i think, is that it ties together an random bunch of stuff, so it's difficult to think of a nicely descriptive name for it | 09:14 |
| axw | rogpeppe1: what's the alternative? ConfigureAuthorizedKeys and ConfigureOutput? | 09:15 |
| * rogpeppe1 goes to look at the manual provider case | 09:16 | |
| axw | maybe we don't even both with that method, and have provider/common do it. | 09:16 |
| axw | bother* | 09:17 |
| axw | ah, but provisioning non-bootstrap machines needs it too | 09:17 |
| axw | rogpeppe1: manual provider only uses ConfigureJuju | 09:17 |
| rogpeppe1 | axw: hmm, that's interesting in itself - where does its cloudinit output end up? | 09:18 |
| axw | rogpeppe1: it doesn't use cloud-init | 09:18 |
| axw | all output goes to stdout | 09:18 |
| axw | which comes back to the SSH client | 09:18 |
| rogpeppe1 | axw: if it doesn't use cloud-init, what's it doing with ConfigureJuju? | 09:18 |
| axw | rogpeppe1: it translates the cloudinit.Config into a script. there's an open bug to update cloud-init to have a way of invoking manually/interactively | 09:19 |
| rogpeppe1 | axw: oh gosh | 09:20 |
| rogpeppe1 | axw: where does that translation happen? | 09:20 |
| axw | rogpeppe1: currently, environs/manual/agent.go; I have a followup CL that moves this to a new package, cloudinit/sshinit | 09:21 |
| axw | and makes it sufficiently generic of course | 09:21 |
| rogpeppe1 | axw: well, in that case, it would be fine for the manual provider to use exactly the same base config, no? | 09:22 |
| rogpeppe1 | axw: because it will ignore what it doesn't care about | 09:22 |
| axw | rogpeppe1: depends on whether we want to do different things in ConfigureBase (like run additional commands) | 09:22 |
| rogpeppe1 | axw: if we do, then we probably want those commands to work in the manual provider too, i'd guess | 09:23 |
| axw | rogpeppe1: hmm perhaps, yes. actually, now that I think of it... there's probably no good reason to split them anymore | 09:23 |
| rogpeppe1 | axw: this is part of the problem with the name "base" - it implies it's "base" for everything, but it's really "base for everything other than the manual provider" | 09:24 |
| axw | rogpeppe1: I went through a few iterations here | 09:24 |
| rogpeppe1 | axw: sure | 09:24 |
| axw | I'll ponder a bit | 09:24 |
| rogpeppe1 | axw: it kinda feels trivial, but it's also the kind of thing that can grow in complexity | 09:24 |
| axw | yup, fair enough | 09:24 |
| rogpeppe1 | axw: so i'd like us to get the abstractions right here | 09:24 |
| axw | it's confusing enough as it is anyway :) | 09:25 |
| rogpeppe1 | axw: yeah | 09:25 |
| axw | so many different interactions | 09:25 |
| rogpeppe1 | axw: it's one of those places where the code with common pieces factored out is arguably harder to read and maintain than the original copypasta. | 09:33 |
| axw | rogpeppe1: I lied apparently, anyway. I didn't end up using ConfigureJuju in environs/manual. I was going to use it in the followup, but I can use the existing one after all | 09:33 |
| rogpeppe1 | fwereade: is it possible you could have a glance through this doc for sanity checking, please? https://docs.google.com/a/canonical.com/document/d/1vZrav8Do80Lnk1uFNdVVV2jRqpEH99wcX3YWErPwWIg/edit | 09:43 |
| rogpeppe1 | fwereade: nate has a couple of reasonable comments, but i don't want to change things away from your suggestions without your goahead | 09:44 |
| axw | rogpeppe1: LGTM stands with the two functions joined? | 09:50 |
| rogpeppe1 | axw: i think so. what's actually changed in that case then? | 09:51 |
| axw | rogpeppe1: just the signature (it was a bit funky - took an arg that's modified and returned it), and dropped the unnecessary New function | 09:51 |
| axw | the file could be reverted, but I think it's a bit cleaner now | 09:52 |
| rogpeppe1 | axw: looks like you haven't proposed the changes yet | 09:52 |
| axw | rogpeppe1: it was a hypothetical question, but the changes are coming.. super slow from here | 09:55 |
| rogpeppe1 | axw: ah, that's fine. in principle it all sounds great. | 09:56 |
| axw | rogpeppe1: updated now | 09:56 |
| TheMue | interesting behavior, first eth0 closed, later whole instance terminated, but netstat still sees an established connection :( | 10:00 |
| rogpeppe1 | axw: reviewed | 10:02 |
| axw | thanks rogpeppe1, good suggestions - will do them before landing | 10:03 |
| rogpeppe1 | axw: thanks | 10:03 |
| axw | I mean to use US spelling, it's just automatic | 10:03 |
| rogpeppe1 | axw: yeah - i try to use the US spelling everywhere just for consistency with the go std lib | 10:04 |
| axw | I defied it at IBM for a long time - but that was internal code | 10:04 |
| axw | yup, fair enough | 10:04 |
| rogpeppe1 | axw: i will still use initialise and colour everywhere *except* in the source tree :-) | 10:05 |
| axw | alright, that's it for me. have a nice weekend all | 10:10 |
| rogpeppe1 | axw: have a good one | 10:10 |
| === rogpeppe1 is now known as rogpeppe | ||
| fwereade | rogpeppe, can I trouble you for a review of https://codereview.appspot.com/26100043/ ? I am catching up to the point I can review your doc I think | 10:26 |
| rogpeppe | fwereade: looking | 10:27 |
| fwereade | rogpeppe, mgz is ocr but I can't see him | 10:27 |
| fwereade | rogpeppe, cheers | 10:27 |
| rogpeppe | fwereade: are there any pieces that i should look at in particular (i presume it's all been reviewed before when it went onto trunk) ? | 10:28 |
| rogpeppe | fwereade: in particular, are there any bits you made some manual changes in, rather than just running patch? | 10:30 |
| fwereade | rogpeppe, the hack in jujud/machine_test.go is the only bit that hasn't been reviewed individually | 10:34 |
| rogpeppe | fwereade: is all the code in state/cleanup.go new, or is it just moved from elsewhere? | 10:35 |
| rogpeppe | fwereade: ah, i see; some's new. | 10:37 |
| fwereade | rogpeppe, I'm pretty certain all the revs I cherrypicked were mentioned in the message so there is at least some route back to the original reviews | 10:39 |
| rogpeppe | fwereade: ah i see. in future it would be good if those were codereview links | 10:40 |
| rogpeppe | fwereade: or MP links anyway | 10:40 |
| rogpeppe | fwereade: reviewed | 10:42 |
| dimitern | rogpeppe, fwereade, wallyworld_ : standup | 10:47 |
| === 3JTAADOM8 is now known as wallyworld | ||
| === wallyworld is now known as Guest37360 | ||
| rogpeppe | dimitern: https://codereview.appspot.com/22100045/ | 11:09 |
| dimitern | rogpeppe, cheers | 11:10 |
| rogpeppe | can anyone explain to me how this line of code actually works? /home/rog/src/go/src/launchpad.net/juju-core/state/environ.go:34 | 11:17 |
| dimitern | rogpeppe, err := st.environments.Find(D{{"uuid", D{{"$ne", ""}}}}).One(&doc) ? | 11:18 |
| rogpeppe | dimitern: yeah | 11:18 |
| rogpeppe | dimitern: i suspect it only works by accident | 11:18 |
| dimitern | rogpeppe, what's wrong with it | 11:18 |
| rogpeppe | dimitern: there's no uuid field in the document | 11:18 |
| dimitern | rogpeppe, ha, really? | 11:19 |
| rogpeppe | dimitern: i think it only works because "" != nil | 11:19 |
| dimitern | rogpeppe, and it's always "" | 11:19 |
| rogpeppe | dimitern: yeah - there's a UUID field in the struct, but it's renamed to _id | 11:19 |
| rogpeppe | dimitern: it's always nil | 11:19 |
| rogpeppe | dimitern: because it doesn't exist | 11:19 |
| dimitern | rogpeppe, so this block returns err == nil | 11:20 |
| dimitern | rogpeppe, and works, i've tried | 11:20 |
| rogpeppe | dimitern: it'll correctly fetch the only environment document | 11:21 |
| dimitern | rogpeppe, what if there are more than one? | 11:21 |
| rogpeppe | dimitern: it would fetch the first | 11:21 |
| dimitern | rogpeppe, this looks like a lurking bug | 11:22 |
| rogpeppe | dimitern: +1 | 11:22 |
| dimitern | rogpeppe, good catch | 11:22 |
| rogpeppe | dimitern: i'm not sure why there's any search condition in there at all, tbh | 11:22 |
| rogpeppe | dimitern: i don't know any case where we'd insert a document with an empty uuid | 11:23 |
| dimitern | rogpeppe, because I think when environs were added to the schema it was unclear how to refer to them | 11:23 |
| dimitern | rogpeppe, or that changed later with the uuid perhaps | 11:23 |
| fwereade | rogpeppe, what's the latest on that panic of davecheney's? Irecall you talking about it yesterday but I hadother thingsonmy mind | 11:24 |
| rogpeppe | dimitern: really i think we should hand the uuid to the state when opening an environment | 11:24 |
| rogpeppe | fwereade: the gccgo issue? | 11:24 |
| dimitern | rogpeppe, TheMue, that's in rev 1122 | 11:24 |
| rogpeppe | fwereade: it's much less of a problem than we thought | 11:24 |
| fwereade | rogpeppe, I have no context on it, but it's targeted for 1.16 | 11:24 |
| fwereade | rogpeppe, does it not need to be? | 11:24 |
| rogpeppe | fwereade: it is a gccgo bug though, and we should work around it | 11:25 |
| rogpeppe | fwereade: for the time being | 11:25 |
| rogpeppe | fwereade: the problem only happens when the gccgo runtime tries to call a function/method that returns an empty struct | 11:25 |
| rogpeppe | fwereade: i think there's probably only one such method (Pinger) | 11:26 |
| rogpeppe | fwereade: and we can trivially work around the issue by including a dummy field (e.g. _ bool) | 11:26 |
| rogpeppe | fwereade: in the srvPinger struct type | 11:26 |
| rogpeppe | fwereade: it seems like a good candidate for backporting to 1.16 (a 1 line change) | 11:27 |
| fwereade | rogpeppe, sorry, I'm thinking of https://bugs.launchpad.net/juju-core/+bug/1227952 | 11:27 |
| _mup_ | Bug #1227952: juju get give a "panic: index out of range" error <regression> <goyaml:Fix Committed by dave-cheney> <juju-core:In Progress by dave-cheney> <juju-core 1.16:In Progress by dave-cheney> <https://launchpad.net/bugs/1227952> | 11:27 |
| fwereade | rogpeppe, (is anyone building 1.16 with gccgo?) | 11:28 |
| rogpeppe | fwereade: ah, that was a goyaml bug which is now fixed | 11:28 |
| rogpeppe | fwereade: i dunno | 11:28 |
| rogpeppe | fwereade: the 1.16 deps would need updating | 11:28 |
| rogpeppe | fwereade: that's all, i think | 11:29 |
| rogpeppe | afk | 11:31 |
| rogpeppe | back | 11:45 |
| === gary_poster|away is now known as gary_poster | ||
| * rogpeppe wishes it took less than 3 minutes to run lbox propose | 13:16 | |
| rogpeppe | mgz, natefinch, fwereade: first step towards keeping track of state servers: https://codereview.appspot.com/26880043 | 13:21 |
| * rogpeppe goes for lunch | 13:22 | |
| mgz | rogpeppe: looking | 13:23 |
| rogpeppe | mgz: did you have some queries about it? | 14:00 |
| rogpeppe | fwereade: i would really appreciate your feedback too, please, as i don't want to go up another blind alley | 14:01 |
| natefinch | rogpeppe: I'm looking, but need a little more time with it... once my two very cute distractions are with their mother, I'll be able to give it more attention | 14:01 |
| rogpeppe | natefinch: ok | 14:01 |
| natefinch | rogpeppe: which will be shortly | 14:01 |
| rogpeppe | can anyone think of a nicer way to test (in mongo) if the length of an array is odd than this? $where: function() { return this.stateservers.length % 2 == 1} | 14:41 |
| mgz | that seems reasonable | 14:41 |
| rogpeppe | i don't think it's a problem in this case, but it would be nice to use built-in ops if possible | 14:42 |
| rogpeppe | mgz: did you manage to review the above CL, BTW? | 14:42 |
| rogpeppe | mgz: 't'would be much appreciated if you could | 14:42 |
| mgz | rogpeppe: yeah, getting there... sorry | 14:42 |
| rogpeppe | mgz: np | 14:42 |
| natefinch | rogpeppe: I'm in the middle of it too. Looks fine so far. | 14:42 |
| rogpeppe | natefinch: thanks | 14:43 |
| natefinch | rogpeppe: also, %2 == 1 is pretty standard "is odd" ... seems unlikely there'd be a built-in op, but I don't know for sure | 14:43 |
| rogpeppe | natefinch: well, there's a built-in $mod operation | 14:43 |
| rogpeppe | natefinch: but i'm not sure there's a way to apply it to the length of an array | 14:43 |
| natefinch | rogpeppe: doesn't look like it's possible | 14:50 |
| rogpeppe | natefinch: yeah | 14:50 |
| rogpeppe | natefinch: as it turns out, i don't need to do it anyway... :-) | 14:51 |
| natefinch | rogpeppe: even better | 14:52 |
| natefinch | good god state_test.go is too big | 14:54 |
| mgz | rogpeppe: reviewed, just some side questions due to my shakey understanding of state management bits. | 14:56 |
| rogpeppe | natefinch: yeah, State is big | 14:56 |
| rogpeppe | mgz: ta! | 14:57 |
| rogpeppe | fwereade: could we have a chat about this? | 15:09 |
| fwereade | rogpeppe, ofc | 15:09 |
| rogpeppe | fwereade: https://plus.google.com/hangouts/_/calendar/am9obi5tZWluZWxAY2Fub25pY2FsLmNvbQ.mf0d8r5pfb44m16v9b2n5i29ig?authuser=1 | 15:09 |
| rogpeppe | fwereade: i had to reboot, sorry | 15:38 |
| rogpeppe | fwereade: back there now | 15:38 |
| sinzui | rogpeppe, natefinch : do either you you have a few minutes to review this dep change: https://codereview.appspot.com/26940043 | 15:50 |
| rogpeppe | sinzui: looking | 15:50 |
| rogpeppe | sinzui: looks like it should be rev 50, not 49 | 15:54 |
| sinzui | really? | 15:54 |
| sinzui | rogpeppe, is tip also wrong? I can set that to 50 | 15:55 |
| rogpeppe | sinzui: yeah, that would be better, i think (rev 50 altered the rev 49 fix slightly) | 15:55 |
| mgz | rogpeppe: was trunk bumped to r50? | 15:56 |
| rogpeppe | mgz: yeah | 15:56 |
| rogpeppe | mgz: i mean, no | 15:56 |
| rogpeppe | mgz: goyaml trunk was | 15:56 |
| rogpeppe | sinzui: BTW I'm a bit surprised there's no test case to go along with this | 15:56 |
| sinzui | rogpeppe, yep. We see it in CI though. the lander would do if it honoured godeps | 15:57 |
| rogpeppe | sinzui: ah, that's good | 15:58 |
| sinzui | rogpeppe, I am focused on a release today, but I or someone on juju-qa can get such a test by next week. | 15:58 |
| sinzui | such a test would have caught the spaces yesterday | 15:59 |
| rogpeppe | sinzui: it sounds like there already is a test, if tests now pass in CI but did fail before this fix | 15:59 |
| rogpeppe | sinzui: ah, but CI doesn't just run the tests, gotcha | 15:59 |
| rogpeppe | sinzui: yeah, i think this should probably have a test case, but i won't hold up this change for that | 16:00 |
| sinzui | rogpeppe, we are thinking about adding the test runner because it offers a sanity check that the integration tests have a sane start | 16:00 |
| rogpeppe | sinzui: yeah, i think that's probably worth doing | 16:01 |
| rogpeppe | sinzui: reviewed | 16:01 |
| rogpeppe | fwereade, natefinch, mgz: PTAL https://codereview.appspot.com/26880043 | 16:18 |
| rogpeppe | natefinch: any chance of a LGTM ? | 16:37 |
| rogpeppe | hmm, i thought that instance.HardwareCharacteristics was a result, not a parameter | 16:44 |
| rogpeppe | but in state.AddMachineParams it seems to be being used as a kind of constraint | 16:44 |
| fwereade | rogpeppe, it's not aconstraint -- it's there for injected machines iirc | 16:46 |
| rogpeppe | fwereade: ah, ok | 16:46 |
| fwereade | rogpeppe, it's the result of what we got, that should hopefully match the constraints | 16:47 |
| rogpeppe | fwereade: i'm wondering whether i should be using addMachineOps or addMachineContainerOps when creating new state server machines | 16:47 |
| fwereade | rogpeppe, the former, almost certainly | 16:47 |
| fwereade | rogpeppe, and please don't be afraid to massage them into a better shape if necessary | 16:48 |
| rogpeppe | fwereade: i couldn't see how point 2) in the comment for addMachineContainerOps didn't also apply to creating state server machines | 16:49 |
| rogpeppe | fwereade: and since addMachine itself calls addMachineContainerOps, i thought perhaps i should too. | 16:51 |
| * rogpeppe still doesn't really understand the containers stuff | 16:52 | |
| fwereade | rogpeppe, it looks like addMachineContainerOps is *actually* there to create a parent machine if it's necessary and doesn't already exist,but Imay be misskimming | 16:53 |
| rogpeppe | fwereade: that seems plausible | 16:53 |
| fwereade | rogpeppe, it's not helpfully named | 16:53 |
| rogpeppe | fwereade: in which case, I'd only use it when we come to do --to (or whatever it's called) | 16:54 |
| fwereade | rogpeppe, and would surely benefit from a smart person WTFing at it a bit and probably renaming some bits for clarity ;) | 16:54 |
| rogpeppe | fwereade: i'm not smart enough | 16:54 |
| fwereade | rogpeppe, sell not yourself short dude | 16:55 |
| fwereade | rogpeppe, but it's not a demand anyway ;) | 16:55 |
| fwereade | rogpeppe, I'm EOD, and I'm afraid I need to be off pretty sharpish -- we are out of booze that cath feels like drinking ;p | 16:55 |
| rogpeppe | fwereade: just one thing: | 16:56 |
| rogpeppe | fwereade: i'm presuming it should be started with series taken from environ config default series, right? | 16:56 |
| fwereade | rogpeppe, sure (and I'll try to stick my head back in later as well so leave other questions as necessary, someone else might know the answers in the meantime) | 16:56 |
| rogpeppe | fwereade: i suggest you alleviate the booze emergency a.s.a.p BTW :-) | 16:57 |
| fwereade | rogpeppe, that sounds like a good start | 16:57 |
| fwereade | rogpeppe, in future --to a machine that isn't default-series should probably be fine too though | 16:57 |
| fwereade | rogpeppe, cheers :) | 16:57 |
| fwereade | happy weekends all | 16:57 |
| rogpeppe | fwereade: have a good one | 16:57 |
| === TheFreeMue is now known as TheMue | ||
| natefinch | TheMue: how old are your kids? | 17:04 |
| TheMue | natefinch: 17 and 19 | 17:05 |
| * TheMue just fights a bit with his network :( | 17:05 | |
| rogpeppe | natefinch: any chance of another look at the https://codereview.appspot.com/26880043; i'd like to land it tonight if possible, and i haven't got that long | 17:09 |
| natefinch | rogpeppe: yep, looking | 17:16 |
| rogpeppe | natefinch: thanks | 17:16 |
| natefinch | rogpeppe: btw, you can use jc.SameContents to compare to unordered lists | 17:22 |
| rogpeppe | natefinch: ah, i'd forgotten that | 17:22 |
| rogpeppe | natefinch: given that sort.Strings is easy at this point, i might just leave it as is | 17:23 |
| natefinch | rogpeppe: yeah yeah, that's fine. Just a reminder for next time when it's not as easy. | 17:23 |
| rogpeppe | natefinch: yeah, thanks | 17:23 |
| sinzui | rogpeppe, This is the comparable goyaml dep fix for trunk: https://codereview.appspot.com/26960043 | 17:28 |
| rogpeppe | sinzui: LGTM | 17:29 |
| sinzui | thank you | 17:29 |
| rogpeppe | natefinch: personally I feel that it's a mistake for callers of a function to be relying on specific error kinds returned by the function | 17:33 |
| rogpeppe | natefinch: unless they're explicitly documented | 17:34 |
| rogpeppe | natefinch: because they're really part of the implementation detail of the function | 17:34 |
| rogpeppe | natefinch: my errgo package (launchpad.net/errgo/errors) makes that a point of principle and always masks the underlying error kind unless you explicitly ask otherwise | 17:34 |
| natefinch | rogpeppe: the common and expected errors *should* be documented | 17:38 |
| rogpeppe | natefinch: indeed | 17:38 |
| rogpeppe | natefinch: and in this case there are none | 17:38 |
| rogpeppe | natefinch: because the document must have been created already | 17:38 |
| rogpeppe | natefinch: error kinds are important when a caller might legitimately decide to act differently based on the kind of error | 17:39 |
| rogpeppe | natefinch: in this case all the errors are as bad as each other | 17:39 |
| natefinch | rogpeppe: yes, definitely. That's fair. I thought there might be some well-known errors, but if there are not, then no big deal. | 17:40 |
| rogpeppe | natefinch: it's annoying that with the standard error stuff you can't add contextual information to an error without either losing the kind or needing to define your own type | 17:41 |
| natefinch | rogpeppe: that's why I made errorWrapper in errors... though it's not directly exposed currently | 17:42 |
| rogpeppe | natefinch: check out errgo - I think it works pretty well. i've been using it a reasonable amount. | 17:42 |
| natefinch | rogpeppe: neat, but not sure I understand the use case for having a diagnosis that is different from the actual error | 17:57 |
| rogpeppe | natefinch: the actual error can have lots of extra context associated | 17:58 |
| rogpeppe | natefinch: the diagnosis is the metadata of the error, if you like | 17:58 |
| natefinch | rogpeppe: why not just wrap the error in a new one that is more accurate? | 17:58 |
| rogpeppe | natefinch: because that's a lot of hassle - you need to define a new error type each time. | 17:59 |
| rogpeppe | natefinch: often it's useful to have classes of error (e.g. "not found") but still keep around some info about the original error | 18:00 |
| rogpeppe | natefinch: for example errgo keeps information about the source locations where the error was propagated with Wrap | 18:00 |
| rogpeppe | natefinch: the division seems to work pretty well in practice | 18:00 |
| rogpeppe | right, i'm done | 18:12 |
| rogpeppe | natefinch: g'night | 18:13 |
| rogpeppe | and happy weekends to one and all | 18:13 |
| natefinch | rogpeppe: night | 18:13 |
| sinzui | natefinch, I think gobot sent my a spurious failure. Do I change the review to approved again to retry, or to I need to also add a comment like LGTM, tests pass locally? | 18:53 |
| natefinch | sinzui: you can just re-approve. looked spurious to me | 18:55 |
| sinzui | thank you natefinch | 18:55 |
| === gary_poster is now known as gary_poster|away | ||
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!