state: remove most AddUnitSubordinateTo

state: remove most AddUnitSubordinateTo

machine_test.go is not yet done because it scares me; expect a followup
handling that and finally dropping AUST.

https://code.launchpad.net/~fwereade/juju-core/strip-most-aust/+merge/143472

(do not edit description out of merge proposal)

[fwereade, 2013-01-16 10:55:36]

Please take a look.

[rog, 2013-01-16 16:35:29]

looks fine in general, with one remark below.

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode806
state/relation_test.go:806: func addRU(c *C, svc *state.Service, rel
*state.Relation, principal *state.Unit) (*state.Unit, *state.RelationUnit) {
i'd quite like a comment on this function. presumably the relation is a
container-scoped relation with the principal unit?

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode823
state/relation_test.go:823: for _, u = range newUnits {
this loop doesn't appear to do anything

[fwereade, 2013-01-16 16:51:15]

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode806
state/relation_test.go:806: func addRU(c *C, svc *state.Service, rel
*state.Relation, principal *state.Unit) (*state.Unit, *state.RelationUnit) {
On 2013/01/16 16:35:29, rog wrote:
> i'd quite like a comment on this function. presumably the relation is a
> container-scoped relation with the principal unit?

Pondering:

// Given the service svc in the relation rel, add a unit of
// svc and create a RelationUnit with rel. If principal is
// supplied, svc is assumed to be subordinate and the unit
// will be created by temporarily entering the relation's
// scope as the principal.

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode823
state/relation_test.go:823: for _, u = range newUnits {
On 2013/01/16 16:35:29, rog wrote:
> this loop doesn't appear to do anything

It leaves a sensible value of `u` by stopping once it finds a unit that didn't
already exist; but, yeah, it could probably use a comment.

[rog, 2013-01-16 17:00:09]

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode806
state/relation_test.go:806: func addRU(c *C, svc *state.Service, rel
*state.Relation, principal *state.Unit) (*state.Unit, *state.RelationUnit) {
On 2013/01/16 16:51:16, fwereade wrote:
> On 2013/01/16 16:35:29, rog wrote:
> > i'd quite like a comment on this function. presumably the relation is a
> > container-scoped relation with the principal unit?
> 
> Pondering:
> 
> // Given the service svc in the relation rel, add a unit of
> // svc and create a RelationUnit with rel. If principal is
> // supplied, svc is assumed to be subordinate and the unit
> // will be created by temporarily entering the relation's
> // scope as the principal.

That's much better, thanks.

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode823
state/relation_test.go:823: for _, u = range newUnits {
On 2013/01/16 16:51:16, fwereade wrote:
> On 2013/01/16 16:35:29, rog wrote:
> > this loop doesn't appear to do anything
> 
> It leaves a sensible value of `u` by stopping once it finds a unit that didn't
> already exist; but, yeah, it could probably use a comment.

ah, the crucial missing ":"!

might be clearer to create another variable and assign to u at the right point.

or perhaps something like this:

  // Find the newly made subordinate.
  for _, newu := range newUnits {
     if !hasUnitName(origUnits, newu.Name()) {
         u = newu
         break
     }
   }


where
func hasUnitName(units []*state.Unit, name string) bool {
    for _, u := range units {
        if u.Name() == name {
             return true
        }
    }
    return false
}

[fwereade, 2013-01-17 10:06:17]

Please take a look.

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go
File state/relation_test.go (right):

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode806
state/relation_test.go:806: func addRU(c *C, svc *state.Service, rel
*state.Relation, principal *state.Unit) (*state.Unit, *state.RelationUnit) {
On 2013/01/16 17:00:09, rog wrote:
> On 2013/01/16 16:51:16, fwereade wrote:
> > On 2013/01/16 16:35:29, rog wrote:
> > > i'd quite like a comment on this function. presumably the relation is a
> > > container-scoped relation with the principal unit?
> > 
> > Pondering:
> > 
> > // Given the service svc in the relation rel, add a unit of
> > // svc and create a RelationUnit with rel. If principal is
> > // supplied, svc is assumed to be subordinate and the unit
> > // will be created by temporarily entering the relation's
> > // scope as the principal.
> 
> That's much better, thanks.

Done.

https://codereview.appspot.com/7137044/diff/1/state/relation_test.go#newcode823
state/relation_test.go:823: for _, u = range newUnits {
On 2013/01/16 17:00:09, rog wrote:
> On 2013/01/16 16:51:16, fwereade wrote:
> > On 2013/01/16 16:35:29, rog wrote:
> > > this loop doesn't appear to do anything
> > 
> > It leaves a sensible value of `u` by stopping once it finds a unit that
didn't
> > already exist; but, yeah, it could probably use a comment.
> 
> ah, the crucial missing ":"!
> 
> might be clearer to create another variable and assign to u at the right
point.
> 
> or perhaps something like this:
> 
>   // Find the newly made subordinate.
>   for _, newu := range newUnits {
>      if !hasUnitName(origUnits, newu.Name()) {
>          u = newu
>          break
>      }
>    }
> 
> 
> where
> func hasUnitName(units []*state.Unit, name string) bool {
>     for _, u := range units {
>         if u.Name() == name {
>              return true
>         }
>     }
>     return false
> }

Done.

[rog, 2013-01-17 10:31:07]

LGTM

[TheMue, 2013-01-17 11:34:49]

LGTM

[fwereade, 2013-01-17 11:43:57]

*** Submitted:

state: remove most AddUnitSubordinateTo

machine_test.go is not yet done because it scares me; expect a followup
handling that and finally dropping AUST.

R=rog, TheMue
CC=
https://codereview.appspot.com/7137044