state: Continued rework of state errors for service.go.

state: Continued rework of state errors for service.go.

service.go is the next file in the rework of the errors
of the state package.

https://code.launchpad.net/~themue/juju-core/go-state-errors-service/+merge/110564

(do not edit description out of merge proposal)

[TheMue, 2012-06-15 16:19:54]

Please take a look.

[fwereade, 2012-06-15 20:47:48]

LGTM assuming first fix, and maybe a bit of thought about the second

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

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode256
state/service.go:256: return fmt.Errorf("can't mark service %q as exposed: %v",
s.Name(), err)
Copy-paste? :)

https://codereview.appspot.com/6300098/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6300098/diff/1/state/state_test.go#newcode439
state/state_test.go:439: c.Assert(err, ErrorMatches, `can't remove service
"wordpress": can't get all units from service "wordpress": environment state has
changed`)
I'm a bit surprised this doesn't have a "doesn't exist" somewhere in there (or
maybe the comment is inaccurate?).

[niemeyer, 2012-06-16 01:35:56]

Good stuff. Some comments:

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

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode31
state/service.go:31: defer errorContextf(&err, "can't get the charm URL of
service %q", s.Name())
Same idea as the unit in the other review. Let's have s.String() returning the
service name, so we can use just "s" across the board in errors and logs.

Please update all errors below after that.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode54
state/service.go:54: _, err = cn.Write()
Please trim lines below.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode131
state/service.go:131: return nil, errors.New("cannot make a principal unit
subordinate to another unit")
This message is on the poor side. I suggest improving it to:

("can't add unit of principal service %q as a subordinate of %q", s, principal)

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode145
state/service.go:145: defer errorContextf(&err, "can't remove unit from service
%q", s.Name())
That's not great for this function. UnassignFromMachine will return a nice error
by itself, and the zkRemoveTree needs special handling. It needs to say the
"can't clean" message the same way we did before, since by that time the unit
was *actually* removed.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode152
state/service.go:152: return stateChanged
return fmt.Errorf("unit not found")

We can probably kill stateChanged entirely once we're done with this
refactoring.

[TheMue, 2012-06-18 16:13:24]

Please take a look.

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

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode31
state/service.go:31: defer errorContextf(&err, "can't get the charm URL of
service %q", s.Name())
On 2012/06/16 01:35:56, niemeyer wrote:
> Same idea as the unit in the other review. Let's have s.String() returning the
> service name, so we can use just "s" across the board in errors and logs.
> 
> Please update all errors below after that.

Done.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode54
state/service.go:54: _, err = cn.Write()
On 2012/06/16 01:35:56, niemeyer wrote:
> Please trim lines below.

Done.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode131
state/service.go:131: return nil, errors.New("cannot make a principal unit
subordinate to another unit")
On 2012/06/16 01:35:56, niemeyer wrote:
> This message is on the poor side. I suggest improving it to:
> 
> ("can't add unit of principal service %q as a subordinate of %q", s,
principal)

Done.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode145
state/service.go:145: defer errorContextf(&err, "can't remove unit from service
%q", s.Name())
On 2012/06/16 01:35:56, niemeyer wrote:
> That's not great for this function. UnassignFromMachine will return a nice
error
> by itself, and the zkRemoveTree needs special handling. It needs to say the
> "can't clean" message the same way we did before, since by that time the unit
> was *actually* removed.

Done. "can't clean up data" is returned by zkRemoveTree() in case of an error.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode152
state/service.go:152: return stateChanged
On 2012/06/16 01:35:56, niemeyer wrote:
> return fmt.Errorf("unit not found")
> 
> We can probably kill stateChanged entirely once we're done with this
> refactoring.

Done. Added reminder to issue.

https://codereview.appspot.com/6300098/diff/1/state/service.go#newcode256
state/service.go:256: return fmt.Errorf("can't mark service %q as exposed: %v",
s.Name(), err)
On 2012/06/15 20:47:48, fwereade wrote:
> Copy-paste? :)

Done.

https://codereview.appspot.com/6300098/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6300098/diff/1/state/state_test.go#newcode439
state/state_test.go:439: c.Assert(err, ErrorMatches, `can't remove service
"wordpress": can't get all units from service "wordpress": environment state has
changed`)
On 2012/06/15 20:47:48, fwereade wrote:
> I'm a bit surprised this doesn't have a "doesn't exist" somewhere in there (or
> maybe the comment is inaccurate?).

Changed comment. The test tries to remove an illegal service, it has already
been removed above. So the state of the service has changed.

[dave_cheney.net, 2012-06-19 06:03:15]

Looks good. Please see my comments regarding named return values

https://codereview.appspot.com/6300098/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode69
state/service.go:69: func (s *Service) addUnit(principalKey string) (unit *Unit,
err error) {
I don't think the returned arguments should be name here. The code below always
explicitly returns 2 arguments.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode141
state/service.go:141: func (s *Service) RemoveUnit(unit *Unit) (err error) {
Ditto, err is always redefined inside the scope of the if blocks below.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode161
state/service.go:161: func (s *Service) Unit(name string) (unit *Unit, err
error) {
ditto

https://codereview.appspot.com/6300098/diff/6001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6300098/diff/6001/state/state.go#newcode206
state/state.go:206: return err
ditto.

[niemeyer, 2012-06-20 14:29:51]

Probably the last points:

https://codereview.appspot.com/6300098/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode226
state/service.go:226: var err error
This is wrong. errorContextf will not work unless this is a named result.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode273
state/service.go:273: return fmt.Errorf("can't unmark previous exposed service
%q: %v", s, err)
Error message is a big bogus. Something like this should do:

"can't clear exposed flag for service %q"

Please change the counterpart message to something similar for consistency:

"can't set exposed flag for service %q"

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode293
state/service.go:293: // String returns the service as string.
// String returns the service name.

[TheMue, 2012-06-20 14:47:46]

Please take a look.

https://codereview.appspot.com/6300098/diff/6001/state/service.go
File state/service.go (right):

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode69
state/service.go:69: func (s *Service) addUnit(principalKey string) (unit *Unit,
err error) {
On 2012/06/19 06:03:15, dfc wrote:
> I don't think the returned arguments should be name here. The code below
always
> explicitly returns 2 arguments.

The usage of errorContextf() needs the named error to decorate it.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode141
state/service.go:141: func (s *Service) RemoveUnit(unit *Unit) (err error) {
On 2012/06/19 06:03:15, dfc wrote:
> Ditto, err is always redefined inside the scope of the if blocks below.

Done.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode161
state/service.go:161: func (s *Service) Unit(name string) (unit *Unit, err
error) {
On 2012/06/19 06:03:15, dfc wrote:
> ditto

See above.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode226
state/service.go:226: var err error
On 2012/06/20 14:29:51, niemeyer wrote:
> This is wrong. errorContextf will not work unless this is a named result.

Done. Oops, don't know how this came in.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode273
state/service.go:273: return fmt.Errorf("can't unmark previous exposed service
%q: %v", s, err)
On 2012/06/20 14:29:51, niemeyer wrote:
> Error message is a big bogus. Something like this should do:
> 
> "can't clear exposed flag for service %q"
> 
> Please change the counterpart message to something similar for consistency:
> 
> "can't set exposed flag for service %q"

Done.

https://codereview.appspot.com/6300098/diff/6001/state/service.go#newcode293
state/service.go:293: // String returns the service as string.
On 2012/06/20 14:29:51, niemeyer wrote:
> // String returns the service name.

Done.

[niemeyer, 2012-06-20 15:18:18]

LGTM

[TheMue, 2012-06-20 15:21:22]

*** Submitted:

state: Continued rework of state errors for service.go.

service.go is the next file in the rework of the errors
of the state package.

R=fwereade, niemeyer, dfc
CC=
https://codereview.appspot.com/6300098