environs: add client version argument to Bootstrap

environs: add client version argument to Bootstrap



https://code.launchpad.net/~rogpeppe/juju/go-environs-add-bootstrap-version/+merge/102896

(do not edit description out of merge proposal)

[rog, 2012-04-20 16:30:33]

Please take a look.

[fwereade, 2012-04-20 16:32:41]

LGTM

[TheMue, 2012-04-20 17:12:38]

On 2012/04/20 16:30:33, rog wrote:
> Please take a look.

LGTM

[niemeyer, 2012-04-24 00:41:48]

https://codereview.appspot.com/6081044/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6081044/diff/2001/environs/interface.go#newcode58
environs/interface.go:58: Bootstrap(clientVersion string) error
Why is this necessary? Bootstrap must indeed send the client version to the
server, but I suppose that's the version of this client, which is in fact a
global somewhere.

Also, this version will not determine the version of juju to be run on the
server as the docs describe above. It will allow the server to pick the correct
version, which may be a newer or older one depending on availability (but
certainly a compatible one with same major; see ML thread).

[rog, 2012-04-24 15:51:03]

https://codereview.appspot.com/6081044/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6081044/diff/2001/environs/interface.go#newcode58
environs/interface.go:58: Bootstrap(clientVersion string) error
On 2012/04/24 00:41:49, niemeyer wrote:
> Why is this necessary? Bootstrap must indeed send the client version to the
> server, but I suppose that's the version of this client, which is in fact a
> global somewhere.

two issues here:
1) It might be useful to be able to deploy a particular
client version, not just the current client's version,
for testing purposes.

2) I'm not sure of the best place to put the version number. Do we have a
package specifically to hold the version number?
I'd thought that perhaps a good place for the version number might be in the
"juju" package, but we can't refer to that here due to the circular dependency.

> Also, this version will not determine the version of juju to be run on the
> server as the docs describe above. It will allow the server to pick the
correct
> version, which may be a newer or older one depending on availability (but
> certainly a compatible one with same major; see ML thread).

I'll rephrase if we keep the argument. I'd intended that meaning. "The client
version is used to select ..."?

[niemeyer, 2012-04-24 17:53:51]

https://codereview.appspot.com/6081044/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6081044/diff/2001/environs/interface.go#newcode58
environs/interface.go:58: Bootstrap(clientVersion string) error
On Tue, Apr 24, 2012 at 12:51,  <rogpeppe@gmail.com> wrote:
> two issues here:
> 1) It might be useful to be able to deploy a particular
> client version, not just the current client's version,
> for testing purposes.

Maybe, but that's an advanced need we can cover down the road. We should get the
basics working first. This will also enable us to avoid such an open ended
branch that changes an API without connecting it to the backend or frontend in a
way we can be sure makes sense.

> 2) I'm not sure of the best place to put the version number. Do we have
> a package specifically to hold the version number?

juju/version might be a good candidate. We could have the simplified
version.Version type there, and version.Current as an instance of it.

> I'll rephrase if we keep the argument. I'd intended that meaning. "The
> client version is used to select ..."?

Yeah, that'd sound good, but I suggest getting rid of the argument, at least for
the moment.