[01:50] hi davecheney [01:50] davecheney: are you working today? [01:51] davecheney: wallyworld is on holiday, and axw has a public holiday [01:51] thumper: is it a public holiday today ? [01:51] i'm terrible at these things [01:51] davecheney: for WA I think [01:51] nup, in in NSW [01:52] i'll be here all week, try the fish [01:52] davecheney: I have a number of small branches that fix saucy issues [01:52] davecheney: https://codereview.appspot.com/14114043/ [01:53] davecheney: oh, just saw your review of it [01:53] davecheney: and it does work on precise [01:53] the other option is --session (for testing only) [01:53] I checked this [01:53] and tested on ec2 [01:55] davecheney: I have a golxc one, and another juju one coming [02:02] thumper: keep 'emcoming [02:03] davecheney: ack [02:03] davecheney: https://codereview.appspot.com/14114044/ [02:04] thumper: LGTM [02:04] reviewed by email [02:05] davecheney: I agree on the name, but that would be a bigger change just now [02:05] I'm trying to keep 'em smallish [02:19] davecheney: it seems it isn't just me failing with this error [02:19] davecheney: the gobot is also failing [02:19] davecheney: could I get you to run the tests on trunk to see if you get it? [02:20] sure, running trunk now [02:21] ta [02:23] same, [02:23] [LOG] 36.85478 DEBUG juju.environs.simplestreams cannot load index "http://127.0.0.1:42617/peckham/private/tools/streams/v1/index.sjson": invalid URL "http://127.0.0.1:42 [02:23] 617/peckham/private/tools/streams/v1/index.sjson" not found [02:23] hmm... [02:24] http://paste.ubuntu.com/6173877/ [02:24] broke [02:24] * thumper wonders how it landed [02:41] davecheney: https://code.launchpad.net/~thumper/golxc/nicer-destroy/+merge/188254 [02:42] * thumper now looks at the failing test [03:02] davecheney: you are running raring? [03:11] * thumper afk for a bit [03:25] thumper: yes sir [04:40] <_thumper_> jam: ping for when you start === _thumper_ is now known as thumper [04:54] thumper: pong [04:54] jam: hangout? fire-fighting [04:55] sure [04:56] jam: https://plus.google.com/hangouts/_/7e75017df572083de566b5fc04dab18866050eb4?hl=en [05:36] jam: https://code.launchpad.net/~thumper/juju-core/revert-1901/+merge/188261 [05:38] jam: https://code.launchpad.net/~thumper/golxc/nicer-destroy/+merge/188254 [06:56] mornin' all [07:05] fwereade: hiya [07:05] rogpeppe, heyhey [07:06] fwereade: looking for a review of https://codereview.appspot.com/14038045/ if you have a mo at some point. (joint work of mgz & i) [07:52] morning [08:02] TheMue: mornin' [08:05] rogpeppe: heya, need a short restart after update [08:08] so, back again [08:16] rogpeppe, reviewed, not sure if there's some reason to mix concerns that I'm not quite getting [08:17] jam, thank you for spotting the AccessDenied [08:17] fwereade: np [08:17] jam, am I right in thinking that mgz has keys to fix that? [08:17] fwereade: It is the ec2 bucket, I don't know who has keys. Dave does, probably curtis does [08:17] I don't [08:18] fwereade, hey [08:19] dimitern, heyhey [08:19] fwereade: the reason i thought it was good to put both the address updater and publisher in the same place is that they both need to respond to almost exactly the same information - it's trivial to do them both together [08:19] rogpeppe, no it's not [08:19] fwereade, https://codereview.appspot.com/14036045/ would you take a look please? [08:19] fwereade: and the publisher is actually thing i actually need out of this work [08:20] rogpeppe, why so? just having the info in state is good enough, surely? [08:20] fwereade: i wanted to avoid doing a scan through all machines every time someone logs in [08:22] rogpeppe, can't we just index by jobs if that turns out to be a cost worth worrying about? [08:22] fwereade: can you index by a set? [08:23] rogpeppe, unless you know for sure that you *can't*, mising concerns like this is seriously premature optimization [08:23] rogpeppe, last resort, not first [08:23] rogpeppe, even if you do know [08:23] rogpeppe, there's nothing stopping a separate publisher task from working with the data collected here [08:24] rogpeppe, and pretending the two tasks are the same is just not helpful [08:24] fwereade: they seem to go together quite nicely to me [08:25] rogpeppe, if your type description says "X does Y. Also, it does Z" you really should be writing either two types, or a long comment detailing the justifications for doing so [08:25] fwereade: we'll also want this logic for publishing the provider stateinfo [08:25] rogpeppe, I'mnot saying the logic is *bad* even [08:25] rogpeppe, just that the package is doing way too much [08:27] rogpeppe, (and when you do the provider state info, I worry you'd say it's "trivial" to add it to this type too, because the tasks go together "nicely"...) [08:27] fwereade: ok, i guess. i thought the publishing bit is a relatively small addition to the rest of the logic, which is concerned with knowing when addresses change. [08:28] fwereade: yes, i'd thought that this package could be concerned with all addressing stuff. [08:28] fwereade: in particular, i'd thought we'd have two places where we'd publish the current set of addresses [08:28] fwereade: in state and in the provider [08:28] rogpeppe, me too [08:28] fwereade: and that the same code can be responsible for both [08:29] rogpeppe, ISTM that that is more than enough reason to put that clever code in its own package [08:29] fwereade: it's not very clever code [08:29] rogpeppe, because then whoever needs to add functionality to it will *only* have that to deal with [08:29] rogpeppe, rather than having to understand all the address-updating stuff as well [08:29] fwereade: i think this is making life harder for ourselves again [08:30] fwereade: but if you insist [08:30] rogpeppe, I am not open to argument here [08:30] rogpeppe, the concerns are separate [08:30] rogpeppe, you've got the first one practically done, it seems [08:30] rogpeppe, it can go in as a worker and start making life easier immediately [08:30] fwereade: that was the plan [08:31] fwereade: I wanted to chat a bit about the ssl stuff, but after you're done with rog [08:31] rogpeppe, and then we can write another worker that might even be properly trivial [08:31] rogpeppe, and really easy to understand and change in isolation for the publish-to-environcase [08:32] fwereade: honestly, the publisher goroutine is really simple, and it won't be that simple when factored out as its own goroutine [08:32] s/goroutine/worker/ [08:32] s/goroutine$/worker/ :-) [08:33] fwereade: because it'll have to duplicate a lot of stuff that this one is doing [08:33] but we like duplication [08:33] rogpeppe, AFAICT the only actual point of overlap is watching all machines [08:33] fwereade: and the environ [08:33] rogpeppe, and that's not really apropriate to a publisher anyway, but it'll do in a pinch [08:34] fwereade: the publisher needs to watch all machines, no? [08:34] rogpeppe, aw man, I guess we're doing the mix-environ-watching-into-everything stuff again?:( [08:34] rogpeppe, depends [08:34] rogpeppe, would be nicer if we could just watch all the state servers [08:34] fwereade: in this case, we can have a separate environ watcher that sets a shared Environ, guarded by a mutex. [08:35] rogpeppe, is there any case we *couldn't* do that in? [08:35] rogpeppe, Environ is meant to be goroutine-safe, right? [08:35] fwereade: yeah [08:35] fwereade: i'm not sure - there might be some cases where we actually want to know when an environ has changed. [08:36] rogpeppe, I would hope not, surely? [08:36] rogpeppe, and if we do, that would seem to be the place for custom environ-watching code [08:38] rogpeppe, anyway axw has some investigation into that in his queue, I think [08:43] jam, ssl? [08:44] fwereade: so. I like smoser's idea to add the cert, mostly because it means I don't have to track down edge cases. It means I still need the code I've landed, because the initial *client* needs to have a way to connect. [08:44] However [08:44] fwereade: It is completely non-obvious how we get the Cert out of the connection. [08:44] jam, ha [08:44] I think we have to create a custom http.Transport object, that overrides Dial [08:44] so that when it connects [08:44] we can peek at the tls.Conn object [08:45] which has a ConnectionState call [08:45] that can have the certs in it [08:45] But the layer at which cloud-init sits [08:45] is about 5 abstractions away from the actuall Conn [08:45] fwereade: and I'm wondering how terrible that is [08:45] The best I can think of is to have a global registry of hostname => Certs [08:45] and then create a custom Transport [08:46] well, custom Dial that adds those certs to the registyr [08:46] and then if you have "ssl-hostname-verification: false" set [08:46] it still does what I've done today [08:46] but then at cloud-init time [08:46] it looks in the global registry [08:46] if there is a cert for auth-url [08:46] and if so, it puts it into cloud-init [08:47] The vagaries of "hostname => certificate " concern me [08:47] jam, I'm shuddering a little there [08:47] but it might be feasible [08:47] fwereade: the net.HTTP stuff doesn't expose any way to get access to the cached Conn objects [08:47] so I can't do it without overriding Dial and peeking at connection time [08:47] jam, that bit seems fine to me [08:48] jam, it's the global registry that freaks me out [08:48] jam, altogether too much action at a distance [08:49] fwereade: so we already have a custom Transport object [08:49] because we have to set tls.SkipInsecuryVerify = true [08:49] it isn't hard to inject a Dial there [08:49] though I'm not sure how to make that dial [08:49] have enough context [08:49] to be able to cache the connection information on the Goose object ? [08:50] jam, weeeell there are always ways... eg can we make the goose object itself supply the custom dial function? [08:51] jam, (I feel like the situation is symptomatic of too many globals, and that adding more is unlikely to bring us to a happy conclusion) [08:52] fwereade: so *today* we are using a shared HTTP Client [08:52] because that seems to be the recommend way [08:52] jam: could you just add the cert to /etc/ssl/certs/ca-certificates.crt ? [08:52] so that you get global connection pooling [08:52] rogpeppe: that is what cloud-init allows for you, the trick is *digging out* the certificate from the connection [08:53] fwereade: we could certainly just punt on all of that (though it is how net/http works), and go with a one http.Client per goose.Client, and then goose.Client asks for an http.Client that has *this custom Dial* func() that is actually an appropriate closure [08:54] fwereade: I'm not 100% sure how we do the juju-side of it. [08:54] Because of simplestreams [08:55] we might not need to [08:55] as in, we leave juju simplestreams as just ignoring the certificate, we teach goose how to grab the certificate, and then we teach juju bootstrap how to ask goose for what the cert is [08:55] note there is still a small problem that the "SWIFT" URL doesn't have to match the AUTH URL [08:56] jam: we can't make it a configuration option, so the user tells juju about their own self-signed cert? [08:56] rogpeppe: how do they get that cert [08:56] jam: off their provider, i suppose [08:56] rogpeppe: users don't really want to connect via Firefox, click on "I understand the warning" then "Download Certificate", copy and paste that into an environment.yaml file (.jenv) [08:57] rogpeppe: my point on the bug is: "ssl-hostname-verification: false" really easy for a user to type and understand [08:57] rogpeppe: go inspect this service over there to pull out its SSL certificate [08:57] rogpeppe: *completely* non-obvious [08:57] jam, +1 [08:58] rogpeppe: right now, I'm looking at how I get "ssl-hostname-verification: false" to work for all our stuff that just downloads from a URL [08:58] cloud-init does [08:58] upgrader does [08:58] charmer does [08:58] etc [08:58] I either propagate ssl-hostname-verification = false into EnvironConfig [08:58] and teach the API [08:58] that for things that return a URL [08:59] they also return a "And you should ignore the certificate for this URL" [08:59] or I teach something like Bootstrap [08:59] to put "here is a new Cert for you to use" [08:59] or you find out the cert somehow, yeah [08:59] jam, I'm starting to feel gordian-knotty here -- I think you should probably just go with skipverify for now, because it delivers actual value to users who specifically say they want insecurity [08:59] to accept [08:59] fwereade: what really sucks is that we have the cert [08:59] jam, giving those users a bit of extra security is just a bonus [08:59] but it is over here on this object that is hidden between 3 interfaces and a type that doesn't expose its internal map [09:00] jam: this problem is almost all about when we're talking to storage, right? [09:00] rogpeppe: right [09:00] rogpeppe: there are 2 problems, but I feel like I've solved the first [09:00] jam: so, storage already exposes a URL method, yes? [09:00] we need to handle connecting to the Provider [09:00] and we need to handle Storage [09:01] rogpeppe: all the agents that aren't on machine-0 don't have a Provider connection [09:01] just a bunch of URLs [09:01] rogpeppe: which is why on Openstack the Storage() has to be a world-readable container [09:01] (swift version of s3 bucket) [09:02] jam: so if we've got a URL for a provider, can we can find out the certificates provided by that URL? [09:02] rogpeppe: per the work we've been doing to put everything into the API, we *really* don't want the Provider secrets on any machine but machine-0 [09:02] jam: ISTM that that's a potential way of bypassing the abstraction layers [09:02] rogpeppe: as I've been saying, yes. You just connect to it, and then the tls.Conn object has a "ConnectionState" which has the certs. But *that* object is very hidden. [09:02] jam, I feel like the urge for a solution is going to cause us either to fuck proper layering hard, or to fiddle with quite a lot of code in order to pass certs around with all the urls we store [09:02] rogpeppe: so yes, we could make the API Server proxy for anything you want to download [09:03] but that is quite a bit bigger change. [09:03] jam: i'm not sure i was suggesting that. [09:03] jam: are you saying that it's not possible to use the net/http interface to make a connection and find out the certs at the other end, regardless of our code? [09:03] rogpeppe: so if we've done the work to extract the Certificate, then we we start an instance, we can tell cloud-init to add the certificate to the accepted certs store for that machine. [09:04] rogpeppe: net/http has a global shared Client that pools connections, and that map of address => connection is not exposed (that I can see) [09:04] rogpeppe: we *can* create an http.Client that uses a custom Dial [09:04] sorry, brb [09:04] and when we get a Dial attempt [09:04] we inspect if it is a tls.Conn [09:04] and if so [09:04] grab the certificate [09:04] but *where do we put it* [09:05] so that we can pull it out later when we get to cloud-init time [09:05] jam: can't we put it into the environ's config? [09:05] rogpeppe: http.Client is *intended* to be a global shared state [09:05] rogpeppe: how do we get it from Dial => environ config [09:07] fwereade, ping [09:07] jam: what i'm trying to suggest is that somewhere outside the provider, if we have insecureSkipVerify, we invent a storage request, try to dial its URL, extract the certificate, and save it in the provider (and possibly change the global http client too) [09:07] jam: so we don't have to wait until the provider does its own Dial [09:08] jam: we preempt it by doing our own first [09:08] rogpeppe: so we don't actually know where storage is until we've connected to the provider [09:08] rogpeppe: openstack uses a registry of URLs for where things like swift is at [09:08] vs how ec2 has "known urls" ahead of time. [09:08] rogpeppe: so you log in, then get back a list of "this is the URL to use for Swift" [09:09] jam: but that's in code that isn't hard to change to allow insecureSkipVerify, no? [09:11] rogpeppe: so that's already been done, but it also means we've already done the Dial, so it doesn't make a lot of sense to do it separately [09:11] jam: i don't mind a bit of inefficiency in this case [09:13] jam: it's only one extra http request, after all; i'm probably missing something though. [09:13] rogpeppe: so we have a fair number of abstractions about what URLs we are downloading from [09:13] there isn't Just One [09:13] we could probably do just Environ.Storage [09:14] (and assume that tools-url is going to match that) [09:14] though there are no guarantees to that effect [09:14] jam: isn't the whole reason for URL so that we can use it in shell scripts ? [09:15] jam: what other abstractions are you thinking about? [09:15] rogpeppe: you mean for env.Storage().URL ? [09:15] jam: yeah [09:15] fwereade, jam, updated https://codereview.appspot.com/14036045/ [09:15] rogpeppe: so you're allowed to specify "tools-url" and "imagemetadata-url" which are just URL roots that we will use to look for image metadata and for tools metadata [09:16] dimitern, heyhey [09:16] dimitern, I will take a look [09:16] jam: is it ok to assume that they use certs signed by the same authority? [09:17] jam: or that if someone uses ssl-hostname-verification=false, that adding a cert from one of them will be good enough? [09:18] rogpeppe, that does not sound ideal to me [09:18] rogpeppe: so ian's design for simplestreams is that it can be any-old-http-server that you want, one of which might be swift/s3 [09:18] rogpeppe: for the *immediate* use case, that might be ok [09:18] though auth-url and swift-url are different machines, I think [09:18] so if they are using self-signed, they might be different self signed. [09:19] jam, can we land it with just the existing disabling in place, and triage a bug for doing it better as wishlist or something? I feel like we're in danger sacrificing better to best [09:19] fwereade: it won't work today with just what I've done so far [09:19] we can bootstrap [09:20] and with the cloud-init it will start [09:20] but Upgrader Uniter etc will still be broken [09:20] I can land this, and work on those [09:20] fwereade: but that is why I was tempted by smoser's idea [09:20] jam: smoser's idea is good *if* you know where to find the certificates to add [09:21] rogpeppe, well, yeah,but it's *that* problem that feels to me like an uncontainable horror [09:21] rogpeppe: so we can iterate over all the simplestreams DataSources and get all of there certs (if any) I suppose [09:21] ah, except the Sources [09:21] use their own connection [09:22] we just call Source.Fetch() [09:22] but we *do* have source.URL [09:23] so for _, source := GetToolsSources(): customClient.Get(source.URL()) => drops the Cert somewhere we can get it [09:24] jam: can we not define our own global http client, and have everything in juju use it? [09:24] jam, but can we even be sure that a simplestreams file will only specify relative addresses for the actual downloads? [09:24] jam: hmm, that's not good either [09:24] fwereade: ha ha, good point [09:24] fwereade: that is part of the simplestreams spec [09:24] jam, ok, sweet, so long as someone's committed to that, I'd missed that [09:25] fwereade: so there is stuff about mirrors, but the design is that the index always gives relative paths, so when you mirror the data, you don't have to change it [09:25] jam, I think there's a disconnect there [09:25] jam, but it's not worth worrying about actually [09:26] jam, ech, or is it [09:26] fwereade: so when we go to cloud-images... it tells us where the data is for amazonaws, etc. [09:26] jam, anyway if it's in the spec this is moot [09:27] fwereade: I'm not 100% sure about how the tools stuff is going to go, we are intending that you mirror tools into a local index [09:29] fwereade: so I think we can avoid doing an HTTP get, we can iterate the sources, get the URL("") and then if the URL.Scheme == "https" do a tls.Dial and grab the cert out of there. [09:29] (if ssl-hostname-verify: false) [09:30] create a Set() of those certs, and the add them all to cloud-init [09:30] jam, ok... we're still left assuming that those certs won't change... is that ok? [09:30] we probably still need to use ssl-hostame-verification: false when talking to the Provider itself (maybe), or we include auth-url as one of the bits we want to add [09:30] fwereade: I think for the use case, it is fine. I was worried about that as well [09:31] fwereade: but I don't think people are going to change their self-signed certs and expect juju to upgrade in place [09:31] I think [09:31] jam, I guess it's the same problem as updating authorized-keys in essence anyway [09:31] jam, ie we could actually build the infrastructure to handle it if we had to [09:31] fwereade: well ssl-hostname-verification: false would just disable it always, right? [09:31] fwereade: we'd start managing certificates [09:31] which I would love to avoid [09:32] (oh, revoke that certificate, add this one), but I guess people want us to do that for authorized-keys as well [09:32] jam, that was my thought, yeah [09:33] fwereade: I think it is worthwhile to think how this interacts with the httpstorage proposal as well [09:33] (a storage url may not be available before bootstrap time ?) [09:34] jam, anything using it before bootstrap time will be able to get what it's looking for off the filesystem, won;t it? [09:35] fwereade: I'm meaning httpstorage with the local provider [09:35] we're talking about it exposing https [09:35] and I guess we'll do something about accepting that cert [09:35] fwereade: I thought I saw an axw commit that said "we'll need to disable certs for this" [09:40] jam, I may have missed that bit... but the wouldn't the environment's CA cert be what we'd use/need there? [09:40] fwereade: I honestly don't know what the plan is, and axw is already gone for the day [09:41] jam, ok, fair enough [09:41] fwereade: I just know of it as yet-another HTTPS source we might need to worry about [09:42] jam, indeed, got you [09:42] I don't really know how we set ca-certs for those instances [09:42] I don't think we use cloud-init there [09:42] given we would have to run a metadata server on the users' machine [09:42] (I think) [09:45] jam, alternate tack again: the cost and complexity of adding and using a bool-returning api method for each of the facades is known to be small, and fulfils the current use case adequately if not admirably [09:46] jam, the cost and complexity of the alternatives stm to make it unlikely that we'll get an adequate implementation anywhere near as soon [09:47] dimitern: so for https://codereview.appspot.com/14036045/ couldn't we have something that takes a list of jobs and tells you if you need state access? [09:47] jam, no argument, the admirability ceiling of keeping track of the actual certs is much higher, but it feels wishlisty [09:47] that seems generic enough to work whether or not you're on the end of the API or directly on state [09:47] fwereade: well, it was also impacted by the fact that scott raised the question, and nobody else reviewed the proposal :) [09:48] fwereade: so I certainly considered the "add this cert manually" but the feeling of how to do the manual cert seemed terrible for users. So I explored this possibility of doing it for them [09:48] fwereade: but yeah, disabliing it everywhere seems more directly straightforward [09:48] fwereade: and avoids the "oh you need 3 certs for the various services, etc" [09:49] jam, don;t get me wrong, I am happy that you explored it, it's exactly the sort of thing I'd like us to be considering by default [09:50] fwereade: yeah, I felt it was worth discussing at least, I certainly wasn't committing to code it yet, but I did investigate to see what it would have taken. [09:51] If net/http could have exposed the existing conn I was pretty interested. Having to do it via inspecting Dial made me a bit sad. [09:51] jam, I originally though to add a method on MachineJob to return true if the job needs state, but we need the same one on params.MachineJob and state.MachineJob [09:51] dimitern, that's a bit surprising [09:51] dimitern, where do we get a state.MachineJob when we're not connected to state? [09:52] fwereade: so there are 2 aspects (AIUI). One side needs to know if it should add a MongoPassword, and the other side needs to know if it should ensureStateConnection [09:52] ensureStateWorker [09:53] dimitern: so I think your point is that we actually have 2 Job types [09:53] one that is exposed on the API [09:53] and one that is directly in state [09:53] and so 1 function wouldn't take both types of objects [09:53] shame they aren't just an Enum [09:54] jam, dimitern: there are a few places where types moved from state to api rather than being copied... what are the forces that led us otherwise here? [09:54] fwereade, the tricky part is the machine agent code [09:55] fwereade, there we have both a state connection and an api connection [09:55] dimitern: because we might run the env provisioner? [09:55] fwereade, and we can't have the latter until we know we can connect - i.e. not bootstrapping and we know our jobs [09:55] dimitern: I think fwereade's point is that why aren't they just params.MachineJob enums ? [09:55] jam, because of JobManageState, but also because of the firewaller [09:56] jam, the env provisioner also uses the api [09:56] dimitern, jam: more to the point, they're ints in state and strings in the api [09:56] bah [09:56] fwereade: good times [09:56] fwereade, all int consts are strings in the api - c.f. live [09:56] jam, dimitern: however, no reason not to keep the int storage and expose them in methods as params.Job, right?> [09:57] fwereade, that's because json doesn't have true ints IIRC [09:57] dimitern, and we want it to be half-way comprehensible too:) [09:58] fwereade: right, in an API call it is nice to see "hosts-units" vs "1" [09:58] but that would have been a reason to put them as strings into the DB as well :) [09:59] dimitern: so I think the idea is that we would have state.JobHostsUnits only long enough to turn it into params.JobHostsUnits. [09:59] But that sounds like EOUTOFSCOPE for your patch [09:59] I'd probably still rather it be a function that takes a slice of jobs [10:00] rather than putting functions on what is otherwise a blob [10:00] hey jam [10:00] mgz: mumble/hangout ? [10:01] jam, fwereade, if it has to be a helper taking a slice of jobs, we still need 2 helpers [10:02] dimitern, to be fair, if one of them is a wrapper that does the conversion and calls the other, that wouldn't be so bad, would it? [10:05] mgz: ping [10:05] fwereade, ok I suppose [10:05] dimitern: so I don't feel like we need tons of overengineering, but we did have logic that needed to be changed in several places to keep them in sync [10:05] it certainly felt like it should be centralized [10:06] not helped by "this is an if clause, this is a map index, etc" [10:06] rogpeppe: hey [10:07] mgz: do you want to continue with the address worker at some point? [10:07] rogpeppe: that would be good [10:08] after standup? [10:08] mgz: sounds good [10:33] jam, rogpeppe: ISTM that provider.StartBootstrapInstance and provider.StartInstance are out of whack [10:33] fwereade: how so? [10:33] jam, rogpeppe: it looks like StartInstance is something you do *to* an environ, and should be in environs [10:34] fwereade: so I think the idea is that we have 90% common code between all implementations [10:34] jam, rogpeppe: but StartBootstrapInstance looks like it's a common implementation of Bootstrap [10:34] that set up the machine-config etc [10:34] jam, provider implementations call StartbootstrapInstance but, other code calls StartInstance [10:36] jam, rogpeppe: I thought the idea of the code in the provider package was to help people implement providers [10:36] fwereade: i think you're probably right [10:36] fwereade: I have a feeling it was exploring the bounce-and-bounce-back stuff. (Do we call Env.Foo() which calls something common, and then calls back on Env or do we just call something common, or...) [10:36] fwereade: I have the feeling someone found the balance different each time [10:37] but we should make them at least similar [10:37] fwereade: i mean, you're definitely right that the code in the provider package is to help people implement providers [10:37] fwereade: and i think you're probably right about StartInstance. i'm just looking around - i'm not that familiar with it [10:37] fwereade: (although i suppose i may have been responsible for putting it there!) [10:38] rogpeppe, jam: ehh, we progress uncertainly, but we do progress [10:38] rogpeppe, jam: they're certainly named very confusingly given the different domains though [10:38] fwereade: agreed [10:39] * rogpeppe hates the .(T) magic strewn around seemingly at random [10:40] rogpeppe, jam: ok, I think I'll move StartInstance over to Environs in a mo [10:40] it feels exceedingly fragile to me [10:40] fwereade: it is also exceedingly unhelpful that env.StartInstance does already exist [10:40] jam, oh, wtf [10:40] and is called by provider.StartInstance IIRC [10:41] jam, ahh Environ.StartInstance, sorry [10:41] fwereade: "last line of StartInstance is broker.StartInstance" [10:41] and broker == environ [10:41] (if env, ok := broker.(environs.Environ)" [10:41] jam, yep [10:41]