[00:52] wallyworld: is there a page on the wiki describing the days the company is closed this year ? [00:53] davecheney: not sure, i know there was something about the 28th. let me see if i have anything in my email [00:55] davecheney: this is all i know - it says company closed between xmas and new year https://wiki.canonical.com/PeopleAndCulture/Policies/Leave [00:55] i love how out of date this information is [00:55] but there's a separate page somewhere with reference to the 28th being a free day [00:55] it makes it really useful [00:55] yeah [00:56] there is an up to date pdf on the page [00:56] thanks [00:58] davecheney: i think that pdf is the one with the free day etc [06:48] Morning === TheMue_ is now known as TheMue [07:19] TheMue, rogpeppe: heyhey [07:19] fwereade: Hi [07:21] fwereade: The improved firewaller is in at https://codereview.appspot.com/6875053/ [07:21] TheMue, cool, looking [07:21] fwereade: Cheers [07:42] TheMue, what happens when a machine has a unit removed after initGlobalPorts and createMachineData? [07:42] s/and/and before/ [07:43] fwereade: This is catched by the error check for an unassigned machine. [07:44] TheMue, and what happens in response? [07:45] TheMue, I don't get how you can distinguish between "this unit is not assigned to the machine I'm looking at: keep it around until we find the machine that does" [07:45] fwereade: Today the unit is skipped, but you're right, it should be dropped from those temporary stored units. [07:46] TheMue, but then the first machine you process will trash all the others, won't it? [07:46] fwereade: Why? [07:47] TheMue, because it will go through all the valid data saying "nope, not mine, trash it" -- won't it? [07:47] fwereade: Each machine data, when created, is scanning the remaining units from the initial scan and checks, if that unit is assigned to the machine the data is representing. [07:47] fwereade: No, I only should trash those without any assigned machine id. Today I just skip it. [07:47] TheMue, yes [07:48] TheMue, and you *have* to [07:48] fwereade: Line 228. [07:48] TheMue, because that unit might be assigned to another machine [07:48] fwereade: So far it's by accident as good as it is. ;) [07:49] TheMue, ok, what I'm afraid I'm saying is that you have I think rearranged the problems but not fixed them [07:49] fwereade: I had this when during testing units have been created while init runs in background and those units are not yet assigned. [07:50] TheMue, the fundamental problem is that you cannot effectively take a delta between the initial-scan state and the state at the point where you've started all the watches you need [07:51] fwereade: machineData and unitData are proper initialized before returning to the loop. I had masses of runs with debugging statements. They shown that, depending on who is faster, init, adding or mixed, the state afterwards is always correct. [07:52] fwereade: Yes, you never can, because scanning it initially takes time, especially when state is large, and during this time state can be changed. [07:54] fwereade: So this CL fetches its initial state as good as it can and at the time machineData and unitData are created it looks if those initial units match to them. [07:54] TheMue, so how are you in a position to assume that the initial-changes state that you build up corresponds to the globalUnitPorts state you built up before? [07:55] TheMue, ISTM that a removed unit will stay in globalUnitPorts forever [07:56] fwereade: Yes, that's the only problem I still have. I have no kind of safe signal that says "Now you can clean it up, guy." [07:56] TheMue, yeah [07:57] TheMue, and this is an inescapable consequence of the decision to separate the initial scan from the creation of the watchers, isn't it? [07:57] fwereade: One way could be a kind of timer checking those entries from time to time if they are removed. [07:57] TheMue, it's just another layer of paper over the problem [07:57] fwereade: Sure. [07:58] fwereade: We need the initial scan to look if the current open ports are those we need when the Firewaller starts. [07:58] fwereade: And the initial scan needs time. [07:59] fwereade: And during this time the state can (and will) change. [07:59] TheMue, understood -- but why don't you also create the *Datas at that point? [07:59] TheMue, they hold the watchers which have properties that allow us to avoid this problem [07:59] fwereade: Thought about it, niemeyer disliked it. [07:59] fwereade: But here it's the same problem. [08:00] fwereade: Starting those watchers still produces a gap while scanning. [08:04] TheMue, the initial events from those watchers *give* you the initial state, don't they? [08:05] fwereade: At least if I start the right ones which may possibly not happen because the state changes while I'm scanning to know which watchers I have to start. [08:06] TheMue, er, don't you just need to start the machines watch and handle the initial event? [08:07] fwereade: That's what the firewaller has done in his first "release", just that. [08:07] fwereade: :/ [08:08] TheMue, I've never seen the FW setting up initial UD state by consuming the first event from the unit watcher before starting the watchLoop [08:08] TheMue, did it really do that> [08:09] TheMue, AFAICS the only way to do this reliably is to follow that model [08:09] fwereade, TheMue: morning! [08:09] fwereade: Please rephrase the UD sentence. [08:09] rogpeppe: Morning. [08:09] TheMue, if everything in the data tree makes sure it's initialized via initial event before starting its watch loop [08:10] TheMue, then all you need to do the global port open/close (just once) at the end of the first machinesw.Changes event [08:10] s/need to/need is to/ [08:11] no that s/ is crack [08:11] whoops, no, it's what I meant [08:11] TheMue, er, am I making any sense? [08:13] fwereade: I have to think about it more. [08:13] fwereade: It would avoid possibly remaining removed units in that one map, yes. [08:14] fwereade: But it would totally change the way of initialization and have to check how this effects the instance mode, because we here talk about the global mode. [08:14] fwereade: *sigh* [08:19] TheMue, yeah, I'm sorry :( [08:19] fwereade: Fridays discussion ended with this proposal, but seems it falls too short. [08:22] TheMue, I was always trying to communicate that separating watcher setup from initial scanning was intrinsically broken -- sory I haven't been very effective at that [08:22] fwereade: Funnily it works really fine, it only has the I-dont-know-if-those-left-units-are-removed-during-startup-in-global-mode-problem. [08:22] TheMue, which is exactly the bug we're concerned about [08:23] TheMue, fwiw, you've also introduced a goroutine-unsafety bug in unitData.watchLoop [08:23] TheMue, ud.ports is used on the main goroutine [08:24] fwereade: Could you point me to the line? [08:24] TheMue, old: line 542 removed; new: lines 612, 615 changed [08:25] fwereade: IMHO the goroutine is started after initialization (btw opposit to the former solution). [08:25] TheMue, sorry, rephrase please? [08:26] fwereade: Seems to be independent, sorry. I have a deeper look. Thought you meant something different. [08:27] jam, wallyworld, rogpeppe: PTAL https://codereview.appspot.com/6877054/ [08:28] fwereade: But I see the problem, yes. I stumbled about the same naming ports as field (where it's just used by the Firewaller itself) and the local variable. Shit, has been intended, but not lucilly. [08:28] luckilly [08:28] Hehe, this word doesn't exist. [08:28] Just lucky. [08:28] TheMue, sorry, I'm still a bit confused :) [08:30] fwereade: In the old version the unitData has a field named ports and in watchLoop() a variable named ports. I thought that this has been a mistake, but that has been intended. [08:30] fwereade: So my "correction" has been wrong. [08:30] TheMue, yeah, it's definitely required, but it demands close reading -- probably worth a comment for future reference [08:30] fwereade: Yes. [08:30] TheMue, otherwise people will definitely try the same "correction" in future :) [08:31] fwereade: Maybe also a better naming. Dunno, will think about it. First I'll mark it again as WIP. [08:32] TheMue, cool, thanks [08:32] fwereade: Done [08:33] fwereade: So the problem now is, that following your/our approach is a larger change. [08:34] fwereade: And starting the datas based on the initial event is independent of instance or global mode. [08:34] fwereade: The later ony is interesting to mark ports as opened and to manage the ref count. [08:35] fwereade: Which can be handled during this startup phase. [08:36] fwereade: So, before entereing the firewaller loop, init is called waiting for the first machine watcher event. [08:37] fwereade: It starts the machines, but then? Hmm, how do I know that all unit watchers and service watchers are set up based on it? [08:38] fwereade: All machines have to notify the init via a channel when they are done and the same with units and services, bottom up. [08:38] fwereade: Does that makes sense? [08:39] TheMue, sorry, I'm still processing [08:40] * TheMue thinks of a concurrently initializing tree which at least has to signal the firewallers init that it's done so that it can continue and enter the loop. [08:40] TheMue, the trouble is partly that I haven't ever analysed the non-global mode [08:41] fwereade: Hehe [08:42] TheMue, but I *think* that the "initial scan" can be perfectly synchronous if the MD creation goes something like... [08:42] w := m.WatchUnits() [08:42] units := <-w.Changes() [08:42] for _, u := range units { [08:42] // create unit data similarly [08:42] } [08:42] go md.loop(w) [08:43] TheMue, (very poorly sketched ofc) [08:44] fwereade: When are the ud loops started? [08:44] TheMue, exactly the same model as the MD ones [08:45] fwereade: Then it's very similar to my thought, only expressed in code. ;) [08:45] TheMue, so, after you've created all your MDs (synchronously) in the first event [08:46] TheMue, sorry -- it's the thought I've been trying to communicate for a week or 2 ;p [08:46] fwereade: *rofl* [08:46] fwereade: To come back to the mode question. [08:47] fwereade: IMHO independent of the mode the startup of the watcher is always the same. What differs is, how the firewaller itself (the type Firewaller) handles those informations based on the mode. [08:48] fwereade: And here instance and global mode have to be handled differently, yes. [08:48] TheMue, yes, indeed -- in global mode you can do a flush-everything at the end of the first machine event, and in instance mode just flush each machine as it's handled [08:49] fwereade: +1 [08:49] TheMue, I *think* this also means you can drop everything global* except globalPortRef [08:50] fwereade: A ref != 0 means true, yes [08:50] TheMue, you just need to be a little bit careful about updating it for initial events in create methods *only* before the first init run has completed [08:50] TheMue, and subsequently doing so only as a consequence of flushes [08:51] TheMue, ...or maybe I'm overcomplicating it [08:51] fwereade: Could you please explain your thoughts more. [08:53] TheMue, I think that I misspoke -- you never need to touch the refcounts while you're doing the "initial scan", they can be handled purely on the main goroutine as they currently are [08:54] fwereade: Am I right you're thinking of a kind of "init" argument set to true during startup and later being false and internally work as described above only when it's true? [08:54] TheMue, I *was*, but I don't think it's needed by anything except the main FW loop, so it can just be a var therein [08:55] TheMue, sorry, bbiab [08:55] fwereade: Hmm, I need the refcounts at the end of the startup to compare the needed ports to the currently open ports. [08:55] TheMue, you can build the refcounts during the initial global flush, can't you? [08:56] fwereade: Pardon? [09:02] TheMue, sorry, back [09:02] fwereade: NP [09:03] TheMue, when you first do the initial global mode flush, won't you have a complete set of MDs and UDs available? [09:03] TheMue, so you can just build the refcounts from that data [09:04] TheMue, I think? [09:05] fwereade: So you think of an explicit flush at the end of the init? Otherwise I have somewhere to remember that it's the first time. [09:05] TheMue, yeah, exactly -- I'm saying that I think the only place you need that information is within the main FW loop [09:07] TheMue, I could well be wrong though -- I'm hardly conversant with all the details of the FW [09:07] fwereade: In the loop? Opposit to a firewaller field? [09:07] TheMue, yeah -- I can't see when else you'd need it -- but you know the FW better than I do [09:08] fwereade: We're still talking about the ref count? Just to make sure. [09:08] TheMue, maybe "initial global mode flush" is the wrong term? [09:08] TheMue, yeah, we're talking about the refcounts [09:08] TheMue, they can be build up in initGlobalMode [09:09] fwereade: I need access to it in the method flushGlobalPorts(). [09:09] TheMue, but *that* only needs to be called, once, at the end of the first machines change [09:09] TheMue, doesn't initGlobalMode call flushGlobalPorts? I may be confused [09:10] fwereade: Not today, but I would do it at the end of the init after the change we talked about and when global mode is active. [09:10] TheMue, ah, no, I'm on crack, sorry [09:10] TheMue, so "flush" was the wrong term [09:10] fwereade: But it's also needed later, it is the alternative to flushInstancePorts() [09:10] fwereade: You smoke too much. *lol* And maybe the wrong stuff. [09:11] fwereade: *scnr* [09:11] TheMue, what I'm trying to say is that if you call initGlobalMode after handling the first machines change [09:11] TheMue, you can use all the MD/UDs that have been built up -- and never have to hit state at all [09:11] TheMue, it's a simple loop through all the units to count up the port refs [09:12] TheMue, and then do the usual open/close as you currently do at the end of initGlobalMode [09:12] fwereade: Yes, that's right, here I would update the refcounts based on the datas, compare it to the open ports and handle the needed opens and closes. [09:12] TheMue, ...and then you're done, because you *know* that any changes from that state are already being watched for [09:13] TheMue, and will be handled sanely once you hit the main loop select again [09:13] fwereade: Bingo! [09:14] TheMue, the only wrinkle about which I am still uncertain is service exposure [09:15] fwereade: If that changes during init? [09:15] TheMue, just in general -- I know there's code to handle it but I've never needed to tease out the details [09:15] TheMue, but, yes, the same general strategy needs to be used, I think [09:16] fwereade: We're watching the expose flag to see if a service is exposed. That's covered today in init (what will change) and during runtime (by the watches). [09:18] TheMue, yeah -- my point is that you need to unify init and watcher setup in the same way we discussed for MD/UD [09:19] fwereade: Yep, the whole chain MD/UD/SD. [09:20] TheMue, I think it's pretty easy so long as you're careful to make sure all your *Data creation is done on the main goroutine [09:20] TheMue, including for subsequent events [09:21] fwereade: Yes, sounds reasonable. [09:21] TheMue, cool :) [09:22] fwereade: So I think I'll exactly do this now. [09:22] TheMue, great [09:22] TheMue, tyvm for your time on this, I'm sorry it's such a hassle [09:23] fwereade: No, it's absolutely great to do this kind of design review. [09:23] fwereade: I have to thank you. [09:25] TheMue, always a pleasure :) [09:41] btw TheMue, or rogpeppe, I need another review on https://codereview.appspot.com/6864050/ if either of you has a moment [09:43] fwereade: looking [10:06] fwereade: you've got a review [10:07] fwereade: BTW i don't *think* it's possible for the transaction mechanism to determine which op failed. [10:07] fwereade: because the asserts are executed in a distributed way [10:07] rogpeppe, I'm still not sure it's a good idea -- but it should at least be able to figure out the doc, right? [10:07] fwereade: i'm not sure [10:08] fwereade: because it can't do the check and write the doc in the same moment [10:09] fwereade: if someone else happens to execute the transaction, how do we get that information back to the entity that started that transaction? [10:12] rogpeppe, you could be right [10:12] rogpeppe, I suspect it would still be possible, but almost certainly not worth the effort :) [10:13] fwereade: i'm not sure actually. seeing when a transaction fails and writing some information about it is non-atomic, and it needs to be atomic to work. [10:16] rogpeppe, maybe so, I certainly don't claim to understand all the nuances of txn's implementation [10:17] fwereade: neither me :-) [10:31] rogpeppe, TheMue: I have a couple of responses to the reviews, would appreciate your thoughts [10:31] rogpeppe, (I'm not disagreeing that a comment is called for, btw, I'm explaining the logic in case I'm on crack) [10:31] fwereade: Already writing. [10:33] morning Aram [10:33] yo [10:40] fwereade: replied [10:41] Aram: hiya [10:41] hey there [10:41] rogpeppe, TheMue, cheers guys [10:41] fwereade: YW [10:41] Aram: Hiay [10:41] Hiya [10:41] hello. [10:42] Grmpf, fingers too fast. [10:44] oh, bugger, doc appointment, bb after lunch [11:33] dimitern: hi, i just commented on your mp [11:33] wallyworld: hey, 10x [11:33] sorry about the delay, i was eating dinner and having a few tasty drinks :-( [11:33] :-) i mean [11:34] wallyworld: no worries :) [11:34] wallyworld: the FlavorDetail used internally is enough to return lists of either Entity or FlavorDetail with the HTTP API [11:36] dimitern: ok, so long as the current goose nova client can unmarshall the data as returned by the double, then i;m happy :-) [11:36] wallyworld: certainly [11:37] dimitern: i'll plug in the live/local tests once this lands and we can see how it all pans out [11:38] wallyworld: yep, can't wait already [11:39] rogpeppe: when you can, could you look at my CL please? [11:39] wallyworld: i've just sent another review of your errors CL [11:39] dimitern: yeah, when you have worked on a branch for a little while, you just want it to land! [11:39] wallyworld: perhaps we could have a chat about it some time [11:39] dimitern: which one? [11:40] rogpeppe: ok, let me take a look [11:40] rogpeppe: https://codereview.appspot.com/6877054/ [11:40] dimitern: looking [11:43] rogpeppe: happy to chat when you have a moment, after you finish reviewing. just ping me [11:46] dimitern: sorry, "d" is shorthand for "delete this line" (ed(1) syntax :-]) [11:47] rogpeppe: I see :) [11:47] Hello all [11:47] niemeyer: mornin' [11:47] niemeyer: hiya [12:15] dimitern: you've got another review [12:15] wallyworld: ping [12:16] rogpeppe: thanks! [12:16] rogpeppe: mumble? [12:16] wallyworld: what *is* mumble?? [12:16] oh, maybe google hangout? [12:16] rogpeppe: mumble is a chat program, like skype [12:16] wallyworld: i'd be happy to use mumble if i knew what it was :-) [12:16] except free, open source etc [12:16] wallyworld: ah [12:17] wallyworld: i'll give it a go. presumably i can apt-get it? [12:17] yeah, believe so. but you need a passwrd etc for the server - it's on y=the wiki [12:17] i'll see if i can dig up a link [12:18] rogpeppe: https://wiki.canonical.com/StayingInTouch/Voice/Mumble [12:27] wallyworld: ok, i'm on mumble [12:28] wallyworld: which space do you use? [12:28] rogpeppe: goto cloud engineering kitchen? [12:33] rogpeppe: settings links to []nova.Link{} prevents AddFlavor/Server from generating them (it does that when links == nil). Not setting them will cause DeepEquals to fail and will force the unnecessary IMHO creating the full list of links every time I need to compare what's in the state vs. what I set there. [12:34] s/settings/setting [12:50] * niemeyer => reboot [13:12] fwereade: ping [13:12] niemeyer, pon [13:12] er, pong [13:12] fwereade: Heya :) [13:12] fwereade: pon is great, though [13:12] niemeyer, how's it going? [13:12] fwereade: We should totally switch over [13:13] pin.. pon [13:13] niemeyer, yeah, think how much typing we'd save :) [13:13] niemeyer, thanks for your reviews last night, very helpful [13:14] fwereade: No worries.. that's actually what I'm pinging about.. is there anything you'd like to chat on to unblock? [13:15] niemeyer, I think I'm good, thanks [13:16] niemeyer, I'm still marshalling my thoughts re testing methods [13:17] fwereade: Coolio [13:17] niemeyer, I guess the differing perspective may be that I see the testing methods as implying "do all required busywork to get to the point where I can call, say, RemoveService" [13:18] niemeyer, but the current uses are not very compelling [13:19] niemeyer, I'm fine dropping AddAnonymousService, but I reserve the right to get it out and wave it around if and when I spot a situation that deserves it, even if it probably never deserves to go on a shared test case [13:19] s/case/suite/ [13:20] ;) [13:22] fwereade: Hehe :) [13:22] fwereade: Sounds good :) [13:38] lunchtime, biab [14:00] rogpeppe: Regarding this comment: [14:00] """ [14:00] one possibility would be to allow forcing the version number only, and have [14:00] version.Current always report the current series. [14:00] """ [14:00] rogpeppe: I don't get the idea [14:00] rogpeppe: version.Current.Series already does report the current series [14:01] on a call, back in mo [14:02] rogpeppe: Suppa [14:06] niemeyer: version.Current.Series doesn't report the current series if the version is forced, i think [14:07] niemeyer: i may be wrong, let me check [14:07] niemeyer: no, that's right [14:07] rogpeppe: Ah, I see what you meant [14:08] rogpeppe: Hacking the hack sounds a bit hackish :-) [14:08] niemeyer: actually we'd be making the hack slightly smaller [14:08] niemeyer: by making it change the version number only [14:09] rogpeppe: I guess so [14:09] rogpeppe: I'll have a look at this [14:12] brb cleaner cleaning [14:18] rogpeppe: Actually, on a second thought, I don't understand why that matters for that particular CL [14:18] niemeyer: aren't you trying to verify that an instance with the given series is actually started? [14:19] Yes, and where's current series picked from? [14:19] Ah, from the system [14:19] That's slightly unexpected (by me) [14:20] niemeyer: where else could it come from? [14:20] It basically means what we get out of the agent tools is reporting the machine, not the actual running tools [14:21] Not sure which way is more useful, though [14:22] niemeyer: they're both potentially useful [14:22] Yeah [14:22] niemeyer: but it's hard to bake the current series into the compiled executable [14:22] rogpeppe: Well.. I'm coding a change right now to *avoid* doing it, so it shouldn't be so hard.. 8) [14:23] rogpeppe: Either way, we'd be guessing in both directions.. so we don't have to make any changes to that right now [14:23] I'll just do that one side-change as an independent CL and see how it looks [14:24] After lunch, though [14:24] biab [14:59] dimitern, wallyworld, jam: is http://paste.ubuntu.com/1416964/ expected? I just updated goose... [15:00] fwereade: well, that seems you're missing some openstack env vars [15:01] dimitern, surely the tests should pass on a clean system? [15:01] fwereade: if you don't pass -live to the tests it should not matter [15:01] dimitern, I'm pretty certain my env vars should not be able to cause tests to either fail or succeed :) [15:01] dimitern, I didn't, though [15:02] dimitern, I concede the point re -live though :) [15:02] fwereade: hmm.. well, I don't know if wallyworld is around - that are his last changes [15:03] dimitern, bah, I never know whether to dive in and fix myself, or revert, or flag and move on [15:04] fwereade: as a workaround until fixed, you'll need to set these to non-empty strings: OS_USERNAME, OS_PASSWORD, OS_TENANT_NAME, OS_REGION_NAME [15:05] fwereade: if tests are failing I think revert is the best bet [15:06] dimitern, yeah, probably sensible :) [15:06] rogpeppe, TheMue: https://codereview.appspot.com/6864050 comments addressed I think [15:08] (fwiw, does anyone know best practice for reversing a merge into trunk? absent better ideas I'm just going to copy the old version over the top and ask for a trivial LGTM on trust) [15:09] bah, it's not the latest [15:09] fwereade: last time i did it, i used patch with a revert diff [15:09] reverse diff [15:10] fwereade: LGTM BTW [15:10] rogpeppe, yeah, I'm *sure* bzr would do it for me if I knew what I were doing :) [15:13] fwereade: Review is in. [15:27] niemeyer: next stage in bottom-up API implementation: https://codereview.appspot.com/6913043 [15:28] rogpeppe: Cheers [15:39] dimitern: Can you give a hand there and propose a fix to avoid the issue for the moment? [15:40] niemeyer: what fix? [15:40] dimitern: One that unbreaks tests [15:40] niemeyer: ok, I'll take a look [15:40] dimitern: Thanks a lot [15:40] niemeyer, dimitern: I just proposed a branch with that commit straight-up reverted -- https://codereview.appspot.com/6907049 [15:41] niemeyer, dimitern: I'll go ahead and reject it if a fix is incoming [15:41] fwereade: I'm assuming that a trivial temporary fix for this would be under 10 lines of code.. if it seems any controversial, +1 on reverting [15:41] fwereade: I have no idea what the fix should be yet, looking.. [15:41] rogpeppe, fwiw, `bzr diff -r-2..-3` gave me a patch that I could just apply [15:42] fwereade: yeah, that's what i was suggesting [15:42] niemeyer, gut says it's a bit of trivial test setup [15:42] fwereade: sorry if i wasn't clear enough [15:42] rogpeppe, not to worry [15:43] fwereade: i cannot even compile the tests from trunk - go test -i in environs/openstack says: ../../state/open.go:72: undefined: mgo.DialWithInfo; mgo.DialInfo [15:43] dimitern, if it looks like it'll be complex then maybe just give my CL a quick check and let me know if it's sane [15:44] dimitern, go get -u labix.org/v2/mgo/... [15:44] dimitern: Update your mgo [15:44] What fwereade says [15:44] dimitern: How long since you last run tests, btw? :-) [15:44] fwereade: ok I'll check out the CL in the mean time [15:44] niemeyer, I think he's been on goose :) [15:44] Ah, that'd explain it [15:44] niemeyer: a week probably - been working on goose mostly, not juju-core [15:45] fwereade: Can we do that on our side maybe, if we know what to do? Is it just a matter of putting those vars in the env? [15:46] niemeyer, no idea, it probably is [15:46] niemeyer, I was trying to avoid distracting myself... didn't actually manage that, though :) [15:46] fwereade: If it is, +1 on adding them next to a TODO: This is wrong as the test shouldn't have to hack the environment to work out-of-the-box. Please fix at next chance. [15:46] fwereade: LOL [15:47] fwereade: the CL looks fine to me [15:47] dimitern, I'll see if there's an easy fix -- we can race :) [15:49] fwereade: I don't think the revert was premature - ListServers() for example in the test I have has some arguments in the provider, but not in goose, so I think it's half-baked anyway [15:50] dimitern, yeah, looking at the tests I haz a bit of a confuse, it looks like it *ought* to work [15:52] it's going to be interesting keeping goose and juju-core in sync [15:52] Slightly unfortunate but if the APIs are diverging, there's no other way [15:52] rogpeppe: Feels pretty easy [15:53] rogpeppe: We've been doing that with every other package for quite awhile [15:53] niemeyer: true, but the external packages weren't perhaps changing quite as fast [15:53] rogpeppe: Sure, but the idea is still the same.. merge only when both as ready to go in [15:53] s/as/are [15:58] rogpeppe: https://codereview.appspot.com/6907050 [16:04] dimitern: It doesn't look like the API is broken, though? [16:04] dimitern: It seems to compile fine here [16:04] niemeyer: maybe I'm not getting something right then [16:06] dimitern, ah, no, it's just a broken string (and tests which appear to run twice), plus whatever the deal is with local_test which I don't really get at all [16:06] fwereade: I think the local tests are supposed to use the test doubles [16:08] dimitern, yeah, but none of those tests actually hit anything let alone the test double [16:09] fwereade: no idea, wallyworld would know I haven't looked at them yet [16:09] niemeyer: looking [16:09] dimitern, ISTM that they're failing to construct an env and trying to get the provider from that [16:11] fwereade: I have a fix.. pushing [16:11] dimitern, it looks like that's what we do in ec2, though, although I can't figure out *why* [16:13] fwereade: probably it needs both tenant and region, which is not needed for ec2? [16:13] dimitern, it doesn't need an env at all :/ [16:14] dimitern, it's creating one for absolutely no reason [16:14] fwereade: I see, so it shouldn't? [16:14] niemeyer: LGTM [16:15]