Code review comment for lp:~fwereade/juju-core/config-6-state-service-sane-methods

*** Submitted:

state: Service ConfigSettings methods

Config, SetConfig, and SetConfigYAML methods have been dropped in favour
of
ConfigSettings and UpdateConfigSettings, which use sensible types.
Clients
are expected to parse their own damn data and supply a sensible format.

R=mue, rog
CC=
https://codereview.appspot.com/10083047

https://codereview.appspot.com/10083047/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/10083047/diff/1/cmd/juju/deploy.go#newcode95
cmd/juju/deploy.go:95: return errors.New("must deploy at least one
unit")
On 2013/06/11 12:31:23, rog wrote:
> personally i don't see why we require >0 units.

I'm trying not to get sidetracked with such questions here ;).

https://codereview.appspot.com/10083047/diff/1/state/apiserver/client.go
File state/apiserver/client.go (right):

https://codereview.appspot.com/10083047/diff/1/state/apiserver/client.go#newcode70
state/apiserver/client.go:70: changes, err :=
ch.Config().ParseSettingsYAML([]byte(p.Config), p.ServiceName)
On 2013/06/11 12:53:12, rog wrote:
> we should check with the GUI folks that they haven't put in a
workaround for the
> current behaviour before this branch lands.

Agreed before embarking on this course of action -- they just pass it
through directly when they actually use it, so they're not affected.

https://codereview.appspot.com/10083047/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/10083047/diff/1/state/service.go#newcode729
state/service.go:729: return settings.Map(), nil
On 2013/06/11 12:53:12, rog wrote:
> i'd be tempted to return settings.core to avoid yet another
unnecessary map copy
> (or implement Settings.sharedMap which just returns settings.core,
> amounting to the same thing).

> perhaps the overhead amounts to nothing in the long run though. <hits
self>.

I officially do not care about the overhead of in-memory copies of small
maps :).

https://codereview.appspot.com/10083047/diff/1/state/statecmd/get.go
File state/statecmd/get.go (right):

https://codereview.appspot.com/10083047/diff/1/state/statecmd/get.go#newcode52
state/statecmd/get.go:52: info := map[string]interface{}{
On 2013/06/11 12:53:12, rog wrote:
> this should really be a struct, i think.

I resisted the temptation to pull this whole lot into the charm
package... I think I'll leave it out for now.

https://codereview.appspot.com/10083047/diff/1/worker/uniter/context_test.go
File worker/uniter/context_test.go (right):

https://codereview.appspot.com/10083047/diff/1/worker/uniter/context_test.go#newcode582
worker/uniter/context_test.go:582: })
On 2013/06/11 12:53:12, rog wrote:
> c.Assert(err, IsNil)

Gaaah! Thank you.

https://codereview.appspot.com/10083047/