[00:06] thumper, https://bugs.launchpad.net/juju-core/+bug/1438951 [00:06] Bug #1438951: destroy-enviroment --force destroy all aws instances [00:07] thumper, y cant give more info about this, and i cant test it again in current aws [00:07] redelmann_: thanks [00:11] wallyworld_: regarding IsLocal, the key thing is that we are checking if the init system corresponding to the package is the one running on the *local* host [00:12] IsLocal() doesn't imply that [00:12] wallyworld_: in code it won't be "IsLocal()", it will be "systemd.IsLocal()", etc. [00:12] wallyworld_: k [00:12] hmmm [00:12] IsUsed() perhaps [00:12] systemd.IsUsed() [00:13] wadda ya reckon? [00:13] wallyworld_: IsCurrentlyTheOneRunningOnTheLocalHost? :) [00:14] always one smarty pants [00:14] systemd.IsRunning() [00:14] what happens if both files are there though? [00:14] wallyworld_: IsRunning is good [00:14] can a host have both binaries installed? [00:14] wallyworld_: the Ubuntu guys promised me that that will *never* be a problem [00:15] :) [00:15] promises, yeah right [00:16] wallyworld_: how about IsRunningLocally? too long? I was hoping that it would be super clear that we're only checking the local host [00:16] wallyworld_: BTW, thanks for the review :) [00:16] how is IsRunning() not implying that? [00:17] sure :-) [00:17] wouldn't it be assumed that it pertains to the host on which it is run? [00:17] wallyworld_: yeah, you're right; the lack of args limits the possible interpretations :) [00:21] ericsnow: i started reviewing the other one. i don't understand why ioutil.TempFIle is not used [00:21] wasn't the issue that we were using /tmp/somestaticfilename [00:22] so /tmp/somerandomfilename would be fine [00:22] wallyworld_: right [00:22] wallyworld_: but I want to be consistent between the shell script and the Go code [00:22] what do you mean? [00:23] wallyworld_: the patch fixes both the discovery script (written in bash for remote use) and the Go code that does the same thing [00:24] both can still use a random tmp dir [00:25] wallyworld_: I looked at using mktemp on the shell side to do that, but doing it made the code (not insignificantly) more complicated [00:26] hmmm [00:26] wallyworld_: using a subdirectory of the juju data dir helped it stay simple (er) [00:27] i'm not across the implementation, why is using mktemp in the shell script hard? [00:29] wallyworld_: because subsequent commands must be written in relation to where that script gets written [00:30] what generates the shell script? juju? why can't the dir be a script arg? [00:31] wallyworld_: juju does (it's all in service/discover.go + service.go) [00:35] Bug #1438951 was opened: destroy-enviroment --force destroy all aws instances [00:36] yikes, mup! [00:37] ericsnow: so, i'm missing something maybe. in ListServicesScript(), we have const filename = "/tmp/discover_init_system.sh, and that is passed to the script generation commands. why not just generate a random file and pass that [00:40] wallyworld_: so just generate a random filename under /tmp and use that on the remote host? [00:40] oh you are generating the scripts locally and then running remotely [00:41] wallyworld_: correct [00:42] wallyworld_: currently all for the sake of manual provider :) though I have considered using some of this for the cloudinit script [00:42] can init_system=$(blah) run the script inline then? [00:42] rather than from a filename [00:42] then the problem goes away [00:43] wallyworld_: perhaps [00:44] this is a one off bit of bash I think?, so no need to write it out i don't think [00:44] wallyworld_: originally that is effectively what I was doing, but I had to add more complicated logic to the script, I switched to writing the script out to a separate file [00:45] bash doesn't care about how complicated it is when it runs it [00:45] wallyworld_: having a separate script certainly requires much less knowledge of bash :) [00:45] wallyworld_: it's more for my and our sakes than for bash's :) [00:46] wallyworld_: biab [00:46] ok [01:00] wallyworld_: back [01:01] wallyworld_: did I mention that bash is the devil? [01:01] yeah [01:01] i like tcsh [01:02] wallyworld_: hey, isn't Python system-installed everywhere nowadays... [01:02] so ioutil.TempFile is nice and portable, and simple to use, none of this data dr and series stuff andeep juju's dir clean [01:02] can't count on it maybe? not sure? [01:04] the script is only 20 lines or so, so surely it can be run inlien and all this just goes away [01:04] wallyworld_: I'll work up a different approach in that patch [01:05] ok, sorry to be a party pooper, it's just my opinion, maybe a different reviewer would feel differently [01:05] wallyworld_: after that other patch you reviewed the script gets a lot shorter and simpler [01:05] yeah [01:05] wallyworld_: I'll get that landed and see where things are at [01:05] i was having a big problem with the various g=hacks also [01:05] wallyworld_: and hey, I appreciate you taking a stand :) [01:05] and this will remove those [01:59] wallyworld_: just gave you a shipit [02:00] axw: awesome, ty [02:16] wallyworld_: I think you'll like this better: http://reviews.vapour.ws/r/1351/ [02:16] ok [02:20] ericsnow: yay, looks much better [02:20] it works? [02:20] wallyworld_: thanks for sticking to your guns :) I like it better too [02:20] wallyworld_: yep [02:20] \o/ [02:20] wallyworld_: there's a unit test that checks [02:20] yeah, much cleaner [02:26] wallyworld_: thanks for those reviews [02:27] np :-) [02:28] wallyworld_: I'll land those for 1.23 now-ish and then vanish into the night [02:28] sure [02:28] i'll re-merge if there's a spurious failure [02:28] ty [03:30] wallyworld_: got a few minutes? [03:30] wallyworld_: I need an insanity check [03:31] ok, give me 2 mins [03:31] kk [04:34] axw: would be awesome if you got to look at this sometime if you are free http://reviews.vapour.ws/r/1354/ [04:35] wallyworld_: sure, will look in a little while [04:35] np ty, i have to duck out for an hour or so anyway [04:35] soon [05:11] wallyworld_, still around? [05:49] wallyworld_: ping [06:10] wallyworld_: I'll have to read the spec before I can review, so probably won't get to it until I'm OCR tomorrow sorry [06:37] * dimitern steps out - back in a few hours [07:29] jam: hi, i was out, about to have dinner, can i ping you soon? [07:30] wallyworld_: np [07:30] I was just checking if there was anything you want me to bring up to Mark. [07:31] I know you had the Storage "hints" vs "properties" [07:31] wallyworld_: but any other information about things you want clarity on for Storage or Health, I'm on the call with him now [07:45] morning o/ [07:46] dimitern: I'll be working in an interrupted way today. coordinating cleaning of the ground, expecting the reviewer, calling the garage to pick the car etc [07:46] TheMue: sorry about your car! [07:46] dimitern: I've made those tweaks [07:47] dimitern: I'll land the branch so I can fix the issue with upgrading and then forward port the fix to trunk [07:48] dimitern: regarding the fix imho our tests showed that it isn't the primary interface. so now I'll focus on the empty interfaces, because they are the only difference returned from the lshw parsing function [07:49] dimitern: so I'll compare the values and follow their usage bottom-up to see, how they influence the further process [07:50] voidspace: thanks, yes. thankfully first feedback is, that it likely is full covered [07:50] TheMue: *phew*, what a hassle :-/ [07:51] jam: with "juju status-history ", I was going to default to say 10 recent status changes with an option -n arg. Also you would see agent and workload status interleaved [07:51] voidspace: now it's "only" all that organizational stuff as well as looking/waiting for a new one [07:51] wallyworld_: mark likes "hints" [07:51] ok [07:52] jam: with the legacy agent state, I added Stopped (so now we have Pending, Error, Started and Stopped [07:52] jam: the status interleaving is so you could see that an agent ran X hook, which caused the unit to report Y status [07:54] jam: we also firmly believe that when a hook fails, it should be the agent that is in error, not the workload (as the workload is most very likely still fine). Nonetheless, I followed the spec and we can discuss again if users complain [08:16] * dimitern is back [08:17] TheMue, voidspace, thanks guys, sounds good - keep me posted :) [08:18] dimitern: fix for upgrade on 1.23 nearly ready for review - just pushing it now [08:19] voidspace, awesome! [08:28] wallyworld_: "juju status-history" sounds good to me, can you put it into the doc as you've developed it? [08:28] jam: haven't put fingers to keyboard yet; that's how i propose to do it, will update doc [08:28] wallyworld_: for the workload, we're certainly in a situation where "we have no idea what is going on" once we're in an error state. So there are at least questions about what is the smallest lie [08:29] jam: agreed to an extent. we can see how it unfolds once in the hands of users. william has very strong views [08:32] voidspace, I assume your latest PR includes both the worker and the fix? [08:37] dimitern: ah yes [08:37] dimitern: yeah, to fix on 1.23 I needed to base it off the release-address branch which has the worker [08:37] dimitern: once that branch lands the diff will be smaller... [08:37] voidspace, right, got it [08:38] voidspace, reviewed [08:39] dimitern: thanks [08:39] dimitern: they're good comments - except for the very last one [08:39] voidspace, oh yeah? :) [08:40] dimitern: what do you mean by an extra test for a removed machine? [08:40] dimitern: we have a test for a machine that doesn't exist [08:40] dimitern: what's the difference between that and a removed machine? [08:40] voidspace, no, that's fine then [08:40] voidspace, I must've missed it [08:40] dimitern: the SetUp code that creates dead addresses allocates them to a non-existent machine [08:41] voidspace, right, all good then :) [08:41] which simulates the situation we're guarding against [08:41] I'll fix the other issues you pointed out, thanks [08:41] voidspace, cheers [08:41] dooferlad, hey [08:41] dimitern: hi [08:42] dooferlad, I've realized while reviewing your update command that some bits of the spec are contradictory [08:42] dimitern: ah [08:42] dooferlad, we shouldn't allow a space with no subnets [08:42] dooferlad, and create has to be changed slightly to accept at least one subnet [08:43] dimitern: I was pondering this yesterday, but after reading the spaces and subnets sections of the spec several times it seemed that just implementing what it said was the way to go for now. [08:43] dooferlad, that's due to the way I plan to implement spaces on AWS at least - using tags on subnets [08:44] dimitern: I agree that a space without subnets doesn't make any sense [08:44] dimitern: logger.Debugf("requested ReleaseAddress of address %q for unknown instance, ignoring", addr.Value) [08:44] dooferlad, I agree - with nothing better to go with :) sure [08:44] dimitern: will change to (ignoring) - I think that's your preferred style [08:45] voidspace, I'd go with "release address for an unknown instance ID is a no-op, ignoring" ? [08:45] voidspace, or (ignoring) at the end - both are fine [08:46] dimitern: and not log the actual address? [08:46] voidspace, ah, good point - please do :) [08:47] release address %q ... [08:47] done [09:01] TheMue, voidspace, standup? [09:04] dimitern: oops, omw [09:17] dimitern: the forward port to trunk: http://reviews.vapour.ws/r/1358/diff/# [09:19] voidspace, LGTM === natefinch-dinner is now known as natefinch [09:59] dimitern: saw your review on my WIP branch for ensure-availability converting machines... after looking at the code, I think I understand the problems with the API stuff, but want to make sure I'm going down the right path. [10:00] natefinch, great, do you think we need to have a chat or? [10:01] natefinch, I'm glad to help if you have things to ask as well [10:01] dimitern: yeah, but I have a lightly sleeping baby next to me, so it'll need to be text. :) [10:01] natefinch, sure :) [10:02] dimitern: So, I presume I should be using UnitsWatcher to watch a machine, instead of rolling my own, correct? [10:02] dimitern: wayne wrote the watcher code for this, and I think didn't realize UnitsWatcher was also for watching machines (it took me a while to figure that out too... actually just got lucky seeing a comment about how it also worked for machines) [10:03] natefinch, hmm UnitsWatcher IIRC reports changes to a service's units [10:04] natefinch, i might be missing some context [10:04] dimitern: it seems to work on machines and services in practice, plus the code just takes a tag [10:04] * dimitern revisits the PR [10:05] dimitern: you were pretty vague about what was wron with the code, so maybe you should start by explaining what you meant: http://reviews.vapour.ws/r/1299/ [10:05] :) [10:06] natefinch, right, sorry - I tried to explain 3 times, but each time my connection dropped or something else was wrong, so I lost 3 separate summaries (growing shorter with frustration) [10:06] ha, I know how it is. [10:06] natefinch, yeah, so let me have another look now and will explain [10:07] dimitern: thanks [10:07] dimitern: I think I have an idea, after looking at the other API endpoints, but want to make sure I'm on the right track. [10:08] natefinch, ok, so one thing is definitely about using the error result for "fatal" errors, while using the Error field of the result structs for per-tag issues (e.g. failed to start watching machine X's units) [10:08] natefinch, that's in the client-side api methods [10:09] natefinch, and checking that if you asked for 1 tag you got 1 result and there wasn't an error in the result's Error field [10:09] natefinch, this applies to both client methods [10:11] dimitern: ok, yeah, trhat's what I was thinking [10:11] natefinch, on the server-side, it seems you are taking a bunch of args (tags), but in some cases returning on error with one of them (e.g. ParseMachineTag failed), rather than continuing [10:11] dimitern, ping when you have a moment [10:11] mattyw, sure, in a little bit [10:11] dimitern, sure, I'll ping you some stuff for you to take a look at at your convenience if that's ok? [10:11] mattyw, no worries [10:12] natefinch, also server-side - it's more common to have a getAuthFunc which returns an AuthFunc(tag) and checks permissions, rather than using the authorizer directly [10:13] natefinch, the JobsResult struct should have a Error field like other result structs [10:14] dimitern: yep, ok. This is all exactly what I just fixed up yesterday afternoon... I had hoped that's what you meant. Of course now that I've fixed it all, it doesn't work anymore, but I'm sure there's just a dumb bug somewhere :) [10:15] natefinch, finally, please use names.MachineTag rather than string client-side; the rest are trivial formatting things [10:15] dimitern: yep, cool. [10:15] natefinch, oh, too bad - have you found out why it doesn't work? [10:17] dimitern: not yet, need to put in some logging to see... the handler fires, so that much is working, maybe I mucked up the code that returns the jobs [10:18] natefinch, I see, well ping me if I can help with something else [10:18] dimitern: thanks [10:25] dimitern: you yesterday wanted to take a look into the cloud-init.log. I still cannot reach it on a failing node, but I'm now on a working one and try to find aspects that are matching to the information generated based on the lshw results [10:26] dimitern: do you had something special in mind to look at or only wanted to find some kind of errors or strange looking messages? [10:26] TheMue, well, it's hard to understand what's going on without being familiar with cloud-init source - there's a lot of stuff in that log [10:26] * TheMue btw should create another vmaas environment for overlapping tests :/ [10:27] dimitern: it is, indeed [10:27] TheMue, more useful usually is cloud-init-output.log [10:27] dimitern: ah, thx for that hint, will switch [10:27] TheMue, there could be some errors or warnings in cloud-init.log, but most of them are ok - unless it says something serious (e.g. "cannot start" or "failed ..") [10:28] dimitern: ok [10:29] dimitern: I'm currently adding debugs all the call stack upwards to see how the data influences the processing [10:29] TheMue, +1 [10:30] dimitern: sometimes I wish my old smalltalk back, breaking the execution with live editing of the code and manipulation of the data [10:31] TheMue, or even a proper debugger with breakpoints and patching the code on the fly? :) [10:34] dimitern: so far no debugger for static languages I've seen during my travel through programming languages had the same simplicity and power of a Smalltalk environment [10:37] TheMue, I haven't used it, but I believe you :) [10:38] dimitern: it's a wonderful language as well as environment (everything is in one so called image). but sadly it doesn't scale on multicore, everything is single-process :( [10:39] TheMue, I see - well, multicore CPUs where not that common at the time I guess [10:43] dimitern: exactly, but multicore isn't so young anymore too, so one could at least have added this as a feature to the VM and then the according code. maybe I should port it to the Erlang VM, like LFE is doing it for LISP :D [10:43] :) [10:44] but then I would need a time compressor before, putting 48h into a day [10:51] I'm seeing test failures like this when I run tests in juju master: [10:51] http://paste.ubuntu.com/10717600/ [10:51] the weird part is that I'm getting these with go1.4, go1.3 and go1.2.2 [10:52] they seem to be an ordering issue [10:52] but as far as I can tell, nobody can reproduce them [10:52] could I have messed something up in my environment? [10:55] first off, only use 1.2.2 .. we don't officially support newer versions (and I know there's a problem at least with 1.4) [10:55] Bug #1439112 was opened: cmd/juju DeploySuite.TestUpgradeCharmDir fails on windows

