| === alexlist` is now known as alexlist | ||
| davecheney | thumper: http://paste.ubuntu.com/12069959/ | 02:45 |
|---|---|---|
| davecheney | this is from this morning's run | 02:45 |
| davecheney | summary: juju is now as stable on ppc64 as any other platform | 02:45 |
| thumper | hazaah | 02:50 |
| menn0 | thumper: here's the final allenvwatcher pr: http://reviews.vapour.ws/r/2374/ | 03:35 |
| menn0 | thumper: any chance you can look at the above? ^^ | 04:51 |
| * menn0 is keen to be done with this | 04:51 | |
| * thumper is looking now | 04:53 | |
| menn0 | thumper: thanks | 04:53 |
| thumper | menn0: done | 05:25 |
| thumper | one (strong) suggestion | 05:25 |
| menn0 | thumper: ok... | 05:25 |
| menn0 | thumper: no problems. I had something like that at one point. | 05:27 |
| menn0 | thumper: to be fair those funcs aren't really for truly public use (hence the comment) | 05:29 |
| thumper | well... if they have capital letters, they're public | 05:40 |
| dimitern | TheMue, morning | 06:55 |
| dimitern | TheMue, I see you've assigned the constraints card to yourself, but you're yet to land your other branch, so I've reassigned it to myself | 06:56 |
| TheMue | dimitern: morning, the landing card is just waiting, that CI is unblocked | 07:10 |
| dimitern | TheMue, cool | 07:10 |
| TheMue | dimitern: so which card can I start with now? | 07:11 |
| dimitern | TheMue, hmm.. let me have a look | 07:12 |
| dimitern | TheMue, you have a follow-up to do, don't you? :) | 07:13 |
| dimitern | also it seems I've assigned the wrong card to myself - I'm actually working on the other constraints card | 07:13 |
| TheMue | dimitern: hehe | 07:13 |
| TheMue | dimitern: you mean "Test isNetworkingEnvironment() in ..."? we haven't moved it from backlog to iteration backlog, so I thought it's not as important as the spaces cards | 07:15 |
| dimitern | TheMue, yeah, we forgot about this one | 07:16 |
| dimitern | TheMue, the net-cli stuff is the most important once the previous tasks are done :) | 07:16 |
| TheMue | dimitern: ok, so I'll move it | 07:16 |
| dimitern | TheMue, cheers, and feel free to estimate it as well | 07:16 |
| TheMue | let's move it, move it, let's move it, move it *sing* | 07:17 |
| TheMue | dimitern: ok | 07:17 |
| voidspace | dimitern: ping | 07:22 |
| dimitern | voidspace, pong | 07:24 |
| voidspace | dimitern: did you write ListSubnets? | 07:24 |
| voidspace | dimitern: unlike all the other API methods it tages a *names.SpaceTag rather than a names.SpaceTag and I'm wondering why | 07:25 |
| voidspace | I changed it, as it was marginally inconvenient in one place, and a few things broke | 07:25 |
| voidspace | so I probably have to put it back | 07:25 |
| dimitern | voidspace, I did - it's because the space is optional for filtering | 07:26 |
| dimitern | voidspace, it was supposed to make calling the client-side method more convenient :) | 07:26 |
| voidspace | dimitern: and you need to be able to distinguish between not used (nil) and an explicit empty space ("") | 07:26 |
| voidspace | dimitern: so, changing API methods to take tags instead of strings means changing a bunch of CLI calls too | 07:28 |
| voidspace | dimitern: they need to use the tags and do the validation | 07:28 |
| voidspace | so I'm now down to only 23 test failures... | 07:29 |
| voidspace | (the CLI methods were already validating of course - but not always storing a tag. And it's *mostly* tests that need to change.) | 07:29 |
| dimitern | voidspace, :) | 07:32 |
| dimitern | voidspace, sounds good | 07:33 |
| dimitern | TheMue, voidspace, dooferlad, guys, I'm suggesting to cancel the 1:1 meetings today - happy to chat about anything, if you want of course | 07:44 |
| dimitern | but in the face of the feature freeze I'd rather concentrate on that with priority | 07:44 |
| TheMue | dimitern: frome my side nothing special, so sounds good | 07:45 |
| dimitern | TheMue, cheers | 07:47 |
| voidspace | dimitern: sure | 07:47 |
| voidspace | dimitern: thanks | 07:48 |
| dooferlad | dimitern: +1 | 07:55 |
| dimitern | thanks guys! | 07:55 |
| voidspace | dimitern: TheMue: dooferlad: ready in a minute or two - coffee!!! | 09:01 |
| voidspace | dimitern: TheMue: dooferlad: not sure if my last message got through, my internet went down | 09:32 |
| voidspace | sorry | 09:32 |
| voidspace | I'm back now but the hangout was empty | 09:32 |
| TheMue | voidspace: you've been frozen and then the connection dropped | 09:33 |
| TheMue | voidspace: so the question is, what has been the last message? ;) | 09:33 |
| voidspace | that was my last message... | 09:33 |
| voidspace | "my internet went down" | 09:33 |
| voidspace | :-) | 09:33 |
| dimitern | voidspace, oh - I think I heard only "own" :) | 09:34 |
| TheMue | hehe | 09:36 |
| voidspace | dimitern: own? | 09:36 |
| voidspace | dimitern: out of "down"? | 09:40 |
| dimitern | voidspace, I think so yeah | 09:42 |
| dimitern | (well it sounded like "on") | 09:42 |
| voidspace | heh | 09:42 |
| voidspace | sorry | 09:42 |
| voidspace | :w | 09:42 |
| voidspace | wrong window... | 09:42 |
| dimitern | voidspace, so it seemed we should chat about subnet add/create vs tagging? | 09:43 |
| voidspace | dimitern: well, I thought we had talked about it a few times already! | 09:43 |
| voidspace | dimitern: that's why I was so surprised | 09:43 |
| voidspace | dimitern: previously you seemed to agree that Create was easier and we didn't need to touch tagging for the MVP | 09:44 |
| dimitern | voidspace, yeah, I might have misinformed you unintentionally | 09:44 |
| voidspace | dimitern: if we *have* to do Add, then so-be-it, but I think it's *more work* | 09:44 |
| dimitern | voidspace, we'll implement create, but not include in the MVP | 09:44 |
| voidspace | sure, I'm talking about what is the least work we can do which gets something useful | 09:45 |
| dimitern | voidspace, also add doesn't need to respect pre-existing tags on a subnet in ec2 | 09:45 |
| dimitern | for the MVP | 09:45 |
| voidspace | so what does it do? | 09:45 |
| dimitern | voidspace, it adds it to state, validating it exists on the provider | 09:45 |
| voidspace | just assumes the subnet exists and adds it to state | 09:45 |
| voidspace | so it does go out to the provider | 09:46 |
| voidspace | (the current implementation doesn't...) | 09:46 |
| dimitern | voidspace, yes, as we have already a Subnets call we can call - it shouldn't be too bad to add this check | 09:46 |
| voidspace | ok | 09:46 |
| voidspace | so if we ignore tagging then Add is easier :-) | 09:46 |
| dimitern | voidspace, indeed - we'll add tagging (discovery and updates) later if we manage - it's strictly an improvement in UX and observability, not so much in functionality | 09:48 |
| voidspace | dimitern: fair enough I guess | 09:52 |
| voidspace | dimitern: like I said, based on our previous conversations I was just surprised | 09:52 |
| voidspace | dimitern: updated PR http://reviews.vapour.ws/r/2360/ | 10:12 |
| dimitern | voidspace, cheers, looking | 10:25 |
| dimitern | dooferlad, others: http://reviews.vapour.ws/r/2376/ spaces constraints | 10:31 |
| dooferlad | dimitern: already reading :-) | 10:31 |
| dimitern | dooferlad, ta! | 10:34 |
| dimitern | voidspace, didn't we talk about ListSubnets returning something like []params.SubnetInfo? | 10:35 |
| dimitern | voidspace, I mean the client-side api method | 10:36 |
| voidspace | dimitern: I don't recall a conversation about that, let me look and see what it does return | 10:37 |
| voidspace | it's been a while since I wrote that code | 10:37 |
| voidspace | dimitern: hmm... it returns a list of params.Subnet | 10:38 |
| voidspace | dimitern: there is no params.SubnetInfo is there? | 10:39 |
| dimitern | voidspace, sorry, I meant params.Subnet | 10:39 |
| voidspace | dimitern: well, it has a result type that holds a list of params.Subnet | 10:39 |
| dimitern | voidspace, well, looking at your branch it returns ListSubnetsResults and an error | 10:39 |
| voidspace | dimitern: do you mean removing the ListSubnetsResults part from the returned value | 10:39 |
| dimitern | voidspace, that's pretty unusual for client-side apis | 10:40 |
| dimitern | voidspace, just return the Results part of it and the error | 10:40 |
| voidspace | dimitern: ah | 10:40 |
| voidspace | dimitern: ListSpaces does the same | 10:40 |
| voidspace | dimitern: ah, and the CLI expects just a list of subnets | 10:41 |
| voidspace | dimitern: so that needs fixing | 10:41 |
| voidspace | probably the same for ListSpaces too | 10:41 |
| voidspace | dimitern: thanks | 10:41 |
| voidspace | dimitern: just return response.Results or can we get rid of the response type completely and just return a slice of Subnet ? | 10:43 |
| voidspace | (I mean can the apiserver return []params.Subnet or do we need a Result type to hold it) | 10:44 |
| voidspace | I'm just changing the api List* methods | 10:46 |
| dimitern | voidspace, nope, the apiserver-side still needs to use the result type like this | 10:46 |
| voidspace | yeah | 10:46 |
| dimitern | voidspace, you have a review | 10:46 |
| voidspace | dimitern: thanks | 10:46 |
| voidspace | dimitern: why do we need to verify that the command members are set correctly on error? | 10:47 |
| voidspace | dimitern: given that we report the error to the user and exist | 10:47 |
| dimitern | voidspace, I'd appreciate a second look on my review as well - esp. if any of the TODOs reminds you of something I might've missed | 10:48 |
| voidspace | dimitern: ok | 10:48 |
| dimitern | voidspace, it's not a big deal to leave this in place | 10:49 |
| dimitern | voidspace, is it? | 10:49 |
| voidspace | dimitern: well, as we're now using SubnetTag I can't construct a SubnetTag for an invalid CIDR | 10:51 |
| voidspace | dimitern: so in the case of error we can't check that | 10:51 |
| voidspace | dimitern: and if there's no *need* for the others then it's better not to test them | 10:52 |
| dimitern | voidspace, ah | 10:52 |
| voidspace | we should only test behaviour we actually need | 10:52 |
| dimitern | voidspace, sure, then it's fine | 10:52 |
| voidspace | thanks | 10:52 |
| dimitern | voidspace, unlike APIs, those CLI methods are not usable from outside the package | 10:52 |
| voidspace | yep | 10:53 |
| voidspace | that was my thinking | 10:53 |
| voidspace | dimitern: what's the value in changing providerId to network.Id? | 10:53 |
| voidspace | dimitern: there's no validation, so it doesn't really offer any benefit like SubnetTag and SpaceTag do | 10:53 |
| voidspace | it can take arbitrary strings | 10:54 |
| voidspace | I believe | 10:54 |
| voidspace | and it probably means finding and fixing another crap-load of tests :-p | 10:54 |
| dimitern | voidspace, fwereade reminded me on more than one occasion that we should use typed strings for foreign ids, like the provider ones | 10:54 |
| voidspace | ok | 10:54 |
| voidspace | fair enough | 10:54 |
| voidspace | I'll bite the bullet | 10:54 |
| dimitern | voidspace, yeah, no validation for now, having a type makes adding this easier | 10:55 |
| dimitern | if/when needed | 10:55 |
| dimitern | voidspace, thanks | 10:55 |
| voidspace | so for our imaginery future use case then | 10:55 |
| voidspace | ;-) | 10:55 |
| dimitern | well :) | 10:56 |
| voidspace | dimitern: network.Id is used for too many *different* things for us to ever be able to add meaningful validation | 10:56 |
| voidspace | we'd have to create a *specific* type | 10:56 |
| dimitern | good design and architecture includes some need for precognition and such | 10:56 |
| dimitern | voidspace, it's used for provider-specific ids | 10:57 |
| voidspace | dimitern: many *different* provider-specific ids with different specifications | 10:57 |
| dimitern | voidspace, at least having them marked as such will simplify changing them to separate types if needed IMO | 10:58 |
| dimitern | voidspace, I hope you don't mind too much for a string cast here and there :) | 10:59 |
| voidspace | dimitern: I'm not totally convinced about the *semantic* value of using types (that offer no tangible other benefit) to represent "things" that actually *are* strings, and having to case to and from everywhere they're used | 11:01 |
| voidspace | dimitern: I'll do it, I'm just not convinced it adds value | 11:01 |
| dimitern | voidspace, I'll wait for your and mine branches to land and then create the net-cli-mvp branch | 11:01 |
| dimitern | voidspace, right | 11:02 |
| voidspace | dimitern: cool | 11:02 |
| voidspace | dimitern: I'll take lunch shortly | 11:03 |
| voidspace | I'll try and get the branch landable first | 11:03 |
| dimitern | voidspace, ta! | 11:04 |
| voidspace | dimitern: right, made those changes | 11:25 |
| voidspace | dimitern: switching to network.Id touched 6 files, so you might want to take another brief look | 11:25 |
| voidspace | :-D | 11:25 |
| voidspace | maybe "want" is the wrong word | 11:25 |
| dimitern | voidspace, sure, looking | 11:26 |
| dooferlad | dimitern: https://github.com/juju/juju/compare/net-cli...dooferlad:net-cli-add-deployment-support-for-spaces?expand=1 seems to be all https://canonical.leankit.com/Boards/View/101652562/116603811 needs. | 11:27 |
| dooferlad | just let me know about printing/logging the deprecation warning and I will tidy up that comment and get it in for review | 11:27 |
| dimitern | dooferlad, ok, I'll have a look shortly | 11:27 |
| dooferlad | dimitern: thanks. | 11:27 |
| dimitern | voidspace, get some lunch first | 11:28 |
| dimitern | voidspace, you're eating tests otherwise :) | 11:28 |
| voidspace | :-) | 11:29 |
| voidspace | dimitern: well, I want to meditate and it will be a little bit of time before I can persuade my wife to clear out of the room I need :-D | 11:31 |
| dimitern | voidspace, I see ok :) | 11:31 |
| dimitern | voidspace, dooferlad, only now I realize ListSpaces API doesn't conform to the usual (results, error) behavior - it returns results and an error, but only the first error when fetching a space's subnets | 11:34 |
| voidspace | dimitern: you mean it should collect all errors | 11:35 |
| dimitern | it needs to use a Results and Error fields of the results for such errors | 11:35 |
| voidspace | dimitern: as the only possible error is connection with state failing (so all should fail or none) | 11:35 |
| voidspace | dimitern: I don't think it's a big issue | 11:35 |
| dimitern | voidspace, yeah, only return an error if it can't return anything | 11:35 |
| voidspace | I mean, we should fix it - but add a card for after mvp maybe | 11:36 |
| dimitern | voidspace, well, just because we're catching this now means it lacks bulk call coverage | 11:36 |
| voidspace | "bulk call coverage"? | 11:36 |
| voidspace | it's a bulk response not a bulk call... | 11:36 |
| voidspace | it's a single call with a bulk result | 11:37 |
| dimitern | voidspace, yeah - a call to it with e.g. one valid, one invalid, and another valid - checking 2 results and 1 error is returned | 11:37 |
| dimitern | voidspace, the apiserver method I mean | 11:37 |
| voidspace | right | 11:37 |
| dimitern | voidspace, it's trivial to fix and we should do it for the mvp by all means, as changing the api later is worse | 11:38 |
| voidspace | dimitern: ok | 11:39 |
| dimitern | voidspace, I'd add a card for it, but please add a TODO about it in your PR - apiserver/spaces | 11:39 |
| voidspace | dimitern: TODO added | 11:40 |
| voidspace | well, pushing now | 11:41 |
| dimitern | voidspace, thanks! | 11:42 |
| voidspace | done | 11:42 |
| voidspace | right, room free | 11:44 |
| voidspace | off to meditate | 11:44 |
| dimitern | dooferlad, hey | 11:46 |
| dimitern | dooferlad, the help docs need to mention the spaces constraints but not --networks btw | 11:47 |
| dimitern | dooferlad, but there are also - tests we do ignore --networks and dropping those that don't (or at least adding TODOs/skipping them) | 11:49 |
| dimitern | dooferlad, hmm sorry, I might have described it badly in the card | 11:50 |
| dimitern | dooferlad, the idea is not only to give a warning for it, but also not to pass it through the layers as well, so it can't interfere with the other work around spaces for deployments | 11:51 |
| dimitern | dooferlad, does that make sense? | 11:51 |
| === ahasenac` is now known as ahasenack | ||
| dimitern | does anyone else get build errors for gwacl on master after updating and running godeps ? | 12:11 |
| dimitern | axw, ping | 12:12 |
| dooferlad | dimitern: yea, that isn't what I got from the card. I read it as we would still accept --networks, but we should mark it as deprecated to discourage its use before removing it in a later version. | 12:40 |
| dimitern | dooferlad, we would've done that if currently it was at least remotely useful | 12:42 |
| dimitern | dooferlad, and it will soon go away to be replaced by deploy time bindings of the right kind, with spaces (once we figure out how'd they look like) | 12:43 |
| dimitern | anyone up for a trivial review? http://reviews.vapour.ws/r/2377/ | 13:02 |
| dimitern | TheMue, voidspace, dooferlad ^^ | 13:05 |
| dooferlad | dimitern: *click* | 13:07 |
| === lp|conference is now known as lazyPower | ||
| dooferlad | dimitern: you said that space contraints are handled elsewhere in the card I am looking at. Where is that? | 13:24 |
| dooferlad | dimitern: also, with network constraints, do you want me to just remove them or just not wire them up in the CLI? | 13:25 |
| dooferlad | or just leave as-is I guess. | 13:25 |
| dimitern | dooferlad, they need to go, but that's a separate card, which will require a lot of prerequisites (mostly provider and provisioner related) landed first | 13:26 |
| dimitern | dooferlad, not sure what do you mean by "handled" - my PR implements parsing and tests for them, and since they're constraints that makes them usable everywhere | 13:27 |
| dooferlad | dimitern: doh! Haven't merged that in :-) | 13:27 |
| dimitern | whether we do anything with them is another thing | 13:28 |
| dimitern | dooferlad, did that answer your questions btw? :) | 13:34 |
| dooferlad | dimitern: I am leaving the constraints code as it is, so it will accept networks. I have removed the code path that --networks used for juju deploy. | 13:35 |
| dooferlad | dimitern: I have had a look at add-machine and add-unit and both seem to have no explicit --network support, so they are just constraints code again | 13:35 |
| dooferlad | dimitern: if that is right, yes. | 13:35 |
| dimitern | dooferlad, indeed - I just double checked | 13:37 |
| dimitern | dooferlad, are there a lot of tests affected by --networks being ignored ? | 13:37 |
| dooferlad | dimitern: I just removed the two that started to fail (correctly) once support for the --network flag was removed. | 13:38 |
| dooferlad | dimitern: I didn't look at any already skipped ones. | 13:38 |
| dimitern | dooferlad, by "removed" do you mean it returns an error like "flag provided but not defined: --networks" or it accepts it, prints a warning and continues? | 13:40 |
| dooferlad | prints a warning and continues as if it isn't there | 13:40 |
| dimitern | dooferlad, great! | 13:40 |
| dooferlad | dimitern: http://reviews.vapour.ws/r/2382/ | 13:40 |
| dooferlad | dimitern: I haven't added any support, so much as removed other support. | 13:41 |
| dimitern | dooferlad, that's a lot better - less fallout than my original attempt which did the former | 13:41 |
| dooferlad | so the title is rubbish | 13:41 |
| dimitern | dooferlad, it's fine :) | 13:49 |
| dimitern | dooferlad, almost done with the review btw | 13:49 |
| dimitern | dooferlad, was the removal of that status test case necessary? | 14:05 |
| dooferlad | dimitern: yes, it failed. | 14:09 |
| dimitern | dooferlad, that's because we're not stopping tests from calling deploy APIs with networks | 14:14 |
| dimitern | dooferlad, rather than removing it, can you comment it out for now with a TODO about fixing it once the API and state layers also ignore (requested)networks (which --networks sets effectively) ? | 14:15 |
| dimitern | dooferlad, I'll add a comment to the review | 14:15 |
| dimitern | dooferlad, reviewed | 14:21 |
| voidspace | dimitern: ping | 14:43 |
| dimitern | voidspace, pong | 14:47 |
| voidspace | dimitern: so, I add a *Error field to params.Space so we can return partial results | 14:47 |
| voidspace | dimitern: the top level params.ListSpaceResult has an Error field (in case fetching the spaces fails - the new error is for an error fetching subnets for an individual space) | 14:48 |
| voidspace | dimitern: and ListSpaces also returns an error | 14:48 |
| voidspace | dimitern: so if we get an error fetching the subnets for a single space obviously we set the Error field on Space | 14:49 |
| dimitern | voidspace, you did all this in your subnets api branch? | 14:49 |
| voidspace | dimitern: do we set an error on ListSpaceResult and do we return an error too? | 14:49 |
| dimitern | voidspace, or in a follow-up? | 14:49 |
| voidspace | dimitern: no, new branch - looking at it now | 14:49 |
| dimitern | voidspace, ah, great | 14:49 |
| voidspace | dimitern: not sure if I can get it done before I go as I go to the gym in 40minutes | 14:49 |
| voidspace | dimitern: that branch landed on net-cli by the way | 14:49 |
| dimitern | voidspace, did the other one land? | 14:49 |
| voidspace | yep | 14:50 |
| dimitern | voidspace, awesome! | 14:50 |
| dimitern | voidspace, so I added a card for the ListSpace/Subnet errors | 14:50 |
| voidspace | I tried looking for other examples in the apiserver but failed to find any and thought it would be quicker to ask | 14:50 |
| voidspace | dimitern: it should just be ListSpace I believe | 14:50 |
| voidspace | dimitern: ListSubnets doesn't make extra calls per subnet, so either it all works or it all fails | 14:50 |
| dimitern | voidspace, so usually bulk calls that return many results for many entities | 14:50 |
| voidspace | (I believe) | 14:50 |
| voidspace | dimitern: with ListSpaces we can fetch the spaces and then fail to fetch the subnets for some of the spaces | 14:51 |
| voidspace | so partial results make sense in theory | 14:51 |
| dimitern | voidspace, ..only return errors if it's not possible to do its job | 14:51 |
| voidspace | I say in theory, because in practise the only error is that the connection to state goes | 14:51 |
| voidspace | so again, in reality it will be all or nothing | 14:51 |
| voidspace | but it's better to stick to the pattern | 14:51 |
| voidspace | dimitern: right, so we don't return a top level error | 14:52 |
| voidspace | dimitern: what about setting an error on ListSpacesResult? | 14:52 |
| dimitern | voidspace, let me have a look at the code now for a moment | 14:52 |
| voidspace | dimitern: or should we rely on the client checking the Error field of each Space | 14:52 |
| dimitern | voidspace, for ListSpaces you're correct - the backing error should be returned as top-level, others can only be per-space (fetching subnets failed) | 14:55 |
| cmars | wallyworld, http://reviews.vapour.ws/r/2387/ please | 14:56 |
| voidspace | dimitern: I'm sorry I don't understand | 14:57 |
| voidspace | dimitern: what is "the backing error"? | 14:57 |
| voidspace | dimitern: if we get an error fetching the subnets for a space obviously we set the Error on the params.Space | 14:57 |
| dimitern | voidspace, coming from the state (backing) | 14:57 |
| voidspace | dimitern: do we set an error anywhere else? | 14:57 |
| voidspace | dimitern: so we also set an error on ListSpaceResults? or are you saying we also return the error at the very top level? | 14:58 |
| dimitern | voidspace, sorry, let me explain better | 14:59 |
| dimitern | voidspace, https://github.com/juju/juju/pull/2980/files#diff-ec0483c931b7d3d2d378df66d465deb1R100 < this is a top-level error (with empty results) | 14:59 |
| voidspace | dimitern: yes | 15:00 |
| dimitern | voidspace, https://github.com/juju/juju/pull/2980/files#diff-ec0483c931b7d3d2d378df66d465deb1L109 < this is a per result error | 15:00 |
| voidspace | yes | 15:00 |
| voidspace | dimitern: that's what we do currently | 15:00 |
| dimitern | voidspace, in the latter case, the results != empty, err = nil | 15:00 |
| voidspace | right | 15:00 |
| voidspace | and we set the error on the individual params.Space we create | 15:01 |
| voidspace | (we should I mean) | 15:01 |
| voidspace | I'm asking do we *also* do anything else with it? | 15:01 |
| dimitern | voidspace, there's no individual params.Space | 15:01 |
| dimitern | voidspace, ListSpaces takes no args at apiserver side | 15:01 |
| voidspace | the object there "result" is a params.Space | 15:02 |
| voidspace | so instead of "return results, err" it would be | 15:02 |
| voidspace | result.Error = common.ServerError(err); result.Results = append(result.Results, result);continure | 15:03 |
| voidspace | *continue | 15:03 |
| dimitern | voidspace, ah, got you | 15:03 |
| voidspace | dimitern: that's the change you have said we ought to make | 15:03 |
| dimitern | voidspace, yes | 15:03 |
| voidspace | I'm asking if we should *also* set results.Error | 15:03 |
| dimitern | voidspace, and I've just realized ListSubnets apiserver side is not yet done, so it's fine | 15:03 |
| voidspace | and we've agreed that the returned err should still be nil | 15:03 |
| voidspace | ListSubnets won't have the same problem | 15:04 |
| voidspace | I'm asking if we should *also* set results.Error | 15:04 |
| voidspace | and we've agreed that the returned err should still be nil | 15:04 |
| dimitern | voidspace, yes, if you can fetch spaces ok, but not all subnets -> return results(with Error set as needed), nil | 15:04 |
| dimitern | voidspace, if you can't fetch spaces - as it is now | 15:05 |
| voidspace | so I should set result.Error for each Space where fetching subnets fail, and set a results.Error to indicate that there are only partial results | 15:05 |
| dimitern | voidspace, sorry, I'm confused - what do you mean by result.Error for each space? | 15:06 |
| voidspace | dimitern: every space we fetch is represented by a params.Space | 15:06 |
| dimitern | voidspace, yes, yes | 15:06 |
| voidspace | dimitern: that will grow an Error field | 15:06 |
| dimitern | voidspace, sorry | 15:06 |
| voidspace | dimitern: that is settled | 15:06 |
| dimitern | voidspace, I need to get in a call | 15:06 |
| dimitern | voidspace, it is, yes | 15:06 |
| voidspace | dimitern: I am asking, do we *also* set an Error on ListSpaceResults | 15:06 |
| dimitern | voidspace, where is that ListSpaceResults defined? | 15:07 |
| voidspace | sorry params.ListSpacesResults | 15:08 |
| voidspace | at the moment it doesn't have an Error field because the method would either return all results or an error | 15:08 |
| voidspace | so it didn't need one there were no partial results | 15:08 |
| voidspace | we're now adding partial results | 15:08 |
| voidspace | so I wonder if ListSpacesResults (apiserver/params/network.go I believe) | 15:09 |
| voidspace | I wonder if it now also needs an Error field | 15:09 |
| voidspace | used to indicate partial results | 15:09 |
| dimitern | voidspace, I believe we need to make api.ListSpaces() ([]params.Space, error) | 15:09 |
| voidspace | dimitern: earlier today you said that apiserver couldn't return []params.Space and ListSpacesResults was still needed :-) | 15:10 |
| dimitern | voidspace, then also change ListSpacesResults' Results field to be of type ListSpacesResult (or whatever), which has a Space and Error *Error fields | 15:11 |
| voidspace | instead of adding an Error to Space... | 15:12 |
| voidspace | add a new intermediate type | 15:12 |
| dimitern | voidspace, it's just confusing because of the api/apiserver transition | 15:12 |
| voidspace | I think you're still slightly misundestanding my question/suggestion :-( | 15:12 |
| voidspace | I think you have too much in your head | 15:12 |
| voidspace | we don't need a new intermediate result type, we can just add an Error field to Space | 15:12 |
| dimitern | voidspace, ok, I see how params.Space can be used as what I'm describing as ListSpacesResult (singular) | 15:13 |
| voidspace | I am asking if the *plural*, the container results type, ListSpacesResults also needs an Error field | 15:13 |
| voidspace | because if we get partial results (an error occurred somewhere) don't we want a top level Error field to indicate that the results are partial? | 15:13 |
| voidspace | maybe not, and just having the error printed inline with the space description is fine | 15:14 |
| dimitern | voidspace, it's not common to have ErrPartialResults from API server methods | 15:15 |
| dimitern | voidspace, and in this case I don't think it's even useful | 15:15 |
| voidspace | ok | 15:15 |
| voidspace | so we won't | 15:15 |
| voidspace | thanks | 15:15 |
| dimitern | voidspace, thanks for bearing with me :) | 15:15 |
| voidspace | :-) | 15:15 |
| voidspace | I go soon | 15:16 |
| voidspace | so I'll be leaving you to it | 15:16 |
| natefinch | ericsnow: if we always flush immediately after set, we should be able to remove the mergeProcMaps | 15:17 |
| dimitern | voidspace, sure | 15:17 |
| ericsnow | natefinch: right | 15:18 |
| voidspace | dimitern: cmd/juju/spaces/list.go - ListSpaces command fetches the subnets using api.AllSubnets() | 15:21 |
| voidspace | dimitern: it doesn't use the subnets on the Space!! | 15:21 |
| voidspace | I guess space.Subnets() is newer than the ListSpaces command... | 15:21 |
| voidspace | I don't think I can fix that in the time I have | 15:22 |
| voidspace | it will make the code simpler though | 15:22 |
| voidspace | I'll add it to the card | 15:22 |
| dimitern | voidspace, ack, otp - thanks | 15:23 |
| voidspace | dimitern: done, card updated - including the code to set the error correctly | 15:26 |
| voidspace | dimitern: have a good week | 15:26 |
| voidspace | TheMue: dooferlad: EOW | 15:26 |
| voidspace | I begin my journey home | 15:26 |
| voidspace | good luck with the MVP | 15:26 |
| TheMue | voidspace: have a safe trip | 15:26 |
| voidspace | thanks | 15:26 |
| natefinch | ericsnow: updated the code according to your suggestions, would like a re-review so we can get it done | 15:28 |
| TheMue | dimitern: maybe I'm missing something and you can point me to the right way. I've got my test, I've got my mock, but the way to inject is seems difficult | 15:28 |
| ericsnow | natefinch: will do | 15:28 |
| natefinch | ericsnow: realized I didn't publish my responses to your earlier comments. Just did so. | 15:47 |
| ericsnow | natefinch: thanks | 15:47 |
| natefinch | (only one real comment) | 15:47 |
| dimitern | TheMue, can I look at the code you're having issues with? | 16:01 |
| dooferlad | dimitern: do you have an opinion about what card I should do next? | 16:03 |
| dimitern | dooferlad, I don't see a card for apiserver subnets - create and lest | 16:05 |
| dimitern | list | 16:05 |
| TheMue | dimitern: it's not a concrete issue, more a missing idea. | 16:05 |
| dooferlad | dimitern: you meen the apiserver side of what voidspace has done? | 16:06 |
| TheMue | dimitern: in the test in http://paste.ubuntu.com/12109009/ I try to inject the mock | 16:06 |
| dimitern | dooferlad, yeah | 16:06 |
| dimitern | dooferlad, we seem to have missed this - can you add a card and start on Add first perhaps? | 16:07 |
| dimitern | TheMue, I'll have a look shortly | 16:07 |
| dooferlad | dimitern: isn't it in the archive? https://canonical.leankit.com/Boards/View/101652562/115620070 | 16:07 |
| dooferlad | dimitern: also https://canonical.leankit.com/Boards/View/101652562/115620186 | 16:07 |
| dimitern | dooferlad, both of these are for spaces | 16:08 |
| dooferlad | dimitern: doh! | 16:08 |
| dooferlad | dimitern: yes, you are right | 16:08 |
| dimitern | dooferlad, I know it's confusing - it gets me every time :) | 16:09 |
| dimitern | dooferlad, and I'm wrong - apiserver has AddSubnets, but no list yet | 16:12 |
| dimitern | dooferlad, we don't need CreateSubnets for the MVP, so we can just do Add and List | 16:12 |
| dimitern | dooferlad, there's a card I added re ListSpacesResults and ListSubnetsResults - if you prefer | 16:13 |
| dimitern | dooferlad, also voidspace has a branch with some progress on it | 16:13 |
| dooferlad | dimitern: https://github.com/voidspace/juju/commits/spaces-subnets-api ? | 16:15 |
| dooferlad | dimitern: that is merged | 16:16 |
| dimitern | dooferlad, I think this is it - https://github.com/voidspace/juju/tree/spaces-list-errors | 16:17 |
| dooferlad | dimitern: that is about spaces, not subnets | 16:18 |
| dooferlad | but if both need list, then I can probably finish that and add subnets | 16:18 |
| dimitern | TheMue, it's about ListSpaces returning proper results, which will apply to ListSubnets as well even more (apiserver side) | 16:18 |
| dimitern | dooferlad, ^^ | 16:19 |
| dimitern | TheMue, sorry, I meant to tell you to have a look at TestManageEnvironDoesNotRunFirewallerWhenModeIsNone | 16:19 |
| TheMue | dimitern: ah, thx. what a java-like name | 16:19 |
| dimitern | TheMue, :) | 16:20 |
| dimitern | TheMue, the next one after it is another example | 16:20 |
| dimitern | TheMue, since this is a pre-existing test suite, no need to go wild with mocks and things when they're not already in place | 16:20 |
| dimitern | dooferlad, does that make sense? | 16:21 |
| dooferlad | dimitern: yes | 16:21 |
| dimitern | dooferlad, cheers, let's discuss it tomorrow if needed | 16:22 |
| dooferlad | dimitern: it is EOD for me, so will make some notes in that card about what is needed and start tomorrow, at which point I will probably have questions :-) | 16:22 |
| TheMue | dimitern: my test so far is pretty similar, but environs.SupportsNetworking() tests against the environment. that's why I want to mock it here | 16:23 |
| dimitern | dooferlad, awesome! | 16:23 |
| dimitern | TheMue, you don't need to mock it with the dummy provider | 16:23 |
| TheMue | dimitern: when using dummy environs.SupportsNetworking() always returns true | 16:23 |
| dimitern | TheMue, ah, yeah - it wasn't calling SupportsAddressAllocation | 16:27 |
| dimitern | TheMue, then, the mocks are unfortunately needed | 16:27 |
| dimitern | TheMue, *or* | 16:27 |
| dimitern | TheMue, I guess you can make it easier to write tests for all future cases like this | 16:28 |
| dimitern | TheMue, by making the dummy provider *not* support networking when a given environ config setting is there | 16:28 |
| TheMue | dimitern: yeah, the better approach, as we already discussed about, is mnot to use type assertion | 16:30 |
| dimitern | TheMue, yeah - it will make writing tests a lot easier | 16:31 |
| TheMue | dimitern: definitely | 16:32 |
| TheMue | dimitern: I change the card description with my ideas. could you take a look please? | 16:42 |
| dimitern | TheMue, looks good, only the name SupportsNetworking() on netEnv needs to be better I think | 16:45 |
| dimitern | TheMue, but I need to think about it - I'd suggest to go ahead and do it as you suggest, we can rename the NetworkingEnviron method name later as you propose it | 16:47 |
| TheMue | dimitern: exactly, renaming it en bloc is simple | 16:49 |
| dimitern | TheMue, +1 | 16:49 |
| TheMue | dimitern: while doing it, how about ProvidesAddressAllocation or AllowsAddressAllocation for Allocate... and Release... | 17:00 |
| TheMue | dimitern: ? | 17:00 |
| === katco` is now known as katco | ||
| dimitern | TheMue, we have this already - SupportsAddressAllocation - true for both Release and Allocate | 17:27 |
| dimitern | anyway, EOD for me | 17:27 |
| TheMue | dimitern: enjoy | 17:28 |
| dimitern | TheMue, thanks, same to you :) | 17:28 |
| TheMue | dimitern: ta | 17:28 |
| katco | natefinch: hey are you still blocked on feature tests for juju status? | 19:33 |
| natefinch | katco: I haven't spent time on it, but in theory, no. | 19:35 |
| natefinch | katco: I can unblock the card | 19:35 |
| katco | natefinch: k just checking. probably makes sense to stay on the 3-pointer? | 19:35 |
| natefinch | katco: yeah | 19:36 |
| katco | natefinch: kk | 19:36 |
| katco | wwitzel3: were you working on the rename cards? | 19:39 |
| wwitzel3 | katco: haven't started them yet, just about have the feature test buttoned up, then that was my next item to start | 19:40 |
| katco | wwitzel3: cool | 19:40 |
| wwitzel3 | katco: thanks to ericsnow's hand holding, I am just about done with the test | 19:41 |
| katco | lol | 19:41 |
| natefinch | ericsnow: does parseID return name, id or id, name? https://github.com/juju/juju/blob/feature-proc-mgmt/process/info.go#L42 | 20:46 |
| natefinch | ericsnow: nvm, usage indicates name, id | 20:48 |
| ericsnow | natefinch: k | 20:48 |
| mup | Bug #1485781 opened: Juju is unreliable on Joyent <joyent-provider> <reliability> <repeatability> <juju-core:Triaged> <https://launchpad.net/bugs/1485781> | 22:06 |
| mup | Bug #1485784 opened: Error creating container juju-trusty-lxc-template; Failed to parse config <oil> <juju-core:New> <lxc:New> <https://launchpad.net/bugs/1485784> | 22:33 |
| davecheney | thumper: free now ? | 23:27 |
| thumper | davecheney: just munching down some food, 5 min? | 23:28 |
| davecheney | i'll see you in the hangout | 23:28 |
| davecheney | take your time | 23:28 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!