|
|
|
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)
Patch Set 1 #
Total comments: 4
Patch Set 2 : environs/configstore: add extra config info #
Total comments: 4
Patch Set 3 : environs/configstore: add extra config info #
Total messages: 6
|
rog
Please take a look.
|
12 years, 3 months ago (2013年09月25日 16:27:14 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
Please take a look.
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 { ;)
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.
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?
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.