| wallyworld | i removed the tabular text bit as all user commands like that are tabular | 00:00 |
|---|---|---|
| wallyworld | no need to mention it every time imho | 00:00 |
| menn0 | wallyworld: LGTM. just needs a line wrapping fix. | 00:01 |
| wallyworld | ta | 00:01 |
| wallyworld | damn, i meant to delete that bit | 00:01 |
| alexisb | alrigty all I am off for the day | 00:04 |
| menn0 | thumper or wallyworld: https://github.com/juju/juju/pull/6398 | 00:07 |
| wallyworld | looking | 00:13 |
| babbageclunk | menn0: Ugh - I can't get my watcher to see a model I create in my test. | 00:15 |
| menn0 | babbageclunk: stink | 00:16 |
| menn0 | babbageclunk: do you want me to look at the code? | 00:16 |
| babbageclunk | menn0: Yes please! just pushing it now. | 00:17 |
| babbageclunk | menn0: After I create a new model here: https://github.com/juju/juju/commit/94f48e609342c26c494c36155fb0df21608f3624#diff-6734d4932d3aead490d5ec768a7df6b7R519 | 00:19 |
| babbageclunk | menn0: I never see it logged in the changes from here: https://github.com/juju/juju/commit/94f48e609342c26c494c36155fb0df21608f3624#diff-19f910c28876da8a8d94937e102b2ebeR686 | 00:20 |
| babbageclunk | menn0: More importantly, I never see it die after the model.Destroy. | 00:20 |
| wallyworld | menn0: your pr looks really nice | 00:22 |
| menn0 | wallyworld: cheers... I got a bit obsessive :) | 00:23 |
| wallyworld | that is a good thing :-) | 00:23 |
| menn0 | babbageclunk: i've been digging for a bit | 00:30 |
| menn0 | babbageclunk: what you've got looks reasonable | 00:30 |
| menn0 | still loooking | 00:30 |
| menn0 | also looking | 00:30 |
| menn0 | :) | 00:30 |
| babbageclunk | Thanks! | 00:31 |
| babbageclunk | I think I need to go to bed - maybe it'll be obvious (later) in the morning. | 00:32 |
| babbageclunk | menn0: ^ | 00:32 |
| menn0 | babbageclunk: ok, i'll email if I see anything | 00:32 |
| menn0 | babbageclunk: good night | 00:32 |
| babbageclunk | night! | 00:33 |
| * babbageclunk craches | 00:33 | |
| babbageclunk | ugh | 00:33 |
| babbageclunk | dumb fingers | 00:33 |
| menn0 | wallyworld or axw: are the status values shown under machine-status in the yaml status output provider specific? | 01:09 |
| menn0 | wallyworld/axw: never mind I figured it out. they're not. | 01:10 |
| wallyworld | menn0: we map provider specific codes onto those generic status values | 01:20 |
| menn0 | wallyworld: yep thanks. I figured out immediately after asking | 01:24 |
| redir | wallyworld: needs more tests, but PTAL and I'll follow up tomorrow: https://github.com/juju/juju/pull/6399 | 01:46 |
| redir | or axw ^ | 01:46 |
| wallyworld | ok | 01:46 |
| redir | wallyworld: expect typos... Just got things changed and the currect tests updates. Haven't made a pass for spelling or grammar | 01:48 |
| wallyworld | ok, np | 01:48 |
| natefinch | axw: you around? I have questions on login/logout tests | 01:56 |
| axw | natefinch: yes, but about to go into a meeting | 01:57 |
| natefinch | axw: ok, let me know when you're out | 01:57 |
| axw | natefinch: finished | 02:23 |
| natefinch | axw: want to do a hangout? Might be easier | 02:24 |
| axw | natefinch: sure | 02:24 |
| axw | natefinch: https://plus.google.com/hangouts/_/canonical.com/andrew-nate&authuser=1 | 02:25 |
| axw | eek, that doesn't work | 02:25 |
| natefinch | heh | 02:25 |
| natefinch | https://hangouts.google.com/hangouts/_/canonical.com/core?pli=1&authuser=2 | 02:25 |
| menn0 | wallyworld: follow on from my earlier PR: https://github.com/juju/juju/pull/6400 | 02:49 |
| wallyworld | menn0: ok, just eating will look real soon | 02:49 |
| menn0 | wallyworld: no rush | 02:49 |
| wallyworld | menn0: swap you https://github.com/juju/juju/pull/6401, an easy one | 03:10 |
| menn0 | wallyworld: ok | 03:10 |
| menn0 | wallyworld: but Brokener is such a great name :) | 03:11 |
| * wallyworld gets the sick bag | 03:11 | |
| menn0 | Breakable :p | 03:12 |
| wallyworld | much better | 03:12 |
| menn0 | CanHazBroken | 03:12 |
| wallyworld | even betta!!! | 03:12 |
| wallyworld | love that one | 03:12 |
| wallyworld | why was that not a convention | 03:12 |
| wallyworld | CanHazAddress is much better than Addresser | 03:13 |
| menn0 | wallyworld: I'll come up with something else | 03:16 |
| menn0 | wallyworld: I don't understand your PR :-/ | 03:16 |
| wallyworld | :-( | 03:16 |
| menn0 | wallyworld: well what came before it | 03:16 |
| wallyworld | menn0: i like Breakable :-) | 03:16 |
| menn0 | wallyworld: i'll probably go with that actually | 03:16 |
| wallyworld | is it the --show-secrets behaviour? | 03:17 |
| menn0 | wallyworld: well yes | 03:18 |
| wallyworld | so, by default list-credentials will not prints passwords etc | 03:18 |
| wallyworld | it needs to query the provider to get the schema to know what attrs are secret | 03:18 |
| wallyworld | to query the provider, it needs to look up the cloud details | 03:18 |
| wallyworld | so if the cloud name is invalid, it can't get the credential schema | 03:19 |
| wallyworld | so it errs not to print anything other than a message | 03:19 |
| menn0 | wallyworld: but why show creds for a removed cloud at all? | 03:20 |
| wallyworld | because that entry is in the file | 03:20 |
| wallyworld | and list-credentials needs to display the contents of the file IMHO | 03:20 |
| wallyworld | maybe the user wants to add that cloud back later | 03:21 |
| menn0 | wallyworld: ok | 03:21 |
| menn0 | wallyworld: I'll suggest a simpler wording on the PR | 03:21 |
| wallyworld | ok | 03:21 |
| menn0 | wallyworld: I think that's what threw me | 03:21 |
| wallyworld | i think what's there gives the necessary info, so if it's just a wording tweak | 03:22 |
| wallyworld | i wouldn't want to not convey that info | 03:22 |
| menn0 | how about: | 03:24 |
| menn0 | The following clouds no longer exists but credentials for them still exist. | 03:24 |
| menn0 | Use --show-secrets to display credentials these credentials. | 03:24 |
| menn0 | ... | 03:24 |
| menn0 | wallyworld: ^ | 03:24 |
| menn0 | ignoring the doubled up word :-/ | 03:24 |
| wallyworld | sgtm | 03:24 |
| menn0 | ok | 03:25 |
| menn0 | wallyworld: done | 03:25 |
| wallyworld | ta | 03:25 |
| menn0 | now to land this Brokener change.... | 03:25 |
| menn0 | :-p | 03:25 |
| blahdeblah | Who wouldn't want a Brokener? | 03:26 |
| blahdeblah | wallyworld: I can't believe you don't like that name | 03:27 |
| wallyworld | call me fussy :-) | 03:27 |
| wallyworld | i really hate how Go appends "er" to everything | 03:27 |
| wallyworld | sounds horrible most of the time | 03:27 |
| blahdeblah | Yeah - I think it should have used "inator" instead | 03:28 |
| wallyworld | or "CanHaz...." | 03:28 |
| blahdeblah | A Brokeinator would totally pwn a Brokener or a CanHazBroken | 03:29 |
| thumper | wallyworld, menn0, axw: https://github.com/juju/juju/pull/6402 | 03:37 |
| thumper | someone | 03:37 |
| axw | looking | 03:37 |
| wallyworld | anyone | 03:37 |
| thumper | :P | 03:38 |
| menn0 | thumper: done | 03:41 |
| * thumper off to jitz | 03:45 | |
| thumper | will land when I'm back | 03:45 |
| thumper | after tweaking year | 03:45 |
| === thumper is now known as thumper-afk | ||
| menn0 | wallyworld: another look at https://github.com/juju/juju/pull/6400 pls | 03:58 |
| wallyworld | ok | 03:58 |
| menn0 | now with Breakable | 03:58 |
| menn0 | and I caught some tests that needed updating | 03:58 |
| wallyworld | thank goodness | 03:58 |
| wallyworld | menn0: still looks good, ta | 04:00 |
| mup | Bug #1190985 changed: Confusing upgrade-charm and deploy -u behavior <deploy> <docs> <improvement> <upgrade-charm> <juju-core:Expired> <https://launchpad.net/bugs/1190985> | 04:21 |
| mup | Bug #1521610 changed: Upgrade hung when moving from 1.18.4.3 to 1.24.7 <canonical-is> <juju-core:Expired> <https://launchpad.net/bugs/1521610> | 04:21 |
| mup | Bug #1583683 changed: juju thinks it is upgrading after a reboot <juju-core:Expired> <juju-core 1.25:Won't Fix> <https://launchpad.net/bugs/1583683> | 04:21 |
| mup | Bug #1190985 opened: Confusing upgrade-charm and deploy -u behavior <deploy> <docs> <improvement> <upgrade-charm> <juju-core:Expired> <https://launchpad.net/bugs/1190985> | 04:30 |
| mup | Bug #1521610 opened: Upgrade hung when moving from 1.18.4.3 to 1.24.7 <canonical-is> <juju-core:Expired> <https://launchpad.net/bugs/1521610> | 04:30 |
| mup | Bug #1583683 opened: juju thinks it is upgrading after a reboot <juju-core:Expired> <juju-core 1.25:Won't Fix> <https://launchpad.net/bugs/1583683> | 04:30 |
| mup | Bug #1190985 changed: Confusing upgrade-charm and deploy -u behavior <deploy> <docs> <improvement> <upgrade-charm> <juju-core:Expired> <https://launchpad.net/bugs/1190985> | 04:33 |
| mup | Bug #1521610 changed: Upgrade hung when moving from 1.18.4.3 to 1.24.7 <canonical-is> <juju-core:Expired> <https://launchpad.net/bugs/1521610> | 04:33 |
| mup | Bug #1583683 changed: juju thinks it is upgrading after a reboot <juju-core:Expired> <juju-core 1.25:Won't Fix> <https://launchpad.net/bugs/1583683> | 04:33 |
| === thumper-afk is now known as thumper | ||
| thumper | axw: I have a question about your review | 05:46 |
| axw | thumper: yup? | 05:46 |
| thumper | axw: I didn't change the time that RunHook took at all | 05:47 |
| thumper | it was already returning cmd.Wait() | 05:47 |
| thumper | all I do is save the output after the Wait call | 05:47 |
| thumper | before we were discarding the output | 05:47 |
| thumper | that's all | 05:47 |
| axw | thumper: what I'm saying is that the client won't get to see the stdout/stderr of the hook until after it's finished | 05:47 |
| thumper | ah | 05:48 |
| thumper | hmm... | 05:48 |
| axw | thumper: oh ... yeah, we weren't setting stdout/stderr | 05:48 |
| axw | huh, how do we see the output | 05:48 |
| thumper | isn't it the tmux session that is started? | 05:49 |
| thumper | I'm not sure | 05:49 |
| axw | thumper: ahhh yes | 05:49 |
| axw | thumper: never mind me then | 05:49 |
| thumper | how can I do QA on this | 05:49 |
| thumper | I'd like to check before landing | 05:49 |
| thumper | I don't think I've ever used debug-hooks in anger | 05:49 |
| axw | thumper: umm. I guess create a charm with a config-changed hook that generates some stdout. then deploy it, debug-hooks, and force a config-changed | 05:50 |
| axw | thumper: we don't actually run the hook at all ... we just trap and then you can run it yourself | 05:50 |
| axw | thumper: so my review was garbage | 05:50 |
| thumper | yeah... | 05:50 |
| thumper | my problem is now that it is almost 7pm on friday | 05:50 |
| thumper | and I have run out of fucks | 05:51 |
| thumper | but I don't want to land something that could potentially screw tings up | 05:51 |
| thumper | so I need to test it | 05:51 |
| wallyworld | axw: if you get a chance, could you look at this for me? after you recover from being hassled by thumper https://github.com/juju/juju/pull/6405 | 05:52 |
| axw | heh | 05:52 |
| axw | wallyworld: sure | 05:52 |
| wallyworld | ta | 05:52 |
| axw | thumper: I think it's fine to land as is. a smoke test of debug-hooks would be good, but I think fine to skip in this case | 05:54 |
| axw | thumper: I do think it would be a bit neater to pass Stdout/Stderr in as a param, but not fussed | 05:54 |
| axw | (just so you don't have to reach in via export_test) | 05:54 |
| thumper | I thought that RunHook might have been an interface function | 05:55 |
| thumper | so signature mattered | 05:55 |
| axw | thumper: it is. I meant pass in via the HookContext | 05:55 |
| thumper | ah... | 05:56 |
| thumper | ok | 05:56 |
| thumper | yeah, that's better | 05:56 |
| thumper | axw: reworked how to get the combined output, https://github.com/juju/juju/pull/6402 | 06:07 |
| axw | looking | 06:07 |
| thumper | so no change to the code for non-test | 06:07 |
| axw | thumper: looks better, thanks. approved | 06:09 |
| thumper | axw: ta | 06:09 |
| thumper | and on that note | 06:09 |
| thumper | I'm off to start drinking | 06:09 |
| thumper | later folks | 06:09 |
| axw | enjoy | 06:10 |
| wallyworld | axw: with error code, you never get a not found from that api - it is always ErrPerm if the model is not there | 06:29 |
| axw | wallyworld: yeah I know, I'm just saying using that function would make it clearer why you're looking for that error | 06:30 |
| axw | wallyworld: alternatively, please add a comment | 06:30 |
| wallyworld | ok | 06:30 |
| wallyworld | axw: also, i was trying to avoid having to add an api. ModelInfoResult does contain the number of machines in the model, but not number of applications. I could just print the number of machines | 06:31 |
| wallyworld | or I guess I could add to the struct and the applications bit won't be there for rc1 and rc2 | 06:32 |
| axw | wallyworld: could we just make a call to FullStatus? | 06:33 |
| wallyworld | we could. seems expensive though just to poll for info like that | 06:34 |
| axw | wallyworld: on the client facade | 06:34 |
| wallyworld | i'd prefer not to tbh | 06:34 |
| axw | wallyworld: why? we want to know the model status... | 06:34 |
| wallyworld | the thing we really want is for the command to block | 06:35 |
| wallyworld | printing the number of machines and apps is a bonus | 06:35 |
| wallyworld | imho | 06:35 |
| wallyworld | i'm worried about calling full status every 2 seconds | 06:36 |
| wallyworld | maybe it's nothing to worry about | 06:36 |
| axw | wallyworld: what's your concern? | 06:36 |
| axw | I don't think we should be concerned about getting status frequently | 06:36 |
| wallyworld | sec | 06:37 |
| wallyworld | status is fairly heavy weight last time i looked - it did a bit to gather all the info, and then all we would do is throw most of it away here simply to print a machine/app count | 06:39 |
| wallyworld | if we do want to do that, we have a ModelInfo api with machine info | 06:39 |
| wallyworld | we'd just need to add application count | 06:40 |
| axw | wallyworld: I'm fine with that. and it would be ok if you only showed the machine count for an rc3 server | 06:40 |
| wallyworld | on large models, status used to tke minutes and minutes to run | 06:40 |
| wallyworld | ok | 06:40 |
| axw | wallyworld: that's a problem that we need to fix :) | 06:41 |
| axw | if it's still a problem | 06:41 |
| wallyworld | right, but for this bug fix, i'd rather use something lightweight. status might be fixed, but it is still heavy weight i think | 06:41 |
| axw | yep, fair enough | 06:42 |
| === frankban|afk is now known as frankban | ||
| axw | wallyworld: sorry, but Controller.ModelStatus isn't suitable :( it requires superuser | 08:12 |
| axw | so you wouldn't be able to destroy your own models if you're non super user | 08:13 |
| wallyworld | really? sigh | 08:13 |
| wallyworld | axw: yeah, you're right. but i think we should change the perm on that method | 08:14 |
| wallyworld | if you had permissions on the model, you should be able to get its status | 08:14 |
| axw | wallyworld: fine by me. superuser || model-owner | 08:14 |
| axw | or | 08:14 |
| wallyworld | will have to fix after soccer | 08:14 |
| axw | model admin or whatever | 08:15 |
| wallyworld | at least i didn't need a new api | 08:15 |
| axw | wallyworld: yeah. it does mean that rc3 would be brokenish, but I think that's ok | 08:15 |
| wallyworld | i think so too | 08:15 |
| wallyworld | we can release note it | 08:15 |
| wallyworld | right, off to soccer, bbiab | 08:16 |
| dooferlad | voidspace / babbageclunk: Small branch for review if you have the time: https://github.com/juju/juju/pull/6406 | 09:43 |
| babbageclunk | dooferlad: looking | 09:52 |
| babbageclunk | dooferlad: lgtm | 10:04 |
| babbageclunk | dooferlad: with a couple of comments | 10:05 |
| macgreagoir | dooferlad: I guess there is no golang way to get the fs reserved space number, rather than a magic 90%. I won't look too hard if you already have. You might assume 5% in the fs, so 10% isn't a bad one. | 10:11 |
| dooferlad | macgreagoir: I am sticking to magic for now because it gets us out of the woods. It would be great to be able to pass in something on the CLI so the user can restrict / expand as wanted or do something based on monitoring the host. | 10:20 |
| macgreagoir | tune2fs -l :-) but we don't want to assume that. | 10:21 |
| macgreagoir | Assuming to reserve twice the fs default as reserved is not so bad. | 10:22 |
| dooferlad | macgreagoir: I was talking about the Juju CLI | 10:23 |
| dooferlad | macgreagoir: I know about the tune2fs thing and leaving some space on the host seems good too. | 10:24 |
| macgreagoir | Oh, aye, I mean a way to get a non-magic number. | 10:24 |
| voidspace | dooferlad: as part of provisioning a new machine (cloud-init) we reboot, right? | 10:42 |
| dooferlad | voidspace: I don't think we reboot at the end of cloud-init | 10:44 |
| voidspace | dooferlad: ah | 10:44 |
| dooferlad | voidspace: pretty sure that cloud-init runs on first boot after install. | 10:45 |
| voidspace | dooferlad: we'd like to up the open file limit, but that requires a reboot - unless we get into the image I guess | 10:45 |
| dooferlad | voidspace: you can do that on the fly | 10:46 |
| dooferlad | voidspace: just looking that up | 10:46 |
| voidspace | dooferlad: not inotify user_watches or max_instances | 10:46 |
| dooferlad | voidspace: sysctl -w fs.file-max=100000 | 10:47 |
| voidspace | dooferlad: ah, ok - interesting I'll try it | 10:47 |
| voidspace | dooferlad: we've been looking at setting it in /etc/security/limits.conf which requires a reboot | 10:47 |
| dooferlad | voidspace: I don't know which file in /etc/ wins on reboot. http://askubuntu.com/questions/594765/ubuntu-14-04-cant-get-past-4096-max-open-files-for-non-root-user has more info | 10:51 |
| voidspace | dooferlad: thanks | 10:51 |
| voidspace | appreciated | 10:51 |
| voidspace | ooh, agent lost on 11 machines - I think I just hit the limit | 10:53 |
| voidspace | past 20 containers though | 10:53 |
| voidspace | ah no, they're back - just the machine grinding to a halt I think | 10:53 |
| voidspace | I'm at 25 | 10:54 |
| perrito666 | Morning | 10:54 |
| voidspace | perrito666: morning! | 10:56 |
| voidspace | at 30 containers I ran out of disk space | 11:07 |
| rogpeppe1 | this PR introduces a new field in api.Info, SNIHostName. reviews appreciated, thanks. https://github.com/juju/juju/pull/6407 | 11:53 |
| === rogpeppe1 is now known as rogpeppe | ||
| wallyworld | axw: i updated the api server to allow model admins or owners to make that status call. rc3 servers will not break the destroy as it will just print a message and continue without blocking | 12:05 |
| perrito666 | K doctor appointment bbl | 12:33 |
| perrito666 | well gotta love when doctors use old houses as practices.... lte wont go through walls | 13:41 |
| rick_h_ | voidspace: ping for standup | 14:01 |
| voidspace | rick_h_: omw | 14:01 |
| rogpeppe | could someone review this please? i've got another one backed up behind it. https://github.com/juju/juju/pull/6407 | 14:27 |
| alexisb | perrito666, ping | 14:45 |
| rick_h_ | katco: can you peek at rogpeppe's link ^ please? | 14:46 |
| katco | rick_h_: sure | 14:46 |
| babbageclunk | voidspace, dooferlad, katco``: anyone feel like a review? https://github.com/juju/juju/pull/6408 | 14:46 |
| rick_h_ | ty! | 14:46 |
| babbageclunk | d'oh | 14:47 |
| katco | rogpeppe: is this fixing a bug? | 14:47 |
| alexisb | babbageclunk, that is why I was pinging perrito666 :) | 14:51 |
| natefinch | rick_h_: btw, I forgot that updating the featuretests for the logout stuff was actually very easy, so I just did it. landing now | 15:00 |
| rick_h_ | natefinch: <3 | 15:00 |
| rogpeppe | katco: no, it's an enhancement to make it easier to deal with controller with public certs | 15:02 |
| natefinch | ahh crap, realized I need to get the change to persistent-cookiejar to land before the juju core stuff will work, oops. | 15:17 |
| natefinch | rogpeppe: I made the suggested changes to persistent cookiejar, in a new PR from a better named branch: https://github.com/juju/persistent-cookiejar/pull/18/files | 15:23 |
| rogpeppe | natefinch: ok, looking | 15:23 |
| rogpeppe | natefinch: swap ya! https://github.com/juju/juju/pull/6407 | 15:23 |
| katco | rogpeppe: natefinch: i am also looking at this ^^ | 15:23 |
| rogpeppe | i need two reviews to land it :) | 15:24 |
| natefinch | rogpeppe: deal | 15:24 |
| rogpeppe | natefinch: i'm also working on getting people on board for general use of my new retry package API... (which I think is actually kinda cool :-]) https://github.com/juju/utils/pull/245 | 15:25 |
| natefinch | rogpeppe: we really need to move juju/utils/clock to a top level repo | 15:26 |
| rogpeppe | natefinch: LGTM | 15:27 |
| natefinch | rogpeppe: thanks | 15:27 |
| rogpeppe | natefinch: yeah, probably | 15:27 |
| rogpeppe | natefinch: although i don't think it's a big issue | 15:27 |
| natefinch | rogpeppe: utils churn has been an issue in the past... backporting stuff in utils often requires one-off ugly branches and godeps. | 15:28 |
| natefinch | rogpeppe: newWebsocketDialer0? | 15:31 |
| natefinch | rogpeppe: are dialwebsocket and dialapi unchanged? They moved, so it's hard to see the diff. | 15:36 |
| rogpeppe | natefinch: they're pretty similar if not entirely unchanged. sorry about the move, but they really belong together. | 15:36 |
| natefinch | oh, no, I see some diffs | 15:36 |
| natefinch | mostly unchanged though | 15:37 |
| rogpeppe | natefinch: i thought that "newWebsocketDialer0" vs newWebsocketDialer is better than the arbitrary newWebsocketDialer vs createWebsocketDialer | 15:38 |
| rogpeppe | natefinch: i often use "0" as a "this is different but essentially the same thing" suffix | 15:38 |
| katco | rogpeppe: review up | 15:42 |
| rogpeppe | katco: ta! | 15:42 |
| katco | i really dislike how comments break up the code... makes it really hard to read reviews | 15:43 |
| rogpeppe | katco: "We need to instead take this in as a dependency to the call-chain." - does that mean every caller to NewServer has to manually wire that dependency up? | 15:43 |
| katco | rogpeppe: that bubbles up to Open(...) doesn't it? | 15:44 |
| rogpeppe | katco: oh yes, sorry, it does... every call to Open then | 15:45 |
| katco | rogpeppe: it would fit nicely into DialOpts i think | 15:45 |
| katco | rogpeppe: yes, that is what it means | 15:45 |
| rogpeppe | katco: i really feel like it's very much an internal implementation detail | 15:45 |
| katco | rogpeppe: if it were, then the code you're writing against it wouldn't have to patch a global variable | 15:45 |
| rogpeppe | katco: it's an internal test | 15:46 |
| katco | rogpeppe: tests are just code | 15:46 |
| rogpeppe | katco: it's allowed to depend on internal implementation details | 15:46 |
| rogpeppe | katco: for me, not exposing it is about encapsulation of implementation detail | 15:46 |
| rogpeppe | katco: if we expose it, we can't change it any more | 15:47 |
| katco | rogpeppe: sure we can, you just pass in something different | 15:47 |
| rogpeppe | katco: clients become dependent on that particular detail of the implementation | 15:47 |
| katco | rogpeppe: if it were an implementation detail, the code you've written wouldn't have to patch out a global variable | 15:47 |
| rogpeppe | katco: ok, we can't change it without changing the clients | 15:47 |
| katco | rogpeppe: yes, but the code you've written shows that different clients have different needs | 15:48 |
| katco | rogpeppe: this is fundamental to IoC | 15:48 |
| rogpeppe | katco: the test in question is a unit test. i think it's legimate for unit tests to depend on internal details that we would not want to expose outside the package. | 15:48 |
| katco | rogpeppe: a test is just code, and in this case it's exposing a design flaw | 15:49 |
| rogpeppe | katco: i don't think all tests should be external tests. that way lies hideously complex APIs | 15:49 |
| katco | rogpeppe: i think that decision was already made | 15:50 |
| rogpeppe | katco: really? *strictly* external tests? | 15:52 |
| katco | rogpeppe: yeah | 15:52 |
| rogpeppe | katco: i mean, i know we define tests as if they were external, but i didn't realise we weren't allowed to test any internal implementation detail directly | 15:53 |
| katco | rogpeppe: i went through that pain and came out the other side agreeing with it a little more. i haven't run into a counter-example, but i don't doubt they're out there | 15:53 |
| rogpeppe | katco: if that's really the case then i give up. i think it's a path to ruin. | 15:53 |
| katco | rogpeppe: :( i don't know how to respond to that. i didn't like it either, but i pushed through | 15:54 |
| rogpeppe | katco: so you really think we should add a field like this to api.DialOpts? | 15:55 |
| rogpeppe | DialWebsocket func(cfg *websocket.Config, opts DialOpts) func(<-chan struct{}) (io.Closer, error) | 15:55 |
| katco | rogpeppe: i am less sure about how it gets passed in, but DialOpts does look like the right spot as a place to specify "options to dial"? | 15:55 |
| rogpeppe | katco: even though there are >70 instantiations of DialOpts in the code and they'll all need to change and websockets are just an implementation detail of the API that we might want to change in the future? | 15:56 |
| katco | rogpeppe: how about this? create a 2nd function that calls the existing one with the correct function passed in? | 15:56 |
| rogpeppe | katco: ? | 15:57 |
| rogpeppe | katco: tbh rather than adding that i think it would be preferable to do a more integration-style test with a real server (but that's hard with code that relies on officially signed certs) | 15:58 |
| katco | rogpeppe: i.e. rename Open => OpenWithWebsocketDialer(..., func()) and then modify Open to call OpenWithWebsocketDialer with the correct func? | 15:58 |
| rogpeppe | katco: why would you want to export that function? | 15:58 |
| katco | rogpeppe: integration tests can be done in the top level featuretests/ but not anywhere else | 15:58 |
| rogpeppe | katco: that still exposes an implementation detail that really should be hidden inside the api package | 15:59 |
| rogpeppe | katco: websockets are not essential to the api abstraction | 15:59 |
| katco | rogpeppe: the code you have written shows that it is not an implementation detail | 15:59 |
| rogpeppe | katco: you're saying that tests aren't allowed to test specific implementation details AFAICS | 15:59 |
| rogpeppe | katco: and i disagree. tests of specific implementation details are often the ones that give you most confidence in the higher level code. | 16:00 |
| katco | rogpeppe: they are certainly free to test the behavior, but if that behavior need be modified by patching out global variables, that's a design smell and proves that it's not an implementation detail | 16:00 |
| rogpeppe | katco: you're saying they're free to check externally visible behaviour but nothing else. | 16:01 |
| katco | rogpeppe: yes, the behavior of the thing you're testing, not how it's implemented | 16:01 |
| rogpeppe | katco: i think there's room for both approaches. we often write tests knowing about the structure of the code we're testing. | 16:02 |
| rogpeppe | katco: because otherwise you would never get decent test coverage and you'd write lots of redundant tests. | 16:02 |
| rogpeppe | katco: i think this is that kind of case. | 16:03 |
| rogpeppe | katco: patching out global variables is a smell, yes, but so is exposing abstraction-breaking details as part of your public API. | 16:04 |
| rogpeppe | katco: as usual, there's a trade-off to be made. | 16:04 |
| katco | rogpeppe: i've heard this argument a lot -- that it breaks encapsulation -- yes, by design it does. the code you've written has shown that it's a detail that should not be encapsulated | 16:04 |
| rogpeppe | katco: you're assuming that a package's tests are exactly the same as any other client of the code. | 16:05 |
| rogpeppe | katco: but as i've tried to say, they really aren't - they are necessarily privy to details of the code structure. | 16:06 |
| katco | rogpeppe: not the same, but it is one of the first consumers | 16:06 |
| rogpeppe | katco: that's very true, and it's great to write tests as much as possible to be strictly external. | 16:06 |
| rogpeppe | katco: but i don't think that it's necessary to have a hard and fast rule there. | 16:07 |
| rogpeppe | katco: in the end what really counts is the overally simplicity and maintainability of the code. | 16:07 |
| rogpeppe | s/overally/overall/ | 16:07 |
| katco | rogpeppe: definitely | 16:07 |
| rogpeppe | katco: and a big part of "simplicity" is simplicity of exported APIs. | 16:08 |
| katco | rogpeppe: but, we tried the other way. our tests are extremely brittle, so a decision was made and we must abide by it | 16:08 |
| katco | rogpeppe: no, that's "easy". simplicity is a different beast | 16:08 |
| rogpeppe | katco: no, simple APIs are not "easy" | 16:08 |
| rogpeppe | katco: FWIW making this an exposed external thing is likely to make code more brittle not less | 16:09 |
| katco | rogpeppe: there are valid arguments against ioc, but i don't think lack of simplicity is one of them. lack of ease is i think | 16:09 |
| rogpeppe | katco: ioc? | 16:09 |
| katco | rogpeppe: inversion of control | 16:10 |
| katco | rogpeppe: dependency injection | 16:10 |
| katco | rogpeppe: please don't take this as dismissive, but i could sit here and enjoy this conversation all day, but i need to get to some other reviews/code. that's my review, i think we have decisions to back it. we can appeal to someone (?) if that helps | 16:11 |
| rogpeppe | katco: for me, i have the most problem with exposing abstraction-breaking details in the API | 16:11 |
| rogpeppe | katco: thanks for the review | 16:11 |
| katco | rogpeppe: ta for the conversation | 16:11 |
| perrito666 | alexisb: sorry for the delay I was at the doctor being yelled at because I am fat and lazy | 16:12 |
| alexisb | perrito666, :) | 16:13 |
| alexisb | those darn doctors always concerned about your health | 16:13 |
| alexisb | perrito666, I was going to ask you to review babbageclunk PR | 16:13 |
| perrito666 | alexisb: if still ther eill be glad to | 16:14 |
| babbageclunk | perrito666: It's still there: https://github.com/juju/juju/pull/6408 | 16:27 |
| babbageclunk | alexisb: I haven't sorted out the http.Transport leaks yet - should I just make a binary now and push it to the OIL CI without a fix for that. | 16:28 |
| babbageclunk | ? | 16:28 |
| perrito666 | babbageclunk: reviewed, lgtm but I would love to have a second pair of eyes there since it is a critical path | 16:39 |
| babbageclunk | perrito666: Thanks - ok, I'll ping Menno about it, he knows that code. | 16:40 |
| === frankban is now known as frankban|afk | ||
| natefinch | sinzui: CI seems broken... it didn't post that my build had failed and it's not taking new builds, AFAICT. | 17:01 |
| sinzui | natefinch: ci? I think you mean the bot lander. | 17:21 |
| natefinch | sinzui: it's all CI to me | 17:22 |
| natefinch | sinzui: but yes | 17:22 |
| sinzui | the bot is a third part tool that uses git. Ci Launchpad, tarfiles and our machines. | 17:23 |
| sinzui | natefinch: I am looking for the job the bt started, sometimes it fails so quickly ti cannot call home. If so I will just requeue | 17:23 |
| natefinch | sinzui: it should have failed to compile, I honestly didn't look at the output since I realize it would fail before I even saw it fail | 17:24 |
| sinzui | oh, it doesn't compile | 17:24 |
| sinzui | natefinch: yes, the compilation error prevented the call back. http://juju-ci.vapour.ws/view/Juju%20Ecosystem/job/github-merge-juju/9452/console. When this happens Add "Build failed: Tests failed" in a comment. That will let you requeue | 17:26 |
| natefinch | oh neat, thanks | 17:26 |
| sinzui | natefinch: I think I fixed this scenaio. if Juju fails to build, It will call back. I will need to watch a few runs to be sure though | 17:29 |
| natefinch | sinzui: even better. Thanks again. always glad to help break things in new and interesting ways. | 17:29 |
| sinzui | :) | 17:30 |
| redir | smallish review anyone? https://github.com/juju/juju/pull/6399 PTAL... | 18:23 |
| rick_h_ | katco: ping, can I bug you please? | 18:25 |
| katco | rick_h_: sure thing | 18:32 |
| rick_h_ | katco: can you join https://hangouts.google.com/hangouts/_/canonical.com/rick?authuser=1 please | 18:34 |
| katco | rick_h_: this might help: https://github.com/juju/juju/blob/master/cloud/clouds.go#L24 | 18:58 |
| katco | rick_h_: we generate this file https://github.com/juju/juju/blob/master/cloud/fallback_public_cloud.go#L8 | 18:58 |
| katco | rick_h_: off the yaml file | 18:58 |
| katco | rick_h_: so if go generate was not run when we built rc3, the file would be stale; not sure if i got the use-case/problem completely straight, but that seemed like a detail that might be important | 18:59 |
| rick_h_ | katco: yea, that's 'duplication' in the looks of what's in the code, but it's generated so that's less actual dupe than it seems I guess | 19:00 |
| katco | rick_h_: right... the idea is to bring that information in at compile time rather than run-time by reading the yaml file | 19:01 |
| rick_h_ | katco: yea, just not sure if that's the peachiest thing imo. I understand folks think different there though so understand | 19:02 |
| katco | rick_h_: moving down the chain of execution is usually better when you can do it. removes chance of errors at runtime, cost of reading every time, etc. | 19:02 |
| natefinch | rick_h_: thoughts on what I should take next? | 19:12 |
| rick_h_ | natefinch: pick your favorite critical please | 19:13 |
| rick_h_ | natefinch: the streams one might be interesting but up to you | 19:13 |
| natefinch | rick_h_: which one is the streams one? | 19:14 |
| rick_h_ | natefinch: generate-tools, second down on the right | 19:14 |
| natefinch | rick_h_: I have no idea how the generate-tools stuff works, but I'm happy to look into it | 19:15 |
| * redir lunches | 19:59 | |
| === beisner- is now known as beisner | ||
| === xnox_ is now known as xnox | ||
| === kragniz1 is now known as kragniz | ||
| redir | must be friday afternoon:) | 22:32 |
| alexisb | redir, heh | 23:36 |
| redir | :) | 23:36 |
| perrito666 | god people, on irc so late on a log weekend friday? you have no life :p | 23:40 |
| alexisb | :) | 23:41 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!