[01:12] hazmat: ping [04:45] thedac, pong [04:45] whoops [04:45] axw, ping [04:46] hazmat: heya [06:03] jam: are you around? if so, can you please prod the bot? [06:03] axw: looking [06:08] axw: poked, it should be trying to merge rogpeppe1's branch right now (473-parallel-try) [06:08] thanks jam [09:00] jam, ping [09:00] dimitern: pong [09:00] jam, i need some bzr surgery :) [09:00] what's up? [09:01] jam, I removed the current branch I was working on using rmbranch (didn't even get a warning!, but I'm usually careful not to do that) [09:01] jam, and now pretty much every command returns bzr: ERROR: Not a branch: "/home/dimitern/src/juju-core/223-putcharm-uses-charm-upload/.bzr/branch/". [09:02] jam, is there an easy way to reset that and switch to trunk? [09:03] dimitern: bzr switch trunk --force [09:03] jam, wow! tyvm it worked :) [09:03] dimitern: by default switch does some sort of introspection on the "current" branch, which obviously doesn't work well when it isn't there, but --force skips that step [09:05] mornin' all [09:08] hey rogpeppe1. your branch timed out in the tests in utils/parallel earlier. I couldn't reproduce it on my machine, but haven't looked into the code in detail yet [09:08] axw: yeah, i just saw that [09:08] axw: i can't repro either [09:08] axw: :-( [09:09] axw: it's a pity that the output for a timed-out test is so meagre [09:09] indeed, don't even get to see which test it was... [09:09] yup [09:09] jam, I was already dreading the possibility that I actually need to start from scratch :) [09:12] axw: i'm just trying with go1.1.2 in case that makes a difference [09:12] rogpeppe1: okey dokey. I'll do some code inspection to see if I spot anything [09:12] axw: that would be useful, thanks [09:13] axw: still passes consistently with go1.1.2 [09:13] :( [09:39] axw: i guess i'll try it in a machine in the cloud [09:40] rogpeppe1: I'd try it on my underpowered VM, but my server's crapped itself [10:09] rogpeppe1, I think you'll like this https://codereview.appspot.com/43330043 [10:09] dimitern: looking [10:20] rogpeppe1: it would appear that something in the trySuite is leaking goroutines. I added -test.cpu=1,1,1,1,1,1,... and number of goroutines keeps going up, and runtime keeps going up [10:20] possibly related, but ... [10:20] seems unlikely [10:21] axw: it will probably go up for a while, but then settle down - it leaks a few goroutines that sleep for 4 or 5 seconds [10:21] rogpeppe1: didn't ever go down, but I may not have waited long enough [10:22] axw: hmm, i'll have a look [10:22] I'll keep digging [10:24] rogpeppe1: trySuite.TestStartBlocksForMaxParallel [10:24] gtg [10:24] axw: thanks [10:27] axw: yeah, i've just repro'd that [10:28] axw: it would have to be the last test i added... [10:35] jamespage: free for a fast catchup? [10:38] jamespage: er... actually, ignore me [10:38] jam: free for a fast catchup? [10:39] found the problem! [10:45] rogpeppe1, mgz, jam, standup [10:48] dimitern: I'll have to skip as in sprint, but can give update on irc [10:49] mgz, sure, go for it [10:52] at mini-sprint in london trying to help the CI guys with testing in charms, testing with charms, and testing juju setups [10:53] if there are any questions that come up over the next few days will bug people here for answers where I'm not sure [11:02] rogpeppe1, btw while fixing the test, I have a couple of comments on https://codereview.appspot.com/42760043/ [11:02] rogpeppe1, i'll publish them shortly [11:03] dimitern: please do - i was about to push and re-approve [11:06] rogpeppe1, sent [11:10] mgz: sorry I missed you earlier. So when I went to sign up my son yesterday, one of the after school activities didn't show, so I had to go *again* today to actually sign him up. I have a 1:1 with Nate, but then I'm available to chat after that [11:11] jam: sure, I'll find a gap [11:23] rogpeppe1, and you've got another review [11:33] dimitern: ta! [11:48] rogpeppe1: what was the issue? [11:48] axw: the loop was spinning [11:48] axw: so nothing made progress [11:48] rogpeppe1: in the test or code-under-test? [11:48] axw: i'm a bit surprised that nothing made progress even with GOMAXPROCS>1 actually [11:49] axw: in the code under test - it was a bona fide bug [11:49] axw: the problem was that i was closing t.close, and always reading from it [11:49] axw: trivially fixed [11:49] ah, not setting to nil? [11:49] axw: yeah [11:49] yeah I see it [11:49] cool [11:50] axw: the fix actually made the code slighly simpler, which was nice [11:55] rogpeppe1: I changed my code to using parallel.Try today. seems a bit more convoluted now due to the need to retry, but I may just have overlooked a simple alternative [11:55] once your MP is landed I'd appreciate a glance over [11:56] axw: i'd put the retries inside the functions started by Try [11:56] rogpeppe1: that's what I did :) [11:56] rogpeppe1, review poke [11:56] each one is given a channel to wait on that's closed when it should stop retrying [11:56] axw: Try does that for you, right? [11:57] dimitern: thanks; getting back to it :-) [11:57] rogpeppe1, cheers :) [11:57] rogpeppe1: maybe I've overcomplicated it. my current code caters for stopping retries on individual addresses [11:57] rogpeppe1: for pathological case where addresses come and go [11:58] maybe it should just keep going forever (until one wins or global timeout) for each address it sees? [11:59] axw: i'm just wondering that too [12:14] mgz: poke [12:17] rogpeppe1: when you have time, https://codereview.appspot.com/40860048/ [12:17] axw: will do after this [12:18] the parallel.Try bit is all in provider/common/bootstrap.go [12:18] rogpeppe1: nps [12:18] thanks [12:19] rogpeppe1, I was thinking of renaming UploadCharm to UploadLocalCharm actually, because we've been just discussing with william about having an Upload[Charm?]StoreCharm to deal with CS urls [12:19] dimitern: so you can override the default charm store fetching? [12:21] dimitern: reviewed [12:21] rogpeppe1, yeah, all the charm upload ops will be performed by these two methods, which will get rid of some code duplication [12:21] rogpeppe1, thanks [12:22] dimitern: so how would the api Deploy call work? [12:22] dimitern: would that need to call the api itself? [12:23] fwereade, any news on that branch? is there stuff I need to be fixing? [12:23] dimitern: sorry, i probably mean the api AddService call [12:23] rogpeppe1, right now there is some shared code between the CLI and API - DeployService [12:23] mattyw, sorry, I'll reapprove, the bot was throwing a fit [12:23] rogpeppe1, I'd like to move all that code into the ServiceDeploy API call and make use of the UploadCharm calls in there [12:24] dimitern: i think i'm confiu [12:24] dimitern: i think i'm confused [12:24] dimitern: are you suggesting that we add an api call UploadStoreCharm? [12:25] rogpeppe1, basically yes [12:25] dimitern: so how would the ServiceDeploy API call be implemented (server-side)? [12:26] rogpeppe1, it means "go and do this charm-related operation that has hitherto been hidden away inside deploy/upgrade-charm" [12:26] fwereade: so this is essentially an AddCharm call? [12:26] rogpeppe1, ServiceDeploy would start with "if the charm url isn't already recognised, barf" [12:26] rogpeppe1, yeah [12:26] fwereade: with no uploading from the client to the server? [12:26] rogpeppe1, we'd have to deprecate [12:26] rogpeppe1, yes [12:27] rogpeppe1, exactly [12:27] fwereade: ah, that makes sense. but i don't think that "Upload" is great in this context [12:27] rogpeppe1, fair enough, I'm not married to the name [12:28] fwereade, dimitern: it made me think that it was something that the client was uploading [12:28] rogpeppe1, there's some advantage to having it rhyme with the local version [12:28] fwereade: perhaps we should call them both Add(Local)?Charm [12:28] rogpeppe1, sounds good? [12:30] fwereade: AddCharm(*charm.URL); AddLocalCharm(*charm.URL, charm.Charm) [12:30] dimitern: what do you think? [12:33] rogpeppe1, why Add when we're doing an upload? [12:33] dimitern: because we want them both to rhyme, and Upload is inappropriate for the other one [12:33] rogpeppe1, in both cases really - there's always a "upload to provider storage" step [12:33] fwereade: I updated the DestroyEnvironment API based on our previous discussions (https://codereview.appspot.com/41280043/). The API call can still render the environment unusable (apart from destroying things) if there are manual machines, if two API clients race. I'm nervous about allowing them to leak, but if you feel strongly enough I can remove that [12:34] dimitern: and they're both adding a charm - one just happens to take the charm from locally [12:34] dimitern: i think of that as an implementation detail - it's downloading as much as uploading [12:34] axw, thanks, I will try to get to it soon, today is meetingy again though [12:34] rogpeppe1, well, i suppose so [12:34] fwereade: no worries [12:35] dimitern: in the future, we probably won't store charms in provider storage, so it'll be even more like download-only [12:36] rogpeppe1, AddStoreCharm perhaps? instead of AddCharm - too generic and confusingly similar to AddLocalCharm, without providing a distinction [12:36] dimitern: i think AddCharm is better because it gives freedom to expand to allow other kinds of URL in the future [12:37] dimitern: for instance we might allow the API server to download github charms directly. [12:37] rogpeppe1, ok, fair enough [12:37] dimitern: it's really "add this url" - we just happen to support only cs: prefixes currently [12:38] dimitern: i just noticed something dubious [12:38] dimitern: why are you using GetNonValidatingHTTPClient ? [12:40] rogpeppe1, because the API server's cert is self-signed [12:40] dimitern: no it's not [12:40] dimitern: it's signed by the CA public key, which we have [12:41] dimitern: the websocket connection is appropriately verified [12:41] dimitern: the POST should be too [12:42] rogpeppe1, somehow I couldn't make a connection with it without skipping the validation [12:42] dimitern: you'll need to add the CA cert to the trusted root keys [12:42] rogpeppe1, the SSL cert for a web server is different than others - domain name needs to match [12:43] rogpeppe1, how do I do that? [12:43] dimitern: how is the websocket code not talking to the web server? [12:43] rogpeppe1, I don't get any issues with the websocket connections, only with HTTPS ones [12:43] dimitern: did you try setting the tls config? [12:44] rogpeppe1, where? [12:44] dimitern: in the http Transport [12:44] dimitern: one mo, i'll investigate [12:44] rogpeppe1, i'm not sure what are you referring to [12:50] rogpeppe1, when I try using http.DefaultClient instead of utils.GetNonValidatingHTTPClient, I get this http://paste.ubuntu.com/6588688/ [12:50] dimitern: you can't use http.DefaultClient [12:50] dimitern: i'm just writing a code fragment for you [12:51] rogpeppe1, why not? [12:51] dimitern: because DefaultClient uses the default root CA certs, which don't include the juju ones [12:55] rogpeppe1, that seems wrong - inventing a self-signed ca cert and using it [12:55] dimitern: huh? [12:55] dimitern: that's the entire basis of our server authentication [12:56] dimitern: what's wrong with using your own certs? [12:57] dimitern: anyway, it's not self-signed - it's just a very short chain :-) [12:57] rogpeppe1, well, anyway [12:57] rogpeppe1, it strikes me as odd [12:57] dimitern: how else can we verify what server we're talking to? [12:59] rogpeppe1, i'm not going there [12:59] rogpeppe1, :) a discussion for another time perhaps [13:00] dimitern: honestly, if you think there are issues with our security model, it would be good to know about them [13:01] rogpeppe1, i think it's inconvenient for others, that want their own hosted juju stuff to talk with standard clients [13:02] dimitern: unfortunately i think that juju is fundamentally incompatible with the standard client authentication model [13:02] dimitern: s/client auth/http server auth/ [13:02] dimitern: for one, we don't have a well known domain name that the API server lives on [13:02] rogpeppe1, not really - as far as tls is concerned [13:03] dimitern: for another, we don't want to require everyone that runs a juju API server to buy a certificate [13:04] rogpeppe1, having the private and public key available makes is very easy for anyone to mount MITM attacks against a running environment [13:04] dimitern: how is the private key available? [13:04] rogpeppe1, well, not very easy, but it's plausible [13:04] rogpeppe1, isn't it in the source? [13:04] dimitern: no [13:05] dimitern: it's created when the env is prepared [13:05] rogpeppe1, so how does the server use it for decryption? [13:05] rogpeppe1, ah, ok [13:05] dimitern: the server is given the private key when it's bootstrapped [13:05] dimitern: (and in general, tls keys are never used for encryption or decryption) [13:06] rogpeppe1, having the ca cert and key, can you generate a key that works for certain environment? [13:06] rogpeppe1, they are, in the handshake process === gary_poster|away is now known as gary_poster [13:06] dimitern: diffie-hellman isn't really encryption [13:07] dimitern: if you have the root ca private key, you can generate any number of server certs and keys [13:07] rogpeppe1, signing or encryption - it's all the same [13:08] dimitern: it's not even signing really [13:08] rogpeppe1, how about generating a *specific* key - preseeding or something, so you can end up with a running env's key pair? [13:09] dimitern: i don't understand that [13:12] rogpeppe1, i'll check it out [13:13] rogpeppe1, so how about that sample code? [13:14] dimitern: almost there [13:16] dimitern: http://paste.ubuntu.com/6588771/ [13:16] dimitern: (something approximately like that anyway) [13:17] fwereade, failed again - I wonder if something is wrong with the branch - I don't see any of those errors though when I run it locally [13:17] rogpeppe1, thanks, looking === BradCrittenden is now known as bac [13:29] rogpeppe1, is the api.Info.CACert always the same or that's the one we're generating? [13:30] dimitern: it's generated at prepare time currently [13:31] rogpeppe1, so i'm thinking of adding utils.GetClientFromInfo(info *api.Info) *http.Client, which can be reused [13:31] dimitern: that doesn't seem quite right (and i don't think you'll be able to anyway) [13:32] rogpeppe1, why? [13:32] dimitern: because you'd get an import cyclre [13:32] cycle [13:33] rogpeppe1, well, then taking a CACert []byte [13:33] rogpeppe1: any reason I shouldn't just land the state api branch? [13:33] dimitern: that could work. [13:33] rogpeppe1: I want to make some more changes, but breakage on a dev version is okay, right? [13:33] dimitern: although i think it would probably live better in the api package [13:33] mgz: which branch is that? [13:34] dimitern: because it's actually all about making an http client that's good for connecting to the api [13:35] rogpeppe1: https://codereview.appspot.com/40350043/ [13:35] dimitern: also, that wouldn't be that convenient to use because the api code actually needs the tls.Config, so would need to type-assert the Client.Transport [13:36] mgz: ah, the *status* api branch :-) [13:36] rogpeppe1: ah yes, whoops [13:36] * rogpeppe1 woz confused for a moment [13:38] rogpeppe1, I don't want to copy 20 lines every time I need to connect to the api server though [13:38] mgz: i have a feeling that rpc calls cannot return both an error and some data [13:39] dimitern: why would you need to do that? [13:39] dimitern: the only place you'd be connecting to the api server would be in the api packages, right? [13:39] rogpeppe1: really? the api doc didn't say that [13:39] rogpeppe1, how about tests? [13:40] rogpeppe1, it doesn't seem right to use a non-validating client there [13:40] dimitern: you might want to export api.State.HTTPClient, similar to how Call is exported [13:40] rogpeppe1: I'm also not clear on the meaning of returning api.Struct vs *api.Struct from rpc [13:41] mgz: rpc requires struct returns for all methods [13:41] mgz: the data is ignored if the error is non-nil... i'm pretty sure - let me check [13:41] rogpeppe1, modify the Caller interface? [13:41] rogpeppe1, that seems way too intrusive [13:41] rogpeppe1: as in, not struct pointers? [13:42] mgz: yup [13:42] I'm confused as to why the tests passed then... [13:42] dimitern: no, you wouldn't necessarily need to modify the Caller interface [13:42] mgz: perhaps there's nothing that tests partial-status-with-error [13:43] rogpeppe1: right, I'm pretty sure there's not for that. but I changed the api return code to *api.Status and if that's illegal I'd expect all the tests to fail [13:44] mgz: hmm, yes [13:44] mgz: ah! [13:45] mgz: perhaps it was just using the fallback code in those tests [13:45] mgz: did you try running all the tests in the tree? [13:45] mgz: i'm pretty sure there's one that tests for inappropriately-ignored rpc methods [13:45] rogpeppe1: I *thought* so, but will recheck [13:48] mgz: launchpad.net/juju-core/state/apiserver tests should have caught that [13:48] mgz: if it didn't then that test is bogus :-) [13:51] dimitern: yeah, i wouldn't modify the Caller interface - but you can add a method State.HTTPClient() *http.Client [13:53] dimitern: then api sub-packages that need to make direct http calls can use an interface that's something like: interface{common.Caller; HTTPClient() *http.Client} [13:53] rogpeppe1, what's wrong with having a common function for that? [13:53] rogpeppe1, in utils [13:53] dimitern: it seems like internal implementation detail of api to me [13:53] rogpeppe1, it's quite useful [13:53] dimitern: we put stuff in utils that's generally useful [13:54] dimitern: where else might we want to use it? [13:54] rogpeppe1, in plugins perhaps? [13:54] dimitern: surely plugins should be using the api package? [13:54] rogpeppe1, i see no harm in add it there [13:55] dimitern: if we find ourselves wanting it somewhere else, i'd factor it out then (it will be trivial to do so) [13:55] dimitern: i prefer to avoid expanding utils indefinitely just because we think that something might be generally useful [13:56] rogpeppe1, btw the code you gave me still causes the same error [13:56] rogpeppe1, x509: cannot validate certificate for 127.0.0.1 because it doesn't contain any IP SANs [13:56] dimitern: could you paste the code you're using? [13:57] rogpeppe1, http://paste.ubuntu.com/6588953/ [13:58] rogpeppe1, and the caCert i'm passing is the one from api.Info used to call Login [13:59] dimitern: could you push your branch please? [13:59] dimitern: aside: it's not right for that code to use a global variable [14:00] rogpeppe1, there's a mutex around it [14:00] dimitern: it looks like you've cargo-culted from NonValidatingHTTPClient [14:00] rogpeppe1, yes [14:01] dimitern: what happens if you pass a different CACert in? [14:01] rogpeppe1, where should I get it from? [14:01] dimitern: where should you get what from? [14:02] rogpeppe1, a different ca cert [14:02] dimitern: maybe you're talking to two environments at once? [14:02] dimitern: that's perfectly allowable [14:03] rogpeppe1, i don't think so [14:03] dimitern: why not? [14:03] dimitern: i should certainly hope it's ok [14:03] rogpeppe1, why should I be doing that? it's a standard code [14:03] dimitern: i'd consider it a serious bug otherwise [14:03] dimitern: what's a standard code? [14:05] rogpeppe1, i mean i'm not doing anything fancy [14:05] dimitern: regardless of anything else, the output of a function should be, if at all possible, a deterministic function of its arguments [14:06] rogpeppe1, pushed lp:~dimitern/juju-core/225-upload-charm-via-api - if you run the tests in state/api you'll see the error [14:06] dimitern: if I call GetHTTPClientFromCert(certA); then i call GetHTTPClientFromCert(certB) the second return will return the http client for certA. that's just wrong. [14:07] rogpeppe1, ok, that's true, I'll remove the mutex + caching and just return a new client every time [14:07] rogpeppe1, but that didn't help with the error - i tried already [14:07] dimitern: no, it wouldn't [14:07] dimitern: it was just an aside [14:19] rogpeppe1, any clue about the error? [14:20] dimitern: i'm investigating currently [14:20] rogpeppe1, ok [14:24] dimitern: this code works fine for me: http://paste.ubuntu.com/6589085/ [14:25] dimitern: (i get a 401 response which indicates that all the tls handshaking and client cert stuff has worked ok) [14:26] dimitern: so i suspect you're not actually using the http client that you created [14:26] rogpeppe1, I am using it [14:26] rogpeppe1, but perhaps the caCert from the api.Info used for Login is not the same as in Environ.Config() [14:29] dimitern: you mean the bug i pointed out above might be triggering the problem? [14:31] rogpeppe1, i'm not using the mutex anymore [14:31] rogpeppe1, so the bug cannot occur [14:31] rogpeppe1, i'm trying to get the environ config and the ca cert in AddLocalCharm to see if it helps [14:31] dimitern: so which test fails? [14:33] rogpeppe1, TestAddLocalCharm, the only one [14:34] dimitern: it passes for me [14:34] dimitern: just to sanity check: what go version are you using? [14:36] rogpeppe1, go version go1.1.2 linux/amd64 [14:36] dimitern: ok, i'll try with that [14:38] rogpeppe1, using the ca-cert from the environ config didn't help [14:38] dimitern: ha! i now see the problem - with go1.1.2 [14:38] rogpeppe1, huh? [14:38] rogpeppe1, those things always happen to me :) [14:39] dimitern: yeah, you seem to be blessed like that [14:39] rogpeppe1, but 1.1.2 is the official version we're building against [14:39] dimitern: yeah [14:39] rogpeppe1, so what's the underlying issue? [14:39] dimitern: i'll try to find that out [14:43] rogpeppe1, so I think the only way around this is to use the non-validating client [14:43] dimitern: we should avoid that if at all possible [14:43] dimitern: but perhaps for now as a workaround with a huge "BUG HERE" comment [14:43] rogpeppe1, i'm open to suggestions for a workaround the issue [14:44] rogpeppe1, yeah, will do [14:46] dimitern: hmm, interestingly my little test example still works ok with go1.1.2 [14:46] dimitern: maybe it's something to do with the cert generation [14:46] rogpeppe1, perhaps [14:50] dimitern: looks like that's the case [14:51] dimitern: my test program succeeds or fails depending on whether the server is compiled with go1.1.2 or go1.2 [14:51] rogpeppe1, if you managed to find an issue# to include in the bug report? [14:51] dimitern: i need to find out what the issue is... [14:52] rogpeppe1, ok, I'll just describe it in general [14:52] dimitern: give me a little more time [14:52] dimitern: i'm just about to find out what the difference between the generated certs is [14:54] rogpeppe1, ok, no rush [14:57] rogpeppe1, it seems we're generating certs without Subject_Alternative_Name field - https://groups.google.com/forum/#!topic/golang-nuts/hCCsphIUuBA [15:01] dimitern: that may be true, but it looks like that's still the case with go1.2, and it works there [15:04] rogpeppe1, x509 in go 1.2 is different than that in 1.1.2 - some more things are supported [15:04] rogpeppe1, specifically the SANs handling has changed, perhaps that's the cause [15:05] dimitern: possibly. but it seems a bit weird that the client version doesn't matter [15:07] rogpeppe1, although the code handling the validation seems unchanged [15:07] rogpeppe1, weird.. [15:22] rogpeppe1, does https://codereview.appspot.com/43330043 look ok to land? [15:24] dimitern: looks like the address->serverRoot name change hasn't been pushed yet [15:24] rogpeppe1, oh, sorry, will repropose [15:33] rogpeppe1, done [15:51] fwereade, rogpeppe1: so here's the problem I've been having with mongodb... the default distribution doesn't support SSL, so creating a new mongo server is failing. That's at least why it's failing locally on my machine. The problem is, I don't know how it was working before, since I'm pretty sure I was running a generic build before, too. [15:52] natefinch: i can't see how you could have run any juju tests with default distribution - we've required that for ages [15:53] rogpeppe1, I must have gotten the special version and forgotten about it, then when I rolled back to 2.2 yesterday, I messed it up. [15:53] rogpeppe1, where do I get the SSL-ified version? [16:01] natefinch: i'm not sure i'm afraid [16:01] natefinch: you can probably apt-get it from somewhere [16:01] natefinch: when you find out, let me know, 'cos i'd like to upgrade to mongo 2.4... [16:01] dimitern: i posted a comment on your CL === teknico_ is now known as teknico [16:02] rogpeppe1, let's not use rpc.NoSuchRequest, because it's nothing like it [16:03] dimitern: in which case, let's unify the two [16:03] rogpeppe1, the transport is different as well [16:03] dimitern: so an api client doesn't have to use several different ways of determining errors [16:03] rogpeppe1, sounds like it has to be built from source or purchased [16:03] rogpeppe1, there are only 2 ways [16:04] dimitern: sure, but there should only be one [16:04] rogpeppe1, and there are 2 transports underneath [16:04] dimitern: the transport is an implementation detail [16:04] rogpeppe1, so you propose to make NoSuchRequest a NotImplementedError [16:05] dimitern: i propose to define CodeNotImplemented and make a non-implemented error just like all the other errors defined in params [16:05] dimitern: so that if you're using state/api/* you only have one way of classifying errors [16:06] rogpeppe1, and how is that going to work for rpc.NoSuchRequest? [16:06] dimitern: one possibility is outlined in my comment [16:06] dimitern: another possibility is to change api.State.Call so that it transforms the rpc error as appropriate [16:10] rogpeppe1, so we can return serverError(&Error{Message: "not impl. blah", Code: params.CodeNotImplemented}) instead of NotImplementedError, and make rpc.NoSuchRequest ErrorCoder-compatible, using params.CodeNotImplemented as code [16:11] dimitern: i *think* params should import rpc rather than the other way around [16:11] dimitern: because we might make rpc into an external package at some point [16:12] dimitern: and serverError is for server-side not client-side use [16:14] dimitern: this is a change that really should be part of another CL; hence i suggest the quick fix so you can land this, then fix all calls to rpc.IsNoSuchRequest to call params.IsNotImplemented instead [16:15] rogpeppe1, problem is, it already landed, sorry [16:15] rogpeppe1, but i'm preparing a follow-up [16:15] dimitern: ok, thanks [16:17] * dimitern will be back soon [16:19] hmm, ears are still closed and whistling, hope it will be better tomorrow. but at least my tests are working fine so far. [16:59] rogpeppe1, how do you suggest doing "make the rpc package return error code CodeNoSuchRequest on method-not-defined [16:59] error" [17:00] rogpeppe1, where should I catch and report this error? [17:01] dimitern: ok, there are few aspects to this: [17:02] dimitern: 1) in rpc/rpcreflect, define type NoSuchMethodError {Method, RootMethod string} [17:02] rogpeppe1, ok [17:04] dimitern: 2) change rpcreflect.MethodCaller to return that error instead of using fmt.Errorf [17:05] rogpeppe1, ok, but why do I need RootMethod if I never set it? [17:06] dimitern: 3) in rpc, define const CodeNotImplemented = "not implemented" [17:06] dimitern: because you'll need it to define the Error return [17:06] dimitern: s/return/method [17:06] dimitern: (more specifically, NoSuchMethodError.Error) [17:07] rogpeppe1, I can do that with only NoSuchMethodError.Method, I don't need RootMethod === gary_poster is now known as gary_poster|away [17:07] dimitern: the message should not change, which means that it should include the root method name too [17:09] 4) in rpc, change bindRequest to check if MethodCaller returns a *NoSuchMethodError, and return RequestError{Message: err.Error(); Code: CodeNotImplemented} if so [17:09]