state: Service ConfigSettings methods

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.

https://code.launchpad.net/~fwereade/juju-core/config-6-state-service-sane-methods/+merge/168580

Requires: https://code.launchpad.net/~fwereade/juju-core/config-5-state-service-config-yaml/+merge/168579

(do not edit description out of merge proposal)

[fwereade, 2013-06-11 02:04:10]

Please take a look.

[mue, 2013-06-11 09:50:28]

LGTM, cleaner interface now.

[rog, 2013-06-11 12:31:23]

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")
personally i don't see why we require >0 units.

[rog, 2013-06-11 12:53:12]

LGTM

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

https://codereview.appspot.com/10083047/diff/1/juju/conn.go#newcode219
juju/conn.go:219: if err := svc.UpdateConfigSettings(settings); err != nil {
much nicer to only have one operation here, thanks.

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#newc...
state/apiserver/client.go:70: changes, err :=
ch.Config().ParseSettingsYAML([]byte(p.Config), p.ServiceName)
we should check with the GUI folks that they haven't put in a workaround for the
current behaviour before this branch lands.

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

https://codereview.appspot.com/10083047/diff/1/state/service.go#oldcode777
state/service.go:777: func (s *Service) SetConfigYAML(yamlData []byte) error {
it's gone!

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
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>.

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{}{
this should really be a struct, i think.

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#...
worker/uniter/context_test.go:582: })
c.Assert(err, IsNil)

[fwereade, 2013-06-12 01:17:31]

*** 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#newc...
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#...
worker/uniter/context_test.go:582: })
On 2013/06/11 12:53:12, rog wrote:
> c.Assert(err, IsNil)

Gaaah! Thank you.