| menn0 | waigani: yep thanks. I changed parseTag during the meeting and those annotator tests started parsing | 00:03 |
|---|---|---|
| menn0 | waigani: and I knew about the megawatcher change | 00:03 |
| waigani | menn0: sweet | 00:03 |
| menn0 | waigani: from reviewing the services change | 00:03 |
| waigani | menn0: okay, i *think* they are the only spots that will need progressive updating as we migrate | 00:03 |
| waigani | other than dealing with collection specific quirks of course | 00:04 |
| menn0 | waigani: that sounds right | 00:06 |
| menn0 | waigani: and then there's all the queries that need to be updated to filter by env uuid | 00:06 |
| menn0 | waigani: but we should get the collections in shape first | 00:06 |
| hazmat | out of curiosity.. anyone using gooracle | 00:07 |
| hazmat | davecheney, pong | 00:07 |
| waigani | menn0: largely mechanical | 00:07 |
| waigani | hazmat: I wrote a sublime text plugin: https://github.com/waigani/GoOracle | 00:07 |
| waigani | hazmat: I use it most days, has really helped :) | 00:08 |
| hazmat | waigani, nice.. i've got sublime.. but emacs users myself.. currently resurrecting my go editor conf.. oracle seems pretty sweet | 00:09 |
| waigani | hazmat: menn0 uses emacs with oracle | 00:09 |
| waigani | I couldn't keep up with him, so a wrote the plugin for sublime ;) | 00:10 |
| hazmat | waigani, solid.. i'm currently pimping my ride following along to http://yousefourabi.com/blog/2014/05/emacs-for-go/ | 00:12 |
| davecheney | hazmat: are you coming to paris on friday ? | 00:12 |
| hazmat | davecheney, unclear.. its not sold out it is it? | 00:12 |
| hazmat | nope.. still avail | 00:12 |
| hazmat | oh.. crap.. super late bird only now | 00:13 |
| davecheney | hazmat: well, you are super late | 00:13 |
| hazmat | davecheney, its like SO next month :-) | 00:13 |
| davecheney | hazmat: its like so 15 days away | 00:13 |
| hazmat | if i get through a day i'm on top of the world | 00:14 |
| hazmat | davecheney, noted.. i'll have it sorted this week | 00:14 |
| davecheney | hazmat: ok, if you get stuck I can ask the organisers | 00:15 |
| davecheney | i won't be a free ticket | 00:15 |
| davecheney | but we might be able to get you into the overflow | 00:15 |
| hazmat | i gotta coordinate with sidnei he'll come out as well if i go | 00:15 |
| davecheney | is sidnei still in zurich ? | 00:15 |
| hazmat | yup | 00:15 |
| davecheney | cool | 00:15 |
| davecheney | well, let me know | 00:15 |
| hazmat | will do | 00:16 |
| hazmat | heading out latter this week to surgecon.. aka disaster porn | 00:16 |
| hazmat | hah.. twitter feed on that topic.. "Bryan Cantrill @bcantrill ยท 8h Does systemd have you #illumos-curious? " | 00:17 |
| davecheney | lol | 00:47 |
| davecheney | systemd-make-me-a-sandwich | 00:47 |
| menn0 | hazmat, waigani: the oracle emacs integration is pretty good. I use go-oracle-referrers and go-oracle-implements fairly often. | 00:57 |
| menn0 | thumper, davecheney: is the first line of this method a little crazy? | 01:10 |
| menn0 | http://paste.ubuntu.com/8414641/ | 01:10 |
| menn0 | or is that intentional for some reason? | 01:11 |
| menn0 | thumper, davecheney: actually I can understand why it might be making a copy, but assigning the copy to u seems a little sketchy | 01:13 |
| * thumper looks | 01:13 | |
| thumper | menn0: where is the assigning to the copy? | 01:14 |
| thumper | oh | 01:14 |
| thumper | ew | 01:14 |
| menn0 | first line | 01:14 |
| thumper | wha...? | 01:14 |
| thumper | I'm guessing the reciever is effectively a value type | 01:15 |
| thumper | assigning to it means nothing other than saving declaring another value? | 01:15 |
| thumper | seems a bit esoteric | 01:15 |
| menn0 | it would be clearer if done like this: u2 := &Unit{st: u.st, doc: u.doc} | 01:16 |
| thumper | agreed | 01:16 |
| menn0 | ok. | 01:16 |
| thumper | or... unitCopy | 01:16 |
| menn0 | as long as it's not a bug or pure evil | 01:16 |
| thumper | or whatever | 01:16 |
| thumper | I don't think it is either of those | 01:17 |
| thumper | just a bit confusing | 01:17 |
| thumper | davecheney: do you agree with that assessment? | 01:17 |
| menn0 | thumper: using a longer than 2 character variable name??? That's crazy talk! This is Go! | 01:17 |
| thumper | :P | 01:17 |
| menn0 | :) | 01:18 |
| thumper | axw: you around? | 01:19 |
| thumper | axw: two things, I just noticed that we have a ci blocker, see topic ^^^ | 01:20 |
| thumper | axw: secondly I want to chat to you about the gridfs storage stuff | 01:20 |
| axw | thumper: I am here now, sorry, bit late | 01:21 |
| * axw looks at topic | 01:22 | |
| axw | gah | 01:22 |
| axw | ok, will get onto it | 01:22 |
| axw | thumper: what about gridfs? | 01:22 |
| thumper | axw: I'm munch on lunch and then catch up | 01:24 |
| axw | okey dokey | 01:25 |
| thumper | axw: mostly about how we store stuff in the paths and what impact multiple environments will have | 01:25 |
| thumper | if any | 01:25 |
| * thumper is hoping for no impact | 01:25 | |
| axw | mkay | 01:25 |
| axw | paths already encode env UUID | 01:25 |
| axw | we do need some modifications to pass in env-specific UUIDs though, atm it's just using State.Environment() to get the one and only UUID | 01:26 |
| thumper | axw: hangout? https://plus.google.com/hangouts/_/gupglu6np5t3logeqtcab2praua?hl=en | 01:37 |
| axw | thumper menn0 davecheney: trivial review please: http://reviews.vapour.ws/r/91/ (fixes CI blocker) | 02:05 |
| * menn0 looks | 02:06 | |
| thumper | axw: what happens if we run this test on arm or power? | 02:09 |
| thumper | axw: should it work? | 02:09 |
| thumper | or do we mock out the architecture elsewhere? | 02:10 |
| axw | thumper: it's uploading fake tools, should work still | 02:10 |
| axw | the UploadArches just tells the fake-tools-uploading code which arches to upload for | 02:10 |
| thumper | ok, but uploading fake tools for only i386 and amd64 | 02:10 |
| axw | it's ec2, there is only i386 and amd64 | 02:10 |
| thumper | ah, ok | 02:11 |
| menn0 | thumper, axw: i've already given it a Ship it (looks through the code to figure out the above) | 02:11 |
| thumper | axw: can I get you to add just a comment before the UploadArches? | 02:11 |
| axw | sure | 02:11 |
| axw | menn0: thanks | 02:11 |
| thumper | axw: just to say that if ec2 expands it architectures, we should add extra tools here? | 02:11 |
| axw | thumper: will do | 02:12 |
| thumper | thanks | 02:12 |
| * thumper goes to rubber stamp menn0's review | 02:12 | |
| * thumper goes to make a coffee | 02:13 | |
| davecheney | thumper: /me reads | 02:15 |
| davecheney | hmm, that is some complex code | 02:15 |
| davecheney | such side effect | 02:15 |
| davecheney | much impurity | 02:15 |
| menn0 | thumper, waigani: this units env UUID change has been tough, especially the unit watcher | 02:25 |
| menn0 | 11 failing tests in state left | 02:25 |
| menn0 | then the megawatcher change | 02:25 |
| menn0 | and then the migration | 02:25 |
| thumper | menn0: yeah, I thought it would be pretty shitty | 02:25 |
| davecheney | menn0: i *think* that line 2 on that paste does not update u | 02:26 |
| davecheney | well, it updates the copy of the pointer passed to WatachSubUnits | 02:26 |
| davecheney | but that is fine | 02:26 |
| menn0 | davecheney: that's the conclusion that thumper and I came up with too | 02:26 |
| davecheney | but it's confusing code and should be rewritten | 02:26 |
| menn0 | but it's not especially clear | 02:26 |
| * menn0 nods | 02:26 | |
| menn0 | since finding that one I've found a few other methods that do the same thing | 02:27 |
| davecheney | *u = &Unit would update the caller's copy | 02:27 |
| davecheney | actually | 02:27 |
| davecheney | *u = Unit | 02:27 |
| menn0 | yep | 02:27 |
| davecheney | u = &Unit changes the pointer value passed to you to point to something else | 02:27 |
| davecheney | it's just a bit nuts | 02:27 |
| thumper | I'm pretty sure it is explicitly creating a copy as there will be a go-routine accessing it | 02:27 |
| thumper | and this way, they avoid extra mutexes | 02:28 |
| thumper | davecheney: I agree, unclear code | 02:28 |
| thumper | menn0: I suggest you create a new variable, not reusing 'u' :) | 02:28 |
| menn0 | davecheney: as I said earlier something like u2 := &Unit{...} would have been much clearer | 02:28 |
| davecheney | it updates u in the scope of that method | 02:28 |
| davecheney | but to point to a new value | 02:28 |
| davecheney | not the valye that u points to | 02:29 |
| waigani | what code are you lot talking about? | 02:29 |
| davecheney | waigani: http://paste.ubuntu.com/8414641/ | 02:30 |
| waigani | thanks | 02:30 |
| === Ursinha-afk is now known as Ursinha | ||
| * thumper makes a sad face | 02:35 | |
| thumper | sel := bson.D{{"_id", bson.D{{"$regex", "^" + regexp.QuoteMeta(ensureActionMarker(prefix))}}}} | 02:35 |
| thumper | iter := actionsCollection.Find(sel).Iter() | 02:35 |
| thumper | bodie_: you around? | 02:35 |
| jcw4 | thumper: I may have been responsible for that code | 02:38 |
| thumper | jcw4: hey | 02:38 |
| jcw4 | hey :) | 02:38 |
| thumper | jcw4: I'm looking at the collections working out what is needed for the multi-environments | 02:38 |
| thumper | what makes up the id for an action? | 02:38 |
| thumper | also, what is the prefix normally in the above code? | 02:38 |
| thumper | as a general rule, I don't like encoding values into an id field that need to be decoded later | 02:39 |
| jcw4 | thumper: the prefix of an action id is the id of the unit to which it has been queued | 02:39 |
| jcw4 | thumper: agreed | 02:39 |
| thumper | jcw4: is an action always for a unit? | 02:39 |
| jcw4 | thumper: at this point yess | 02:39 |
| jcw4 | thumper: the intent is to allow prefixes of service id's too | 02:40 |
| thumper | jcw4: can you concieve of a time where it won't be? | 02:40 |
| thumper | hmm... | 02:40 |
| jcw4 | thumper: although even in the case that an action is queued for a service I think it will ultimately end up queued for a unit | 02:40 |
| jcw4 | thumper: so maybe not | 02:40 |
| thumper | jcw4: right, so the idea that the CLI says run this action for this service, and we create an action for each unit | 02:41 |
| jcw4 | thumper: the main rationale for the prefix is so that the watcher can easily filter | 02:41 |
| thumper | that exists at that time | 02:41 |
| jcw4 | thumper: right | 02:41 |
| thumper | jcw4: was there pushback from someone about having a Unit field explicitly and independently from the id? | 02:42 |
| jcw4 | thumper: long story - I think fwereade may have a strong opinion on it | 02:42 |
| * thumper chuckles | 02:42 | |
| jcw4 | (but I don't want to put him on the spot) | 02:42 |
| jcw4 | :) | 02:42 |
| jcw4 | thumper: when I talk to you, my initial feeling that the id should not encode the unit id is re-inforced | 02:43 |
| thumper | this may be a situation where fwereade and I both hold strong and opposing points of view | 02:43 |
| jcw4 | thumper: however when I talk to fwereade he makes compelling case too | 02:43 |
| thumper | what is the case to keep it in the id, | 02:44 |
| thumper | apart from saving a few bytes? | 02:44 |
| jcw4 | thumper: I beleive the primary reason is to make the watchers efficient | 02:44 |
| thumper | ah fark | 02:44 |
| thumper | that's wright | 02:44 |
| thumper | most of the watchers only see the id field right? | 02:45 |
| jcw4 | thumper: exactly | 02:45 |
| thumper | wright? | 02:45 |
| thumper | right! | 02:45 |
| jcw4 | haha | 02:45 |
| thumper | fudge cake | 02:45 |
| jcw4 | thumper: that being said, I think env UUID prefix on the unit would transparently flow through to the action? | 02:46 |
| thumper | I hate designing our system around problems that we have made for ourselves | 02:46 |
| davecheney | *cough* mongo | 02:46 |
| thumper | jcw4: no... | 02:46 |
| davecheney | where is wallyworld when you need him | 02:46 |
| thumper | jcw4: not exactly | 02:46 |
| thumper | jcw4: the way we are dealing with the environment uuid is as follows | 02:47 |
| * jcw4 leans forward | 02:47 | |
| thumper | jcw4: and we may want to tweak the actions collection too | 02:47 |
| thumper | jcw4: we read the full document | 02:47 |
| thumper | jcw4: and then prefix the existing _id field with "<env-uuid>:" | 02:48 |
| thumper | jcw4: also adding "env-uuid": <env-uuid> | 02:48 |
| thumper | jcw4: and changing the existing _id field to something meaningful | 02:48 |
| thumper | like "name" or "foo" | 02:48 |
| thumper | jcw4: I think it would be worthwhile also adding in "unit" | 02:49 |
| thumper | jcw4: when we do the schema migration for actions | 02:49 |
| jcw4 | thumper: prefixing the existing _id field and then adding something meaningful? | 02:49 |
| thumper | jcw4: the service document changes are now in master | 02:49 |
| thumper | jcw4: do you have an up to date master as of about 18 hours ago? | 02:49 |
| thumper | jcw4: I'll explain | 02:49 |
| jcw4 | thumper: yep | 02:49 |
| jcw4 | thumper: lemme look | 02:49 |
| thumper | hangout? | 02:49 |
| jcw4 | sure | 02:49 |
| thumper | jcw4: who do I add? | 02:50 |
| jcw4 | johnweldon4@gmail.com | 02:50 |
| thumper | kk | 02:50 |
| thumper | https://plus.google.com/hangouts/_/g424bdmxb7yrh4awwwarvkuw7ia?hl=en | 02:50 |
| thumper | jcw4: this commit: 59cfe5aff287b46de440e45dfc2aa25b34dac571 | 03:10 |
| thumper | jcw4: git diff 3c0b77b..59cfe5aff | 03:11 |
| jcw4 | thumper: git diff 59cfe5aff^..59cfe5aff | 03:16 |
| thumper | jcw4: ah, that works too? I'm assuming it takes the first parent if a merge? | 03:21 |
| jcw4 | thumper: yeah I think so | 03:49 |
| jcw4 | thumper: http://git-scm.com/book/ch6-1.html#Ancestry-References | 03:53 |
| thumper | jcw4: cheers | 03:54 |
| menn0 | thumper, waigani: almost done with the units env uuid work | 04:13 |
| thumper | menn0: awesome | 04:13 |
| waigani | menn0: nice | 04:13 |
| menn0 | thumper, waigani: just saw this in TestAddEnvUUIDToServicesIDIdempotent: | 04:13 |
| menn0 | serviceResults[0].DocID = s.state.docID(serviceName) | 04:13 |
| menn0 | } | 04:13 |
| menn0 | that's supposed to be an assertion | 04:14 |
| menn0 | thumper, waigani: we all missed it :( | 04:14 |
| waigani | yikes | 04:14 |
| thumper | haha | 04:14 |
| thumper | yep | 04:14 |
| * menn0 fixes | 04:14 | |
| waigani | thank you | 04:14 |
| menn0 | waigani, thumper: fixed. thankfully the tests pass with the assert in place | 04:15 |
| thumper | :) | 04:16 |
| waigani | few | 04:16 |
| thumper | phew? | 04:16 |
| waigani | true | 04:17 |
| * thumper whimpers | 04:29 | |
| * thumper head desks | 04:30 | |
| thumper | menn0: is the upgrades collection used solely by the state servers to synchronise updates? | 04:32 |
| menn0 | thumper: yep | 04:33 |
| menn0 | thumper: although Will and I were thinking of using it to report the status of the last upgrade in the juju status output | 04:33 |
| thumper | menn0: so, about env-uuid, should we add it? | 04:34 |
| menn0 | thumper: probably not... since the upgrade isn't environment specific | 04:34 |
| thumper | well... it sis | 04:34 |
| thumper | is | 04:34 |
| menn0 | thumper: the status thing isn't planned for any time soon | 04:34 |
| menn0 | thumper: well it is sort of | 04:34 |
| thumper | yeah, sort ov | 04:34 |
| thumper | of | 04:34 |
| menn0 | thumper: the state server upgrade aren't | 04:34 |
| thumper | ugh | 04:34 |
| menn0 | thumper: the whole upgrades with MESS thing needs to serious thought. Maybe a good topic while we're in Brussels? | 04:35 |
| thumper | probably | 04:35 |
| menn0 | thumper: for your current planning I would lean towards not including the env UUID in the upgrades doc | 04:38 |
| thumper | menn0: that is what I have decided too for now | 04:38 |
| menn0 | thumper: k | 04:39 |
| menn0 | thumper, waigani: the tests for adding env uuid to services and units are now each one line | 04:45 |
| menn0 | the test implementation is generic and works for units and services | 04:45 |
| thumper | woot woot | 04:46 |
| menn0 | it should now be much easier to add more env UUID migration steps | 04:46 |
| menn0 | about to try a similar refactoring for the upgrade steps themselves | 04:46 |
| thumper | menn0: we also need to talk about what backup means for other environments | 04:49 |
| menn0 | thumper: definitely | 04:49 |
| === thumper is now known as thumper-cooking | ||
| jcw4 | thumper-cooking: http://reviews.vapour.ws/r/92/ (fwereade and TheMue too) | 04:58 |
| menn0 | thumper-cooking: env UUID change for units here: http://reviews.vapour.ws/r/93/diff/ | 05:27 |
| axw | waigani: if you're still working, can you please take a look at my response to your review? | 05:46 |
| waigani | axw: done | 05:50 |
| axw | waigani: thanks! | 05:50 |
| axw | davecheney: you too, if you don't mind - there's two issues you raised that I didn't resolve, but commented on | 05:51 |
| davecheney | axw: /me looks | 05:54 |
| davecheney | axw: which link ? | 05:54 |
| === uru_ is now known as urulama | ||
| axw | sec | 05:54 |
| axw | davecheney: http://reviews.vapour.ws/r/82/ | 05:55 |
| davecheney | axw: i can't see your comments | 05:56 |
| axw | davecheney: nothing shows up next to your comments on the front page? | 05:57 |
| axw | davecheney: there were two things unresolved: you asked if RB screwed up the formatting; looks fine to me on RB, and go fmt is happy anyway | 05:58 |
| axw | the other was whether we should return an error from StartInstance if we can't record the instance-id | 05:58 |
| axw | and I said no, because (a) we never recorded instance-ids for non-bootstrap machines before (known deficiency), and (b) if we fail to do it for bootstrap ,we'll have other issues anyway and bootstrap will tear down | 05:59 |
| davecheney | axw: nope, can't see your comments | 06:08 |
| davecheney | did you publish them ? | 06:08 |
| davecheney | nah, RB is screwing up the indentation of some blocks | 06:09 |
| davecheney | it's happened on other reviews as well | 06:09 |
| axw | davecheney: yes, I did publish. weird, looks fine on my end | 06:09 |
| davecheney | great, it's not even showing me my comments uinless i'm on the review screeen | 06:20 |
| davecheney | it won't show them on the diff screen | 06:20 |
| davecheney | what the f | 06:20 |
| axw | davecheney: thanks. comments on the diff screen show up as a little box on the LHS, which you need to hover over | 06:28 |
| davecheney | yup, couldn't see them | 06:28 |
| davecheney | honestly given up | 06:28 |
| davecheney | your change is good | 06:28 |
| davecheney | no need to sweat the small stuff | 06:28 |
| axw | hrm, yeah not showing up for me either. maybe because I uploaded a new changeset? I dunno | 06:29 |
| === thumper-cooking is now known as thumper | ||
| axw | woot, once more step till Environ.Storage() is no more | 06:29 |
| axw | one* | 06:29 |
| thumper | \o/ | 06:30 |
| * thumper looks at the reviews | 06:30 | |
| * thumper takes a deep breath | 06:30 | |
| davecheney | axw: i've no fucks left to give rb | 06:31 |
| axw | heh :) I must say I'm not overwhelmed by it, I'd gotten used to GitHub reviews | 06:32 |
| * thumper headdesks | 06:34 | |
| thumper | fwereade: have you started? | 07:11 |
| thumper | guess not | 07:13 |
| mattyw | morning folks | 07:46 |
| dimitern | morning mattyw | 07:49 |
| dimitern | mattyw, you're OCR today, right? have a look at http://reviews.vapour.ws/r/95/ please? | 07:50 |
| mattyw | dimitern, be happy to | 07:51 |
| rogpeppe | does anyone know if it's ok to have a non-subordinate relation to juju-info ? | 08:35 |
| rogpeppe | fwereade: ^ | 08:35 |
| rogpeppe | dimitern: ^ | 08:37 |
| dimitern | rogpeppe, why not? it's not bound to subordinates afaik | 08:39 |
| rogpeppe | dimitern: i *thought* it's probably ok, but i'm just writing some relation-verification code in the charm package and wanted to check | 08:39 |
| rogpeppe | dimitern: i'll assume it's ok for now. the code in the state package doesn't look as if it complains | 08:40 |
| dimitern | rogpeppe, but in general, using juju-info is frowned upon | 08:40 |
| rogpeppe | dimitern: frowning isn't relevant here :-) | 08:40 |
| dimitern | rogpeppe, :) just saying | 08:41 |
| mattyw | dimitern, LGTM | 09:08 |
| dimitern | mattyw, cheers! | 09:08 |
| fwereade | rogpeppe, sorry I missed that -- but yeah I think it's fine | 09:16 |
| rogpeppe | fwereade: cool, thanks | 09:16 |
| fwereade | rogpeppe, pretty sure that if it wasn't we would have scoped it differently | 09:16 |
| rogpeppe | fwereade: yeah | 09:16 |
| fwereade | rogpeppe, I think one of the possible use cases was a penetration testing charm :) | 09:18 |
| rogpeppe | fwereade: nice idea | 09:18 |
| mattyw | reviewboard seems slow today - anyone else seeing that? | 09:19 |
| jam | axw: aren't you supposed to link your RB review back to the original branch somehow? (like via a github PR? ) | 10:03 |
| axw | jam: could be helpful I guess - I didn't think anyone would care about the PR | 10:38 |
| axw | I can do that tho | 10:38 |
| jam | axw: so *I* occasionally like to actually download the code, and that is hard to do from just RB | 10:41 |
| axw | fair enough | 10:42 |
| jam | axw: in this case, I trolled github and found your PR and your branch, but it would be nice to have a direct link (though even just to the git branch would be fine for my purposes) | 10:42 |
| * axw nods | 10:43 | |
| jam | though I don't think github makes it easy to see what PR's a given branch is involved in | 10:43 |
| jam | I can see what PRs are proposed to go into a branch, but is there an obvious "this branch is proposed in these PRs" ? | 10:43 |
| axw | not that I'm aware of | 10:43 |
| jam | mattyw: it seems intermittent, and I'm seeing slowdown elsewhere, so I'm wondering if it is "Internet" or RB | 10:45 |
| mattyw | jam, it feels like it's just slower on the larger reviews | 10:46 |
| jam | dimitern: standup ? | 10:47 |
| dimitern | jam, omw | 10:47 |
| perrito666 | morning | 10:54 |
| dimitern | fwereade, jam, mattyw, axw, please take a look http://reviews.vapour.ws/r/96/ | 11:16 |
| mattyw | dimitern, looking | 11:17 |
| dimitern | mattyw, ta! | 11:18 |
| mattyw | dimitern, one thing from me, might be something we need to discuss | 11:29 |
| dimitern | mattyw, I haven't changed the behavior what's allowed and what's not, I just converted the check already in place to use AuthFuncs | 11:30 |
| * mattyw looks again | 11:31 | |
| mattyw | dimitern, I see it now - that's a great point - and a potential bug - thanks very much | 11:32 |
| dimitern | mattyw, sure, np | 11:33 |
| mattyw | folks, just wanted to say I've landed some code that fails the go vet test because I forgot to enable the pre push hook - this is me saying I'm sorry, it won't happen again | 11:55 |
| jam | mattyw: I'm thinking mgz should be aware that the bot should be running go vet if we want to require it | 11:56 |
| perrito666 | mattyw: we love you anyway | 11:57 |
| jam | mattyw: I don't hold people to very high "you must be very careful doing X", but I do hold our *process* to that standard. So if something happened that shouldn't have, that is more a sign that we're missing a step. | 11:58 |
| jam | especially something like 'go vet' which is highly automatable | 11:58 |
| mattyw | perrito666, I have to assume you love me because of all this ;) | 11:58 |
| mattyw | jam, in that case think of me as a part time - "problem in process finder" | 11:58 |
| axw | fwereade: if you have any time today, I'd appreciate if you could weigh in on http://reviews.vapour.ws/r/94/ -- just to make sure I've not veered wildly off course | 11:59 |
| jam | mattyw: everybody makes mistakes in process, which is why I don't feel bad. Anytime the design of something is "as long as we're careful" that's a sign of something brittle that should be done differently. | 11:59 |
| jam | mattyw: we wouldn't need a test suite if we were just careful enough to not make mistakes :) | 12:03 |
| mattyw | jam, sounds awesome - let's do it! | 12:09 |
| rogpeppe | jam: what state is charm.v4 in? is it ok to move the charm store to use it? | 12:09 |
| jam | rogpeppe: the only change there (to my knowledge) is updating the gocheck dependency, so it will still need to have v3 merged into it | 12:09 |
| rogpeppe | jam: cool, thanks | 12:09 |
| rogpeppe | jam: i have actually been applying v3 patches to v4 too, so all should be ok | 12:10 |
| wwitzel3 | jam: any idea why I would still be getting the, ERROR unknown object type "RunCommand", error from the rpc reflection, even though I've defined an init, registered a common facade 'RunCommand', and added that import to allfacades? | 12:50 |
| perrito666 | wwitzel3: not using upload tools? | 12:51 |
| wwitzel3 | using upload-tools | 12:53 |
| perrito666 | wwitzel3: facadeversions? | 12:57 |
| perrito666 | with capital V | 12:57 |
| wwitzel3 | perrito666: ? | 12:58 |
| wwitzel3 | perrito666: isn't facadeVersions built from registered facades? | 12:58 |
| perrito666 | I am checking Backups facade to see what other steps are required | 12:58 |
| perrito666 | wwitzel3: I see a map of strings and numbers | 12:59 |
| wwitzel3 | perrito666: I do too :/ | 12:59 |
| wwitzel3 | first time noticing it, le sigh | 12:59 |
| perrito666 | it would be nice to have a facade todo list :p | 13:00 |
| wwitzel3 | I even read through TheMue's API implementation doc, and I don't recall it mentioning facadeversions, but I probably just missed it. | 13:01 |
| TheMue | wwitzel3: the current proposal? pushed it yesterday again. | 13:03 |
| jam | wwitzel3: I'd have to see your code to give you any hints | 13:04 |
| wwitzel3 | TheMue: yeah, current version, I didn't see any mention of api/facadeversions | 13:05 |
| wwitzel3 | jam: well perrito666 pointed me at facadeversions which has me getting a different error now, but it is progress :) | 13:05 |
| TheMue | wwitzel3: ah, ok, currently I only cover the server-side | 13:05 |
| jam | wwitzel3: sure, but I'll help you with it if you point me to code. | 13:06 |
| wwitzel3 | TheMue: I'll write some notes so I can help fill out the client side | 13:06 |
| perrito666 | wwitzel3: backups is very cool for that, its a very uncommon word so it makes easy to find what must be done for a faade | 13:06 |
| TheMue | wwitzel3: great, thanks | 13:07 |
| wwitzel3 | jam: https://github.com/wwitzel3/juju/blob/ww3-juju-run-with-context/apiserver/runcmd/runcmd.go | 13:07 |
| wwitzel3 | jam: and the client is https://github.com/wwitzel3/juju/blob/ww3-juju-run-with-context/api/runcmd/client.go | 13:08 |
| wwitzel3 | jam: the new error is ERROR no such request - method RunCommand(1).Run is not implemented .. that is after adding RunCommand, 1 to api/facadeversions.go | 13:09 |
| jam | wwitzel3: your Run() function takes a slice | 13:10 |
| jam | you're only allowed to put structs | 13:10 |
| jam | so if you need a slice, then you need a struct that has a slice | 13:10 |
| jam | wwitzel3: I *think* if you run at the right debug level the RPC code will tell you that it is discarding certain functions (and hopefully why) | 13:11 |
| === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: see calendar | Open critical bugs: None | ||
| jam | wwitzel3: make sense? | 13:11 |
| wwitzel3 | jam: yep, makes sense | 13:12 |
| jam | wwitzel3: so without setting facadeversions, it was probably using BestAPIVersion to then request version 0 of the facade, which didn't (and shouldn't) ever exist | 13:13 |
| jam | wwitzel3: and Run() just wasn't being exposed because it was taking a slice. | 13:13 |
| wwitzel3 | jam: thank you | 13:14 |
| wwitzel3 | perrito666: thank you | 13:14 |
| perrito666 | wwitzel3: np | 13:14 |
| mattyw | dimitern, http://reviews.vapour.ws/r/96/ ship it | 13:23 |
| mattyw | can I get a quick review from someone? I'm OCR today but I don't trust myself: http://reviews.vapour.ws/r/97/ | 13:28 |
| rick_h_ | mattyw: that's a decent excuse I suppose :P | 13:29 |
| === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: see calendar | Open critical bugs: 1373424 | ||
| sinzui | fwereade, can you comment on bug 1372016. We either have a bug in the API or we need to document that clients cannot assume that the error information is in a consistent place | 14:02 |
| mup | Bug #1372016: api errors coming back in response instead of envelope <api> <juju-core:Triaged> <https://launchpad.net/bugs/1372016> | 14:02 |
| * fwereade looks | 14:02 | |
| jam | sinzui: fwereade: I think the statement there is "if your whole request is invalid, Error is on the overall scope", if one of the pieces of your request is invalid, then that piece gets the error. | 14:03 |
| jam | since EA now takes a slice of machines, right? | 14:03 |
| === kadams54_ is now known as kadams54 | ||
| jam | (most bulk apis work this way, just because 1 entry of a slice is bad, we don't error on the whole request) | 14:04 |
| sinzui | jam, fwereade, thank you. since we don't have a time machine to fix old envs, it might be true that all API clients need to check for the error in many locations | 14:04 |
| fwereade | jam, sinzui: yes, the top-level errors are not the primary way of discovering problems at all -- if things are so bad that we can't give you the usual response, that's where the error will be | 14:04 |
| natefinch | gsamfira: can you join #juju? someone is having difficulty deploying a windows charm | 14:08 |
| gsamfira | natefinch: joined | 14:19 |
| sinzui | natefinch, can you ask someone to look into bug 1373424. I think the issue has been in the code for a long time | 14:22 |
| mup | Bug #1373424: method Client.AddMachinesV2 is not implemented <api> <ci> <compatibility> <regression> <juju-core:Triaged> <https://launchpad.net/bugs/1373424> | 14:22 |
| natefinch | sinzui: will do. | 14:23 |
| wwitzel3 | is there a good way to check the type of error? in this case I want to check if the error is one of the ERROR from the rpc, so I can attempt to call the old API client. | 14:30 |
| wwitzel3 | ahh found it, in apiserver/params/apierror | 14:32 |
| natefinch | if v, ok := err.(SomeErrorType) ; ok { // it's that type, v is the value of that type } | 14:36 |
| natefinch | wwitzel3: standup? | 15:06 |
| gsamfira | heya guys. This PR: https://github.com/juju/utils/pull/27 has been reviewed. Can we merge it? | 15:09 |
| === arosales_ is now known as arosales | ||
| === fabrice is now known as fabrice|family | ||
| katco | mattyw: can i get a (hopefully) final review and "Ship It!" (if appropriate) on http://reviews.vapour.ws/r/66/? | 16:22 |
| mattyw | katco, of course | 16:23 |
| katco | mattyw: thanks :) | 16:24 |
| === jheroux_away is now known as jheroux | ||
| === urulama is now known as urulama-afk | ||
| mattyw | katco, done, not quite "Ship it" from me I'm afraid | 16:45 |
| katco | mattyw: no worries, doing a good job. | 16:46 |
| katco | mattyw: i struggle with the split comments however. all that does is just shift the functions to a broader scope and possibly introduce new parameters due to lack of closure | 16:46 |
| katco | mattyw: defining several functions within a function is a pattern, and it's a bit like defining a class w/o the boilerplate. | 16:47 |
| mattyw | katco, I'm fine with the functions being unexported and only being used in FormatSummary. For me it's all about how much I can keep in my head. I don't mind a function having a number of inputs as long as I can quickly work out what it's doing. When functions get long I start getting confused. There's the rule of thumb that a person can remember about 7 "things". With closure I have to remember what they do but also work | 16:52 |
| mattyw | out what parts they're closing over | 16:52 |
| mattyw | katco, if it's all split out that I can usually look at a single function, work out what it does then only need to remember the one thing it does when I go back to what's calling it | 16:52 |
| katco | mattyw: can't you do that within the scope of a function? | 16:53 |
| katco | mattyw: closures are exactly equivalent to classes, except you can't modify the member variables they close over. | 16:53 |
| katco | (outside the scope of what they close over) | 16:54 |
| mattyw | katco, yes, but the larger the scope the harder is is to do | 16:54 |
| katco | mattyw: which is equivalent to "the more member variables, the harder..." i think | 16:54 |
| katco | mattyw: well, you're the second one to bring it up, so i'll move one scope-level higher and make it a class | 16:58 |
| mattyw | katco, sorry - I wasn't ignoring you I was just thinking | 16:58 |
| katco | mattyw: no worries at all | 16:58 |
| mattyw | katco, I don't think you need to go that far, I agree with you that it adds boiler plate | 16:58 |
| mattyw | katco, and as you say the more fields the harder it is to work out what's going on | 16:59 |
| mattyw | katco, I think if you just turned the closures in that function into seperate functions that would be a good start - conceptually nothing much would have changed, but the FormatSummary function would be shorted and I think easier to read | 17:00 |
| katco | mattyw: i guess i just don't like the idea of them floating around when they're only hyper-specific to the summary formatter | 17:00 |
| mattyw | katco, I sort of know what you mean | 17:01 |
| mattyw | katco, I agree. But the reason they are floating around is because people are weak. They're floating around to make a function with lots of things to do look smaller | 17:02 |
| katco | mattyw: mmm... like if this were java, and the summary function were a class, this would look exactly the same indentation-level wise | 17:03 |
| katco | mattyw: i am trying not to be too obstenant, but i'm struggling with why it's easy to refer to top-level functions, but not variables which are functions | 17:04 |
| wwitzel3 | katco: it is about the noise ratio, with the variables, I have to stop reading. with extracted methods, I can read to the end, I don't have to remember to stop. | 17:06 |
| wwitzel3 | katco: so as I'm reading through format summary, I am getting bogged down with the implementation specifics of each step | 17:06 |
| wwitzel3 | katco: when what I really want to read is each step, then go see the specifics, when/if/should i care to | 17:07 |
| katco | wwitzel3: i guess i just scan over closures unless i care what they do. sounds like i'm a weirdo ;) | 17:08 |
| === ev__ is now known as ev | ||
| wwitzel3 | no you're just a lisp programmer / emacs user | 17:08 |
| katco | wwitzel3: rofl | 17:08 |
| wwitzel3 | oh right, you said weirdo .. correct | 17:08 |
| wwitzel3 | :P | 17:08 |
| === JoshStrobl is now known as JoshStrobl[AFK] | ||
| katco | bam! | 17:08 |
| katco | and to be fair, i think i can only barely claim to understand lisp. i've not used it that much. but i am trying to change that ;) | 17:12 |
| mattyw | katco, you are an emacs user though right? | 17:21 |
| katco | mattyw: absolutely | 17:21 |
| mattyw | wwitzel3, vim? | 17:22 |
| katco | mattyw: i make fun of myself, but i love that editor | 17:22 |
| mattyw | katco, I moved from emacs to vim I'm afraid | 17:23 |
| katco | mattyw: the only thing i argue vehemently to people is that they learn 1 editor deeply | 17:23 |
| katco | mattyw: i think it's unproductive to argue over what works for you hehe | 17:23 |
| mattyw | katco, +1000 | 17:24 |
| wwitzel3 | mattyw: yeah, vim :) | 17:25 |
| wwitzel3 | katco: +1 on learning one editor really well | 17:27 |
| katco | wwitzel3: it's like right above your brain on the programming stack. really important. | 17:28 |
| wwitzel3 | katco: yep, I try to go through some commands so I don't forget them, even if I haven't needed some of them in a while. | 17:29 |
| wwitzel3 | katco: I hate the context switch of .. oh how I do do that? | 17:29 |
| katco | wwitzel3: see in emacs, that's just C-s (get up and flush the toilet) M-` 1 2 3 Q | 17:30 |
| katco | wwitzel3: do you do vimgolf? | 17:30 |
| wwitzel3 | katco: haha, I did for a bit, it makes you faster for a while | 17:31 |
| wwitzel3 | then it makes you slower trying to find obscure ways to avoid keystrokes | 17:31 |
| wwitzel3 | until eventually vim is a regex parser | 17:31 |
| katco | rofl | 17:31 |
| wwitzel3 | natefinch: heading to moonstone | 17:32 |
| natefinch | I like thinking about my code not my editor. That's why I use "dumb" gui editors like sublime :) | 17:32 |
| natefinch | wwitzel3: omw too | 17:32 |
| natefinch | I know like 3 hotkeys that are specific to sublime and that's it | 17:32 |
| mattyw | calling it a day folks, see you all tomorrow | 17:35 |
| katco | mattyw: ty for the review, i'll make those changes | 17:36 |
| mattyw | katco, ping me when you want to take another look | 17:37 |
| katco | mattyw: will do | 17:37 |
| mattyw | night all | 17:37 |
| natefinch | hazmat: HA meeting? | 18:02 |
| hazmat | yup | 18:03 |
| sinzui | natefinch, this job is failing. I changed the test to help juju pass http://juju-ci.vapour.ws:8080/job/functional-ha-backup-restore/759/console I see (error: cannot re-bootstrap environment: restore does not support HA juju configurations yet) and it worries me. this looks like a policy change that breaks a test | 18:34 |
| perrito666 | sinzui: :| what? | 18:35 |
| perrito666 | that has always been there and I never in my life saw it triggered | 18:35 |
| === mup_ is now known as mup | ||
| perrito666 | ok, so suddenly environs.Environ StateServerInstances is returning something differnet | 18:45 |
| sinzui | perrito666, All three tries with the revised test consistently show "error: cannot re-bootstrap environment: restore does not support HA juju configurations yet" I recall the rule is that the restore returns a single state-server and the user can run --ensure-ha again | 18:54 |
| sinzui | perrito666, there was traffic on one the mailing lists where we recommend to backup the state-servers in HA because it is always a last restore in a catastrophe | 18:56 |
| perrito666 | sinzui: the thing is, what restore does not support is to restore while other state servers are up and running | 19:01 |
| sinzui | perrito666, right, and the test shows they were deleted using nova. | 19:02 |
| === jog_ is now known as jog | ||
| perrito666 | sinzui: I do see the deletion taking place after the error | 19:05 |
| sinzui | perrito666, the console logs are out of order because the error is reported last. when I see http://juju-ci.vapour.ws:8080/job/functional-ha-backup-restore/760/console, I see the 3 state-servers deleted, then the restore is started, but it fails, then the status/destroy-env cleanup code is run, then we see the error message from rstore | 19:10 |
| * sinzui changes test to print the error immediately so that the older of events is clear | 19:11 | |
| natefinch | katco: do you know if your team has scheduled time to make the debug-log fix from the pain points document? | 19:13 |
| natefinch | katco: (that the filter requires using the internal juju identifier, not the CLI-style unit/0 identifier) | 19:13 |
| katco | natefinch: um, i know ian sent out some bugs to work on while he was out if we ran out of things to do | 19:16 |
| katco | natefinch: let me check those | 19:16 |
| * sinzui re-runs test with better output | 19:16 | |
| perrito666 | sinzui: tx | 19:17 |
| katco | natefinch: are you referring to the customer pain points doc? | 19:18 |
| natefinch | katco: correct | 19:21 |
| katco | natefinch: he prioritized the apt-related bugs for axw and i | 19:21 |
| natefinch | katco: ok | 19:21 |
| katco | natefinch: and for me, that was prioritized behind the status work (also in that document) | 19:21 |
| natefinch | katco: ok, cool. I was thinking of having one of the people on my team tackle the debug-log change and didn't want to overlap. Sounds like there wouldn't be overlap, so that's good. | 19:22 |
| katco | natefinch: yeah i think you're gtg. | 19:22 |
| katco | natefinch: i am half-remembering that i thought someone was looking at this already. for some reason voidspace comes to mind | 19:23 |
| natefinch | katco: ok, I'll just email out on the list, then. no big rush | 19:23 |
| perrito666 | sinzui: lemme know when you have logs | 19:24 |
| sinzui | perrito666, I am watching http://juju-ci.vapour.ws:8080/job/functional-ha-backup-restore/761/console, and it think we are 45 minutes away from completion | 19:26 |
| perrito666 | sinzui: :( ok, ill take a look in 45 | 19:26 |
| sinzui | perrito666, It is failing at this minute: http://juju-ci.vapour.ws:8080/job/functional-ha-backup-restore/761/console | 19:50 |
| perrito666 | sinzui: very odd, I do see something different | 20:02 |
| perrito666 | WARNING: Could not find the instance_id in output | 20:02 |
| perrito666 | which comes from assess_recovery.py line ~94 | 20:02 |
| hazmat | axw, ping re https://bugs.launchpad.net/juju-core/+bug/1373592 | 20:02 |
| mup | Bug #1373592: When bootstrapping select-zone could retry if the instance-type is full in the region <papercut> <juju-core:New> <https://launchpad.net/bugs/1373592> | 20:02 |
| perrito666 | sinzui: and it suggests that at that point restore failed with the HA error once | 20:03 |
| perrito666 | and then you delete and after that it fails again with the same | 20:03 |
| sinzui | perrito666, your script is old. | 20:04 |
| sinzui | perrito666, update to pull the test changes | 20:04 |
| sinzui | perrito666, the failure you are seeing is because the test wanted to get the instance_id that was rejected. It isn't used, so I change the test to just warn that juju no longer tells us what was rejected. | 20:06 |
| perrito666 | sinzui: my script? I am copying that from jenkins | 20:15 |
| sinzui | perrito666, sorry, more ambiguity. the error is note the test, the test is showing why juju refused to restore to a working state-server (juju-restore correctly refused to restore because the state-server was still up.). That is correct. The test once looked for the instance_id in the text. Once we are certain juju wont do something dangerous, the script will remove all 3 state-servers, then we see "starting restore", followe | 20:18 |
| sinzui | d by "Restore Failed" | 20:18 |
| perrito666 | sinzui: so the test should no longer look for the instance_id_ | 20:19 |
| perrito666 | ? | 20:19 |
| sinzui | perrito666, that is right. the test is correct. Why is juju showing a message that contradicts what CTS advises when we restore (error: cannot re-bootstrap environment: restore does not support HA juju configurations yet) | 20:21 |
| perrito666 | sinzui: what I am trying to clarify is, if the test no longer looks for the instance_id is why jenkins still shows the error as if the test was looking for it | 20:22 |
| sinzui | perrito666, because I wanted a warning in place. I think the missing instance_id means juju's has intentionally introduced a regression | 20:23 |
| perrito666 | sinzui: ok, since restore has not changed I will guess that this is an unrelated regression that broke restore by accident, ill try to find it | 20:24 |
| perrito666 | btw something else the description on the jenkins job is odd | 20:25 |
| perrito666 | the revision for the first failure is 0fafcd986aa7501e9796519a2c432c16fe5b231f | 20:25 |
| perrito666 | yet it says: gitbranch:master:github.com/juju/juju r0fafcd98 | 20:26 |
| perrito666 | shouldnt the hex be the shortform for the sha? | 20:26 |
| perrito666 | bbl | 20:28 |
| === ChanServ changed the topic of #juju-dev to: https://juju.ubuntu.com | On-call reviewer: see calendar | Open critical bugs: 1373424 1373611 | ||
| sinzui | perrito666, I need to remove the 'r' the 'r' is from the days of testing bzr revnos | 20:35 |
| fwereade | natefinch, if thumper wasn't fixing the debug-log mess I don't think anyone else was | 20:47 |
| fwereade | natefinch, I would be most grateful if you would unfuck that | 20:47 |
| natefinch | fwereade: heh ok | 20:47 |
| thumper | fwereade: ping? | 21:38 |
| perrito666 | sinzui: back | 21:38 |
| === jheroux is now known as jheroux_away | ||
| fwereade | thumper, heyhey, just having a ciggie, with you in 5 | 21:54 |
| thumper | fwereade: ack | 21:54 |
| perrito666 | sinzui: ping me when you have a moment | 21:55 |
| sinzui | perrito666, I am about | 21:55 |
| perrito666 | ok just if you can resolve my doubt about the commit hash so I know where to start looking | 21:58 |
| sinzui | perrito666, I will arrange for that annoying 'r' to be removed tomorrow from future hashes. Maybe I can also get the branch and hash added to the tests that are missing that info | 22:02 |
| perrito666 | sinzui: duhhh | 22:03 |
| perrito666 | sorry I hadnt noticed the r | 22:03 |
| perrito666 | my apologies | 22:03 |
| * perrito666 prepares coffee | 22:03 | |
| perrito666 | sinzui: well Iam looking at the offending commit.. as usual, does not seem to change anything relevant... such is life | 22:05 |
| sinzui | perrito666, no need to apologise. Humans should work with Jenkins. We are adding links to the s3 data now. next week, engineers can go to pages like this to download everything that the tests collected http://reports.vapour.ws/releases/1871 | 22:05 |
| sinzui | perrito666, there are three suspects https://bugs.launchpad.net/juju-core/+bug/1373611 | 22:05 |
| mup | Bug #1373611: cannot restore a HA state-server <backup-restore> <ci> <ha> <regression> <juju-core:Triaged> <https://launchpad.net/bugs/1373611> | 22:05 |
| sinzui | perrito666, and I agree non look like they wanted to change restore | 22:06 |
| perrito666 | whyy whyyy always something breaks the commit is so long | 22:08 |
| waigani | On ReviewBoard, is there a way/place to see a list of review requests that you are involved in? | 22:13 |
| waigani | or a list or your reviews? | 22:15 |
| ericsnow | waigani: 2 places: "Outgoing" and via some of the columns | 22:16 |
| perrito666 | sinzui: it does not really matter that filter should be removed since I made restore HA compatible, pushing a fix | 22:17 |
| waigani | ericsnow: what I'm actually after is a list of my reviews - is that possible? | 22:17 |
| waigani | I did a bunch yesterday and now I need to follow up | 22:18 |
| ericsnow | waigani: one of the columns is "My Comments" (it shows a colored square with a curly right edge) | 22:19 |
| waigani | ericsnow: right, just found that, thanks | 22:19 |
| ericsnow | waigani: np | 22:19 |
| perrito666 | who is OCR? | 22:24 |
| perrito666 | http://reviews.vapour.ws/r/101/ | 22:24 |
| perrito666 | or who would be so nice to R this anyway, it is extremely trivial | 22:24 |
| perrito666 | sinzui: that patch fixes the issue | 22:25 |
| perrito666 | I am not sure if this is a regression or what in the universe | 22:25 |
| sinzui | thank you perrito666. I really appreciate your fast response | 22:26 |
| thumper | perrito666: if it works in HA, I'm +1 | 22:48 |
| perrito666 | thumper: sinzui my theory is that a bug that had environs.Environ StateServerInstances returning the wrong number of instance ids was fixed and that is why that condition triggered | 23:30 |
| perrito666 | after I finish to put the groceries I just bought Ill merge it tx for the review thumper and ericsnow | 23:31 |
| menn0 | thumper: can you remind me of how to get a State connected to another env in tests? you mentioned something recently. | 23:50 |
| thumper | menn0: st.ForEnviron(tag) | 23:50 |
| menn0 | thumper: cheers. I had been searching but it wasn't jumping out. | 23:51 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!