[10:55] tasdomas, is than on master? [10:56] dimitern, yes [10:56] dimitern, I've been able to reproduce that with a clean checkout of juju (go get github.com/juju/juju) in an empty GOPATH [10:56] tasdomas, and is it random or you can repro it reliably? [10:57] dimitern, can reproduce it every single time [10:58] tasdomas, ok, that's good - we had reports about those tests but they were unreliably reproducible [10:59] tasdomas, it's not an ordering issue as there are not maps involved there at all, something else is wrong [11:00] dimitern: it could still be an ordering issue :) [11:00] dimitern, well, if I can help with solving this let me know [11:00] dimitern: it's a slice :) [11:00] natefinch, with slices? [11:00] natefinch, :) ah, right [11:02] tasdomas, can you replace that gc.DeepEquals with jc.DeepEquals, run the tests a few times and paste the result please? [11:03] dimitern, sure [11:05] dimitern, still a failure [11:05] http://paste.ubuntu.com/10717644/ [11:06] tasdomas, ok, thanks - just to scratch off the easy solution - try one more time with jc.SameContents instead of jc.DeepEquals? [11:07] dimitern, sure [11:07] I think the lists are just different [11:08] natefinch, they are the same length, so ISTM the order is the only issue [11:09] tasdomas, could it be you're using a more recent mongodb? [11:10] dimitern, 2.6.3 [11:10] dimitern, SameContents panics [11:11] tasdomas, hmm I thought it could be mongo related; oh - is the panic about the map field ExtraConfig? [11:11] dimitern, not as far as I can tell: ➜ juju (git)-[master]-% go test github.com/juju/juju/api/networker [11:11] ---------------------------------------------------------------------- [11:11] PANIC: networker_test.go:204: networkerSuite.TestMachineNetworkConfig [11:11] [LOG] 0:00.011 DEBUG juju.environs.configstore Made dir /tmp/check-8674665223082153551/2/home/ubuntu/.juju/environments [11:11] [LOG] 0:00.099 DEBUG juju.environs.configstore writing jenv file [11:11] [LOG] 0:00.099 DEBUG juju.environs.configstore writing jenv file to /tmp/check-8674665223082153551/2/home/ubuntu/.juju/environments/dummyenv.jenv [11:11] [LOG] 0:00.099 DEBUG juju.environs.tools reading v1.* tools [11:11] [LOG] 0:00.099 INFO juju.environs.testing uploading FAKE tools 1.24-alpha1-trusty-amd64 [11:11] [LOG] 0:00.101 INFO juju.environs.testing uploading FAKE tools 1.24-alpha1-precise-amd64 [11:11] [LOG] 0:00.101 INFO juju.environs.testing uploading FAKE tools 1.24-alpha1-utopic-amd64 [11:11] [LOG] 0:00.102 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any [11:12] [LOG] 0:00.102 DEBUG juju.environs.tools no series specified when finding tools, looking for any [11:12] [LOG] 0:00.103 DEBUG juju.environs.simplestreams searching for metadata in datasource "existing metadata" [11:12] [LOG] 0:00.103 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/check-8674665223082153551/5/tools/streams/v1/index2.sjson": stat /tmp/check-8674665223082153551/5/tools/streams/v1/index2.sjson: no such file or directory [11:12] [LOG] 0:00.103 DEBUG juju.environs.simplestreams streams/v1/index2.sjson not found, trying legacy index file [11:12] [LOG] 0:00.103 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/check-8674665223082153551/5/tools/streams/v1/index.sjson": stat /tmp/check-8674665223082153551/5/tools/streams/v1/index.sjson: no such file or directory [11:12] [LOG] 0:00.103 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/check-8674665223082153551/5/tools/streams/v1/index.sjson": invalid URL "file:///tmp/check-8674665223082153551/5/tools/streams/v1/index.sjson" not found [11:12] [LOG] 0:00.103 DEBUG ju [11:12] dimitern, sorry - wrong buffer [11:12] http://paste.ubuntu.com/10717662/ [11:13] tasdomas, right - yeah it's not obvious, but it is because the map field makes the struct unhashable [11:14] tasdomas, ok, so I think we should definitely file a bug about this and tackle it at some point, but because it only happens with a more recent mongod, it's not a serious issue (I mean of the "fix asap" kind) [11:14] dimitern, understood [11:14] dimitern, I'll file the bug [11:15] dimitern, what version of mongo is preferrable ? [11:15] tasdomas, great, thanks for your help [11:15] tasdomas, the one we're using is 2.4.9 from trusty [11:15] tasdomas, there are a few known issues with 2.6 [11:16] (more juju incompatibilities than mongo issues) === gnuoy` is now known as gnuoy [11:42] dooferlad, I've updated the network spec to clarify a few bits around managing spaces, including the changes we discussed around update (added rename as a separate command) [11:49] wallyworld: are you still around? [11:50] yeah [11:52] wallyworld: well, I'm half tempted to just say "go to bed" :) [11:52] wallyworld: but the other half of me has a question about the simplestreams tuff [11:52] stuff [11:53] wallyworld: specifically, in environs/simplestreams/simplestreams.go GetProductsPath [11:53] we filter by datatype [11:53] and then we return "errors.NotFoundF" if there are no entries that have the same DataType [11:53] we alsor return NotFoundf if the cloud spec doesn't match [11:54] but we return NoMatchingProductsError [11:54] for the other circumstances [11:54] (if the only thing that doesn't match is ProductId) [11:54] jam: ? [11:55] wallyworld: so I'm curious why finding an index file but not finding cloudspec or datatype is considered NotFound [11:55] but finding one that just doesn't have the product is considered NoMatchingProducts [11:55] *specifically* because we special case NotFound in how we log errors [11:56] no particular reason, except maybe bad implementation. maybe we should have used NoMatchingClouds and NoMatchingDataTypes errors [11:57] we could or should fix it i guess [11:57] wallyworld: well if we get noMatchingProductsError, then we do something weird and log it, but then fall through [11:57] so you still get an items list instead of nil [11:58] from memory, that could be because the products might still be found in a different products file [11:58] but i'd have to look at the code, it's been a while [11:58] wallyworld: sure, I have the feeling things have evolved, because the next higher loop keeps searching on any error [11:59] the implementation has grown organically to match what's served by cloudimages [11:59] it needs to be looked at fresh and a proper V2 refactoring done [12:00] mirrors were also add after V [12:00] 1 [12:00] so it got very messy [12:00] sure [12:01] i do wish it were rewritten [12:07] wallyworld: http://reviews.vapour.ws/r/1362/ [12:09] looking [12:12] jam: \o/ [13:06] dimitern, ping? [13:09] tasdomas, pong [13:09] dimitern, a uniter API question, if I may [13:10] tasdomas, sure [13:10] dimitern, so the uniter uses only the uniter API client, I need to modify the way metrics are added to state (from the unit) [13:11] should I override the existing method in the uniter API (and APIserver) or should I introduce a different facade? [13:11] s/different/new [13:11] tasdomas, it depends on how compatible the api changes is - if it's adding a new method or adding args to an existing one, it's fine to do it directly in the current uniter api facade [13:12] dimitern, what if it's changing the params of an existing one? [13:12] tasdomas, if it changes behavior or removes args/methods in a backward-incompatible way - a new facade version of the uniter api is needed [13:12] dimitern, got it - thank you [13:13] tasdomas, np - it will help to have a look at the change, when ready [13:14] dimitern, it's related to api/metricstorage (and apiserver/metricstorage) - I introduced this as a new facade, but now see the error of my ways and will move the functionality to the uniter api client [13:15] tasdomas, ok, ISTM it will be fine to add the new methods to the existing uniter api facade (v1 IIRC) [13:16] tasdomas, however, keep in mind when implementing the client-side methods that the apiserver might be older and can return CodeNotImplemented error for the new methods [13:19] dimitern, thanks [13:20] dimitern, now I remember why I added this as a new facade - fwereade suggested I do this to avoid incrementing the version of the uniter facade (which is largish) due to this small change [13:22] tasdomas, I see, there's no need to increment the version if only new (and not critical) methods are added (i.e. if the client code is prepared to handle gracefully the case when the new methods are missing) [13:31] * dimitern steps out for a bit [14:01] Bug #1439204 was opened: environs/tools does not cleanup in the right order

