Code review comment for lp:~dimitern/juju-core/148-apiuniter-fix-configsettings

Revision history for this message
William Reade (fwereade) wrote :

LGTM with an error return on unexpected relation settings.

https://codereview.appspot.com/13908044/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/13908044/diff/1/state/api/params/internal.go#newcode138
state/api/params/internal.go:138: // EnvironConfig holds an environment
configuration.
Man, our vocabulary is still kinda screwed up here... "Config" means too
many things. Not much we can do about it right now though.

https://codereview.appspot.com/13908044/diff/1/state/api/uniter/unit.go
File state/api/uniter/unit.go (right):

https://codereview.appspot.com/13908044/diff/1/state/api/uniter/unit.go#newcode143
state/api/uniter/unit.go:143: return
map[string]interface{}(result.Settings), nil
can you not do charm.Settings(result.Settings)?

https://codereview.appspot.com/13908044/diff/1/state/apiserver/uniter/uniter.go
File state/apiserver/uniter/uniter.go (left):

https://codereview.appspot.com/13908044/diff/1/state/apiserver/uniter/uniter.go#oldcode797
state/apiserver/uniter/uniter.go:797: // All relation settings should be
strings.
Hmm. This is almost legitimate-panic territory. Best not, since we're
running in the API server and will cause a lot of collateral damage, but
I think we should at least error out -- if it happens, we're clearly not
in Kansas any more.

https://codereview.appspot.com/13908044/

« Back to merge proposal