juju: remove dependency on api/params

juju: remove dependency on api/params

The juju package should really not depend on api/params
if possible. We move two functions into State and
delete the now-redundant statecmd shim functions.

We also fix bug 1167344 in passing.

The only logic change in this branch is to remove
"no options to set" check from Service.Set.
IMHO setting no options should be acceptable.

https://code.launchpad.net/~rogpeppe/juju-core/278-juju-no-params/+merge/158137

(do not edit description out of merge proposal)

[rog, 2013-04-10 15:07:44]

Please take a look.

[rog, 2013-04-10 15:08:19]

Please take a look.

[fwereade, 2013-04-10 16:24:24]

LGTM with the following resolved. Ping me if you disagree :)

https://codereview.appspot.com/8626043/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8626043/diff/5001/juju/conn.go#newcode202
juju/conn.go:202: // subsequent operations?
Hmm. Maybe. But we can't eliminate the possibility of half-deployed services
even if we do, so I'm not that bothered. Better to move towards more
comprehensive txns in the long term, I think.

https://codereview.appspot.com/8626043/diff/5001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/service.go#newcode696
state/service.go:696: func (s *Service) Set(options map[string]string) error {
I think I would prefer SetConfig... or even a free SetServiceConfig function, I
feel that there's a qualitative difference between something like this that's
implemented entirely in terms of the existing exported API.

https://codereview.appspot.com/8626043/diff/5001/state/service.go#newcode739
state/service.go:739: func (s *Service) SetYAML(yamlData []byte) error {
SetServiceConfigYAML?

https://codereview.appspot.com/8626043/diff/5001/state/service.go#newcode742
state/service.go:742: if err := goyaml.Unmarshal(yamlData, &options); err != nil
{
This is broken. According to python, the yaml is a
map[string]map[string]interface{}, with the top-level keys being service names
and the sub-dicts containing actual values (or maybe strings that'll be
magically turned into the values, python doesn't mind). If this isn't fixed
here, please bug/card it, it's a wanton incompatibility.

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go#newcod...
state/service_test.go:246: about: "bad configuration",
All the YAML tests are testing incorrect behaviour.

https://codereview.appspot.com/8626043/diff/5001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/unit.go#newcode940
state/unit.go:940: func (u *Unit) Resolve(retryHooks bool) error {
func ResolveUnit(...

[dimitern, 2013-04-10 16:28:31]

LGTM, nice to see temporary and redundant code going away.

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go#newcod...
state/service_test.go:201: err:   `Unknown configuration option: "foo"`,
why is this error starts with a capital letter?

https://codereview.appspot.com/8626043/diff/5001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/unit.go#newcode940
state/unit.go:940: func (u *Unit) Resolve(retryHooks bool) error {
On 2013/04/10 16:24:24, fwereade wrote:
> func ResolveUnit(...

Why? It's a method of the unit anyway?

[rog, 2013-04-10 17:40:35]

Please take a look.

https://codereview.appspot.com/8626043/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/8626043/diff/5001/juju/conn.go#newcode202
juju/conn.go:202: // subsequent operations?
On 2013/04/10 16:24:24, fwereade wrote:
> Hmm. Maybe. But we can't eliminate the possibility of half-deployed services
> even if we do, so I'm not that bothered. Better to move towards more
> comprehensive txns in the long term, I think.

added a TODO.

https://codereview.appspot.com/8626043/diff/5001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/service.go#newcode696
state/service.go:696: func (s *Service) Set(options map[string]string) error {
On 2013/04/10 16:24:24, fwereade wrote:
> I think I would prefer SetConfig...

done.

> or even a free SetServiceConfig function, I
> feel that there's a qualitative difference between something like this that's
> implemented entirely in terms of the existing exported API.

i don't agree.

it's a "convenience" method, sure, but i don't think the caller needs to know
whether or not it's implemented in terms of the existing exported API.

https://codereview.appspot.com/8626043/diff/5001/state/service.go#newcode739
state/service.go:739: func (s *Service) SetYAML(yamlData []byte) error {
On 2013/04/10 16:24:24, fwereade wrote:
> SetServiceConfigYAML?

SetConfigYAML

https://codereview.appspot.com/8626043/diff/5001/state/service.go#newcode742
state/service.go:742: if err := goyaml.Unmarshal(yamlData, &options); err != nil
{
On 2013/04/10 16:24:24, fwereade wrote:
> This is broken. According to python, the yaml is a
> map[string]map[string]interface{}, with the top-level keys being service names
> and the sub-dicts containing actual values (or maybe strings that'll be
> magically turned into the values, python doesn't mind). If this isn't fixed
> here, please bug/card it, it's a wanton incompatibility.

lp#1167465

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go
File state/service_test.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go#newcod...
state/service_test.go:201: err:   `Unknown configuration option: "foo"`,
On 2013/04/10 16:28:31, dimitern wrote:
> why is this error starts with a capital letter?

because that's the way all the charm.Config.Validate errors look. feel free to
fix that in another CL if you like.

https://codereview.appspot.com/8626043/diff/5001/state/service_test.go#newcod...
state/service_test.go:246: about: "bad configuration",
On 2013/04/10 16:24:24, fwereade wrote:
> All the YAML tests are testing incorrect behaviour.

filed a bug.

https://codereview.appspot.com/8626043/diff/5001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/8626043/diff/5001/state/unit.go#newcode940
state/unit.go:940: func (u *Unit) Resolve(retryHooks bool) error {
On 2013/04/10 16:28:31, dimitern wrote:
> On 2013/04/10 16:24:24, fwereade wrote:
> > func ResolveUnit(...
> 
> Why? It's a method of the unit anyway?

+1

[rog, 2013-04-10 17:46:13]

i believe we discussed all the issues except the method vs. function issue.

since a difference between a method and a function is trivial in Go, I'm
submitting with methods for now. if you feel strongly, we can easily change them
to functions later.

[rog, 2013-04-10 17:49:22]

*** Submitted:

juju: remove dependency on api/params

The juju package should really not depend on api/params
if possible. We move two functions into State and
delete the now-redundant statecmd shim functions.

We also fix bug 1167344 in passing.

The only logic change in this branch is to remove
"no options to set" check from Service.Set.
IMHO setting no options should be acceptable.

R=fwereade, dimitern
CC=
https://codereview.appspot.com/8626043