Code review - Issue 8626043: juju: remove dependency on api/paramshttps://codereview.appspot.com/2013-04-10T17:49:22+00:00rietveld
Message from unknown
2013-04-10T14:59:55+00:00rogurn:md5:0f19b0b32a4eddcc4bc5131df41eb497
Message from unknown
2013-04-10T15:07:41+00:00rogurn:md5:7e86053e6ed05ab172a864ce46d8e956
Message from rogpeppe@gmail.com
2013-04-10T15:07:44+00:00rogurn:md5:b3a3ce7d2d739ab54500fee845f930f8
Please take a look.
Message from unknown
2013-04-10T15:08:16+00:00rogurn:md5:a7fccf3cf4d0f055203c730284e2687d
Message from rogpeppe@gmail.com
2013-04-10T15:08:19+00:00rogurn:md5:b2b6d344ddaebfdb45f1a0926bfe4bd4
Please take a look.
Message from fwereade@gmail.com
2013-04-10T16:24:24+00:00fwereadeurn:md5:9f20f0e1ce62baa1581a449bdaab8602
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#newcode246
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(...
Message from dimiter.naydenov@canonical.com
2013-04-10T16:28:31+00:00dimiternurn:md5:3ce555300ac02fb9fb167727498b16b9
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#newcode201
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?
Message from unknown
2013-04-10T17:40:31+00:00rogurn:md5:8246af457d356d1ecd933187bf590164
Message from rogpeppe@gmail.com
2013-04-10T17:40:35+00:00rogurn:md5:d812d779de3f630b740eaa4397fbe864
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#newcode201
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#newcode246
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
Message from rogpeppe@gmail.com
2013-04-10T17:46:13+00:00rogurn:md5:888dd26cf9ffcb586599869980601a21
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.
Message from unknown
2013-04-10T17:48:33+00:00rogurn:md5:e36f213eaba007b752f9e337a355b3ff
Message from rogpeppe@gmail.com
2013-04-10T17:49:22+00:00rogurn:md5:1437c561a2c9159cbae618d3a30ed42d
*** 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