[14:25] * dimitern is back [14:28] dimitern: what's the current standard way of creating a juju review request? [14:29] katco: fairly trivial review: http://reviews.vapour.ws/r/1363/diff/# [14:29] jam: k i'll take a look [14:29] rogpeppe, just propose a branch against one of the branches in juju/juju - a RB diff should be created automatically [14:29] rogpeppe: generally propose a github merge [14:29] dimitern: ah, i'll just make a PR then, thanks [14:30] rogpeppe: rb notices and then updates reviews.vapour.ws and updates github with a link to it. [14:30] jam: wow you weren't kidding :) [14:30] jam: it would be nice if the README had contribution guidelines, i guess [14:30] jam: +1 [14:30] rogpeppe: probably needs to be updated, I agree. [14:30] I thought we had CONTRIBUTING [14:31] we do... the code review section is kind of a wall of text, though [14:32] and links to https://github.com/juju/juju/blob/master/doc/contributions/reviewboard.md [14:32] which has more specifics [14:33] jam: thanks - i hadn't seen that [14:33] jam: the README should probably link to it [14:35] jam: this particular PR is interesting from the p.o.v. of the recent discussion on juju-dev [14:36] jam: as it uses an external type in the API serialisation [14:36] jam: i'm not convinced that it's wrong here though [14:36] jam: i'd be interested in your thoughts [14:37] * katco does happy dance [14:37] i'm feeling good today :) [14:38] * rogpeppe is pleased for katco [14:38] rogpeppe: I'd say I generally prefer being more explicit on API stuff because it is far too easy to stay compatible with yourself and break everyone else in the world [14:38] rogpeppe: i've had a sinus infection for the past few weeks. haven't been able to do much of anything =/ [14:39] katco: ew [14:39] katco: i also had a horrible flu thing last week, still feeling pretty knacked [14:39] rogpeppe: seems everyone's been sick [14:39] katco: yeah [14:39] rogpeppe: and we'll bring it all to nuremberg! :) [14:39] katco: i hadn't had something like this in decades [14:39] katco: joy! [14:40] katco: sinus thing sounds 'orrible [14:40] rogpeppe: yeah it's been a few years since i've been knocked on my butt like this [14:40] rogpeppe: i suppose it's a good reminder to appreciate the good times :) [14:40] katco: well, continue to take it easy! yeah. [14:40] rogpeppe: you too sir! see you in nuremberg i hope [14:40] katco: deffo [14:41] jam: the issue in this particular case is that we want to transfer a macaroon over the wire, and the macaroon wire format is well defined, and implemented inside the macaroon package [14:42] jam: there's actually no easy way to use a different format except by dependending on precisely the format we're trying to avoid depending on [14:42] rogpeppe: isn't a serialized macaroon just a series of lines with a final hash? [14:42] jam: no [14:43] jam: and even if it was, i wouldn't want to duplicate all the (very well tested) serialisation code that's in the macaroon package [14:43] rogpeppe: so I'm saying that the API can be "you get a string" which you then parse as a macaroon. not "you get a Macaroon object" [14:44] jam: how is that any better? [14:44] rogpeppe: fwiw I was confusing M.serialize() with M.inspect(). The latter being the line-by-line with a final sig [14:44]