config: agent-version now defaults to current

config: agent-version now defaults to current

This doesn't affect behaviour yet, because it's still explicitly reset in
environs.BootstrapConfig; but this will change.

https://code.launchpad.net/~fwereade/juju-core/config-default-agent-version/+merge/150248

Requires: https://code.launchpad.net/~fwereade/juju-core/environs-separate-agent-tools/+merge/150245

(do not edit description out of merge proposal)

[fwereade, 2013-02-24 22:37:09]

Please take a look.

[dimitern, 2013-02-25 07:51:19]

LGTM

[jameinel, 2013-02-25 16:35:12]

LGTM

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:228: v := c.m["agent-version"].(string)
Is there a reason you got rid of the 'ok' check? if it isn't a (string) we'll
get a panic, but I suppose we can be pretty confident that config must hold
strings?

[rog, 2013-02-25 16:59:05]

LGTM with one trivial and one minor consideration below.

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:45: //  ~/.ssh/id_dsa.pub
aren't we allowed tabs in comments now?
seems reasonable, but please replace a tab with at least 4 spaces when
replacing.

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:228: v := c.m["agent-version"].(string)
On 2013/02/25 16:35:12, jameinel wrote:
> Is there a reason you got rid of the 'ok' check? if it isn't a (string) we'll
> get a panic, but I suppose we can be pretty confident that config must hold
> strings?

the changes mean that it now always holds a string. that wasn't the case
previously. this looks ok to me.

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:299: "agent-version":            
version.Current.Number.String(),
i'm not *entirely* convinced this is right. some tests meddle with
version.Current, and this will cause the original version to be returned always.

perhaps this should be filled in at New time. i can't quite decide.

that would make it easy to do the parsing check too though.

[fwereade, 2013-02-25 17:32:03]

*** Submitted:

config: agent-version now defaults to current

This doesn't affect behaviour yet, because it's still explicitly reset in
environs.BootstrapConfig; but this will change.

R=dimitern, jameinel, rog
CC=
https://codereview.appspot.com/7396056

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:45: //  ~/.ssh/id_dsa.pub
On 2013/02/25 16:59:05, rog wrote:
> aren't we allowed tabs in comments now?
> seems reasonable, but please replace a tab with at least 4 spaces when
> replacing.

Whoops, sorry -- my editor is replacing them. I do kinda favour spaces though;
I'll fix them as suggested.

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:228: v := c.m["agent-version"].(string)
On 2013/02/25 16:59:05, rog wrote:
> On 2013/02/25 16:35:12, jameinel wrote:
> > Is there a reason you got rid of the 'ok' check? if it isn't a (string)
we'll
> > get a panic, but I suppose we can be pretty confident that config must hold
> > strings?
> 
> the changes mean that it now always holds a string. that wasn't the case
> previously. this looks ok to me.

Yeah, and it's a bit more in line with the other fields too now.

https://codereview.appspot.com/7396056/diff/1/environs/config/config.go#newco...
environs/config/config.go:299: "agent-version":            
version.Current.Number.String(),
On 2013/02/25 16:59:05, rog wrote:
> i'm not *entirely* convinced this is right. some tests meddle with
> version.Current, and this will cause the original version to be returned
always.
> 
> perhaps this should be filled in at New time. i can't quite decide.
> 
> that would make it easy to do the parsing check too though.

I think you're right, better to fill it in at New time. Thanks.