[01:31] anyone around to give a quick review? http://reviews.vapour.ws/r/388/ === liam_ is now known as Guest12049 === kadams54 is now known as kadams54-away === kadams54 is now known as kadams54-away === kadams54 is now known as kadams54-away [19:33] * thumper takes a deep breath and dives into the massive pile of work emails [20:19] thumper: o/ [20:20] o/ lazyPower [20:20] hows my favorite kiwi? [20:57] davecheney: just responded to your review... thanks by the way. I disagreed with both of your comments :) [21:06] waigani: you still need a review of the settings stuff? [21:06] menn0: yes please: http://reviews.vapour.ws/r/368/ [21:06] waigani: doing it now [21:07] thanks [22:14] waigani: done [22:14] wallyworld_: morning [22:14] menn0: thanks [22:14] hi [22:14] thanks for fixing that bug [22:15] waigani: no problems [22:15] thanks for merging it [22:15] np, was a bit of an experience to get it in 1.21 [22:15] wallyworld_, waigani: duh. The "no problems" was intended for wallyworld_ [22:15] turns out other charm/repo stuff has to be backported too [22:15] :) [22:16] wallyworld_: yeah I saw that. there was another way around that. i initially had that test working without creating a new charm [22:16] wallyworld_: by copying and monkeying the existing logging charm [22:16] menn0: np, I take it as a given there will be problems with my review ;) [22:16] all good, at least 1.21 ans trunk are a little less diverged [22:16] waigani: nothing major, except a potential pre-existiing bug [22:17] wallyworld_: do you want me to backport to 1.20 as well? [22:17] menn0: cool, thanks again, looking now [22:17] menn0: not sure, i was hoping we'd stop doing so much with 1.20 now that 1.21 is almost here [22:18] wallyworld_: it's "fairly" unlikely that people will hit that bug but it's pretty bad when they do [22:18] yeah [22:18] i'll find out from curtis what our plans are [22:18] wallyworld_: my inclination is to do the backport [22:18] it shouldn't take long [22:19] ok, may as well then. i know dimiter has backported some network stuff too [22:19] wallyworld_: k [22:20] wallyworld_: I'll try and get it done today [22:20] no rush, not sure when 1.20 is getting a new release [22:20] 1.21 is the focus this week [22:31] wallyworld_: ok === kadams54 is now known as kadams54-away === kadams54-away is now known as kadams54 [23:09] menn0: when you have a moment: http://reviews.vapour.ws/r/368/ [23:12] waigani: looking [23:13] waigani: so you think that was a bug? [23:13] menn0: yes [23:13] waigani: did you expand the unit tests to cover that scenario? [23:13] menn0: I dumped the db, and the key used to get the doc [23:14] menn0: you were right, good catch [23:14] menn0: no, good point [23:16] waigani: so what's with the checkAddEnvUUIDToCleanupsIdempotent comment? is it supposed to be there? [23:17] menn0: no, i deleted it didn't I? It was just a reminder for me while working on the branch. [23:17] waigani: nope it's still ther [23:18] 0_O [23:18] didn't hit save? [23:18] ah right, ugh [23:18] yeah I'll delete it [23:18] AND save [23:20] menn0: so the test. Should we check that the search key matches the settingsKey format with a regex? [23:22] waigani: I don't think so. You want to engineer something where the first transaction attempt fails so that bit of code gets hit [23:22] waigani: and then make sure the alive test works as expected [23:22] menn0: right, got it [23:23] perhaps make the fix and test changes in a separate PR if it's going to take a while? [23:23] waigani: ^ [23:24] menn0: I'll have a poke around, considering it's nearly lunch, if my hunger wins out I'll do it in a followup PR [23:26] waigani: not sure if you know but you can induce transaction failures using SetAfterHooks and SetBeforeHooks [23:26] menn0: ah nice, thanks [23:27] menn0: so code is currently called at bottom of TestSetCharm // SetCharm fails when the service is Dying. [23:28] menn0: but it's not a positive test === kadams54 is now known as kadams54-away