environs/configstore: add extra config info

environs/configstore: add extra config info



https://code.launchpad.net/~rogpeppe/juju-core/420-configstore-extraconfig/+merge/187551

(do not edit description out of merge proposal)

[rog, 2013-09-25 16:27:14]

Please take a look.

[rog, 2013-09-25 16:33:50]

Please take a look.

[mue, 2013-09-25 16:38:39]

LGTM, only a minor comment.

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/interf...
File environs/configstore/interface.go (right):

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/interf...
environs/configstore/interface.go:56: ExtraConfig() map[string]interface{}
Would like a 

type ExtraConfig map[string]interface{} 

and then changed signatures here and in SetExtraConfig() (and their
implementations).

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/mem.go
File environs/configstore/mem.go (right):

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/mem.go...
environs/configstore/mem.go:41: func NewMem() Storage {
;)

[gz, 2013-09-25 16:42:14]

Seems okay, though I'm not super clear on the overall intention here.

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/disk.g...
environs/configstore/disk.go:31: created      bool
Not wild about this mechanism, it's not very clear what the flag actually
indicates. That it's a fresh config, rather than a loaded one, right? Seems like
the same thing could be done by making Config nilable, but I guess that's not
really any more elegant.

[fwereade, 2013-09-25 17:15:09]

LGTM despite mildly suspicious question

https://codereview.appspot.com/13912043/diff/1/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/13912043/diff/1/environs/configstore/disk.go#n...
environs/configstore/disk.go:112: return info.Config
copy?

https://codereview.appspot.com/13912043/diff/1/environs/configstore/disk.go#n...
environs/configstore/disk.go:134: panic("extra config set on environment info
that has not just been created")
Hmm. I think there are at least two places this makes sense.

1) on creation, containing only the fields necessary to bootstrap
2) on bootstrap, containing only the fields necessary to make contact with the
environ and complete bootstrap in the absence of valid state-servers

Are these really definitely the same things?

https://codereview.appspot.com/13912043/diff/1/environs/configstore/disk.go#n...
environs/configstore/disk.go:136: info.Config = attrs
copy?

[rog, 2013-09-26 09:11:03]

Please take a look.

https://codereview.appspot.com/13912043/diff/1/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/13912043/diff/1/environs/configstore/disk.go#n...
environs/configstore/disk.go:112: return info.Config
On 2013/09/25 17:15:09, fwereade wrote:
> copy?

My thought about this (and the other copy suggestion) is that
this is a mutable type, and it's the responsibility
of its user to avoid mutating it inappropriately.

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/disk.go
File environs/configstore/disk.go (right):

https://codereview.appspot.com/13912043/diff/4001/environs/configstore/disk.g...
environs/configstore/disk.go:31: created      bool
On 2013/09/25 16:42:14, gz wrote:
> Not wild about this mechanism, it's not very clear what the flag actually
> indicates. That it's a fresh config, rather than a loaded one, right? Seems
like
> the same thing could be done by making Config nilable, but I guess that's not
> really any more elegant.

Well, Config *is* nilable, and it's quite likely that it'll be nil after reading
the info from an external source; whereas the created field cannot be affected
by unmarshalling.

I've added a comment.